[v3,1/1] qga: minimal support for fstrim for Windows guests
diff mbox

Message ID 1475496534-5728-1-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Oct. 3, 2016, 12:08 p.m. UTC
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 <den@openvz.org>
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
CC: Marc-André Lureau <marcandre.lureau@gmail.com>
---
 qga/commands-win32.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 92 insertions(+), 3 deletions(-)

Changes from v1, v2:
- next attempt to fix error handling on error in FindFirstVolumeW

Comments

Marc-André Lureau Oct. 3, 2016, 12:22 p.m. UTC | #1
Hi Denis

On Mon, Oct 3, 2016 at 4:08 PM Denis V. Lunev <den@openvz.org> 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.
>

Could you describe the changes the made between the versions and why? (btw
I didn't receive reply to my previous review comments)

thanks


> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> CC: Stefan Weil <sw@weilnetz.de>
> CC: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>  qga/commands-win32.c | 95
> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 92 insertions(+), 3 deletions(-)
>
> 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..57436b9 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -840,8 +840,97 @@ 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 =
> g_new0(GuestFilesystemTrimResponse, 1);
> +    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;
> +    }
> +
> +    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_malloc0(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 +1505,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
Denis V. Lunev Oct. 3, 2016, 12:39 p.m. UTC | #2
On 10/03/2016 03:22 PM, Marc-André Lureau wrote:
> Hi Denis
>
> On Mon, Oct 3, 2016 at 4:08 PM Denis V. Lunev <den@openvz.org> 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.
>>
> Could you describe the changes the made between the versions and why? (btw
> I didn't receive reply to my previous review comments)
>
> thanks
>
>

The only change with this patch is error code for FindFirstVolumeW,
which is now generated exactly like in the case inside
qmp_guest_get_fsinfo()

I do not want to reuse this code as this will result in 2 line helper.
I do not see much benefit with that. On the other hand,
qmp_guest_get_fsinfo() should be rewritten to use unicode. I have
started doing that but this work is not completed. These
patches have revealed serious problems inside get_fsinfo(),
which is completely broken now.

But I have missed the fact, that this exact patch is rotten,
I have missed de-allocation on the error path, pls disregard.

Thank you for a prompt reply.

Den

>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> CC: Stefan Weil <sw@weilnetz.de>
>> CC: Marc-André Lureau <marcandre.lureau@gmail.com>
>> ---
>>  qga/commands-win32.c | 95
>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 92 insertions(+), 3 deletions(-)
>>
>> 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..57436b9 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -840,8 +840,97 @@ 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 =
>> g_new0(GuestFilesystemTrimResponse, 1);
>> +    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;
>> +    }
>> +
>> +    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_malloc0(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 +1505,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
Stefan Weil Oct. 3, 2016, 12:52 p.m. UTC | #3
See two comments below.

On 10/03/16 14:08, 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.
>
> This is working since Win8 and Win2k12.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> CC: Stefan Weil <sw@weilnetz.de>
> CC: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>  qga/commands-win32.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 92 insertions(+), 3 deletions(-)
>
> 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..57436b9 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -840,8 +840,97 @@ 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 = g_new0(GuestFilesystemTrimResponse, 1);
> +    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");

This is leaking memory allocated for resp.

> +        return NULL;
> +    }
> +
> +    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_malloc0(sizeof(WCHAR) * char_count);

I suggest to use g_new here (no need to fill the memory with 0).

Regards
Stefan

Patch
diff mbox

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9c9be12..57436b9 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -840,8 +840,97 @@  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 = g_new0(GuestFilesystemTrimResponse, 1);
+    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;
+    }
+
+    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_malloc0(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 +1505,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) {