Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests

[Denis V. Lunev]

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

>     +            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