[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Wind
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests |
Date: |
Tue, 25 Oct 2016 18:53:51 -0500 |
User-agent: |
alot/0.3.6 |
Quoting Denis V. Lunev (2016-10-05 06:13:12)
> On 10/04/2016 04:43 PM, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Oct 3, 2016 at 6:01 PM Denis V. Lunev <address@hidden
> > <mailto:address@hidden>> wrote:
> >
> > Unfortunately, there is no public Windows API to start trimming the
> > filesystem. The only viable way here is to call 'defrag.exe /L' for
> > each volume.
> >
> > This is working since Win8 and Win2k12.
> >
> > Signed-off-by: Denis V. Lunev <address@hidden <mailto:address@hidden>>
> > Signed-off-by: Denis Plotnikov <address@hidden
> > <mailto:address@hidden>>
> > CC: Michael Roth <address@hidden
> > <mailto:address@hidden>>
> > CC: Stefan Weil <address@hidden <mailto:address@hidden>>
> > CC: Marc-André Lureau <address@hidden
> > <mailto:address@hidden>>
> >
> >
> > overall looks good to me, few remarks below:
> >
> >
> > ---
> > qga/commands-win32.c | 97
> > ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 94 insertions(+), 3 deletions(-)
> >
> > Changes from v3:
> > - fixed memory leak on error path for FindFirstVolumeW
> > - replaced g_malloc0 with g_malloc for uc_path. g_malloc is better
> > as we are
> > allocating string, not an object
> >
> > Changes from v1, v2:
> > - next attempt to fix error handling on error in FindFirstVolumeW
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 9c9be12..cebf4cc 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -840,8 +840,99 @@ static void guest_fsfreeze_cleanup(void)
> > GuestFilesystemTrimResponse *
> > qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> > {
> > - error_setg(errp, QERR_UNSUPPORTED);
> > - return NULL;
> > + GuestFilesystemTrimResponse *resp;
> > + HANDLE handle;
> > + WCHAR guid[MAX_PATH] = L"";
> > +
> > + handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
> > + if (handle == INVALID_HANDLE_VALUE) {
> > + error_setg_win32(errp, GetLastError(), "failed to find
> > any volume");
> > + return NULL;
> > + }
> > +
> > + resp = g_new0(GuestFilesystemTrimResponse, 1);
> > +
> > + do {
> > + GuestFilesystemTrimResult *res;
> > + GuestFilesystemTrimResultList *list;
> > + PWCHAR uc_path;
> > + DWORD char_count = 0;
> > + char *path, *out;
> > + GError *gerr = NULL;
> > + gchar * argv[4];
> > +
> > + GetVolumePathNamesForVolumeNameW(guid, NULL, 0, &char_count);
> > +
> >
> >
> > It assumes GetVolumePathNamesForVolumeNameW() == 0, perhaps better be
> > explicit about it with an assert() or a warning()?
> original assumption was that in this case we'll call
> GetVolumePathNamesForVolumeNameW()
> with the exactly the same parameter set and fail there.
>
>
> >
> > + if (GetLastError() != ERROR_MORE_DATA) {
> >
> >
> > Would it be useful to log the error in this case?
> >
> >
> > + continue;
> > + }
> > + if (GetDriveTypeW(guid) != DRIVE_FIXED) {
> > + continue;
> > + }
> > +
> > + uc_path = g_malloc(sizeof(WCHAR) * char_count);
> >
> > + if (!GetVolumePathNamesForVolumeNameW(guid, uc_path,
> > char_count,
> > + &char_count) ||
> > !*uc_path) {
> > + /* strange, but this condition could be faced even
> > with size == 2 */
> >
> >
> > What size?
> >
> with char_count == 2
>
> > Same remark regarding logging error.
> >
> > + g_free(uc_path);
> > + continue;
> > + }
> > +
> > + res = g_new0(GuestFilesystemTrimResult, 1);
> > +
> > + path = g_utf16_to_utf8(uc_path, char_count, NULL, NULL,
> > &gerr);
> > +
> > + g_free(uc_path);
> > +
> > + if (gerr != NULL && gerr->code) {
> >
> >
> > Why check gerr->code? To be consistent with error checking code, I
> > would check if path == NULL instead, which by glib doc says that gerr
> > will be set in this case.
> >
> ok
Thanks, applied to qga tree with the above suggestion squashed in:
https://github.com/mdroth/qemu/commits/qga
>
> > + res->has_error = true;
> > + res->error = g_strdup(gerr->message);
> > + g_error_free(gerr);
> > + break;
> > + }
> > +
> > + res->path = path;
> > +
> > + list = g_new0(GuestFilesystemTrimResultList, 1);
> > + list->value = res;
> > + list->next = resp->paths;
> > +
> > + resp->paths = list;
> > +
> > + memset(argv, 0, sizeof(argv));
> > + argv[0] = (gchar *)"defrag.exe";
> > + argv[1] = (gchar *)"/L";
> > + argv[2] = path;
> > +
> > + if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH,
> > NULL, NULL,
> > + &out /* stdout */, NULL /* stdin */,
> > + NULL, &gerr)) {
> > + res->has_error = true;
> > + res->error = g_strdup(gerr->message);
> > + g_error_free(gerr);
> >
> >
> > It could use continue; here, like the other error code paths, to avoid
> > the else indent?
> I need indent for local variable
>
> >
> >
> > + } else {
> > + /* defrag.exe is UGLY. Exit code is ALWAYS zero.
> > + Error is reported in the output with something like
> > + (x89000020) etc code in the stdout */
> > +
> > + int i;
> > + gchar **lines = g_strsplit(out, "\r\n", 0);
> > + g_free(out);
> > +
> > + for (i = 0; lines[i] != NULL; i++) {
> > + if (g_strstr_len(lines[i], -1, "(0x") == NULL) {
> > + continue;
> > + }
> > + res->has_error = true;
> > + res->error = g_strdup(lines[i]);
> > + break;
> > + }
> > + g_strfreev(lines);
> > + }
> > + } while (FindNextVolumeW(handle, guid, ARRAYSIZE(guid)));
> > +
> > + FindVolumeClose(handle);
> > + return resp;
> > }
> >
> > typedef enum {
> > @@ -1416,7 +1507,7 @@ GList *ga_command_blacklist_init(GList
> > *blacklist)
> > "guest-get-memory-blocks", "guest-set-memory-blocks",
> > "guest-get-memory-block-size",
> > "guest-fsfreeze-freeze-list",
> > - "guest-fstrim", NULL};
> > + NULL};
> > char **p = (char **)list_unsupported;
> >
> > while (*p) {
> > --
> > 2.7.4
> >
> > --
> > Marc-André Lureau
>
>