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

[Richard W.M. Jones]

On Mon, Oct 03, 2016 at 05:01:25PM +0300, Denis V. Lunev 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.

It's good to know that at least in one way, ntfs-3g is more featureful
than Windows :-)

> This is working since Win8 and Win2k12.
>
> Signed-off-by: Denis V. Lunev <address@hidden>
> Signed-off-by: Denis Plotnikov <address@hidden>
> CC: Michael Roth <address@hidden>
> CC: Stefan Weil <address@hidden>
> CC: Marc-André Lureau <address@hidden>
> ---
>  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);
> +
> +        if (GetLastError() != ERROR_MORE_DATA) {
> +            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 */
> +            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) {
> +            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);
> +        } 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) {

The patch looks good to me.  It's a bit of a shame that we have to
grep the output for "(0x" and hope that that is the only way that
error can be reported, but not much else we can do.

  Reviewed-by: Richard W.M. Jones <address@hidden>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW