* Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests
2016-10-03 14:01 [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests Denis V. Lunev
@ 2016-10-04 13:43 ` Marc-André Lureau
2016-10-05 11:13 ` Denis V. Lunev
2016-10-05 13:51 ` Richard W.M. Jones
2016-10-05 18:55 ` Laszlo Ersek
2 siblings, 1 reply; 8+ messages in thread
From: Marc-André Lureau @ 2016-10-04 13:43 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel; +Cc: Denis Plotnikov, Michael Roth, Stefan Weil
Hi
On Mon, Oct 3, 2016 at 6:01 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.
>
> 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>
>
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()?
+ 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?
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.
> + 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?
> + } 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests
2016-10-04 13:43 ` Marc-André Lureau
@ 2016-10-05 11:13 ` Denis V. Lunev
2016-10-25 23:53 ` Michael Roth
0 siblings, 1 reply; 8+ messages in thread
From: Denis V. Lunev @ 2016-10-05 11:13 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: Denis Plotnikov, Michael Roth, Stefan Weil
On 10/04/2016 04:43 PM, Marc-André Lureau wrote:
> Hi
>
> On Mon, Oct 3, 2016 at 6:01 PM Denis V. Lunev <den@openvz.org
> <mailto: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.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org <mailto:den@openvz.org>>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com
> <mailto:dplotnikov@virtuozzo.com>>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com
> <mailto:mdroth@linux.vnet.ibm.com>>
> CC: Stefan Weil <sw@weilnetz.de <mailto:sw@weilnetz.de>>
> CC: Marc-André Lureau <marcandre.lureau@gmail.com
> <mailto:marcandre.lureau@gmail.com>>
>
>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests
2016-10-05 11:13 ` Denis V. Lunev
@ 2016-10-25 23:53 ` Michael Roth
0 siblings, 0 replies; 8+ messages in thread
From: Michael Roth @ 2016-10-25 23:53 UTC (permalink / raw)
To: Denis V. Lunev, Marc-André Lureau, qemu-devel
Cc: Denis Plotnikov, Stefan Weil
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 <den@openvz.org
> > <mailto: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.
> >
> > Signed-off-by: Denis V. Lunev <den@openvz.org <mailto:den@openvz.org>>
> > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com
> > <mailto:dplotnikov@virtuozzo.com>>
> > CC: Michael Roth <mdroth@linux.vnet.ibm.com
> > <mailto:mdroth@linux.vnet.ibm.com>>
> > CC: Stefan Weil <sw@weilnetz.de <mailto:sw@weilnetz.de>>
> > CC: Marc-André Lureau <marcandre.lureau@gmail.com
> > <mailto:marcandre.lureau@gmail.com>>
> >
> >
> > 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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests
2016-10-03 14:01 [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests Denis V. Lunev
2016-10-04 13:43 ` Marc-André Lureau
@ 2016-10-05 13:51 ` Richard W.M. Jones
2016-10-05 18:55 ` Laszlo Ersek
2 siblings, 0 replies; 8+ messages in thread
From: Richard W.M. Jones @ 2016-10-05 13:51 UTC (permalink / raw)
To: Denis V. Lunev
Cc: qemu-devel, Denis Plotnikov, Marc-André Lureau, Michael Roth,
Stefan Weil
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 <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 | 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 <rjones@redhat.com>
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests
2016-10-03 14:01 [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests Denis V. Lunev
2016-10-04 13:43 ` Marc-André Lureau
2016-10-05 13:51 ` Richard W.M. Jones
@ 2016-10-05 18:55 ` Laszlo Ersek
2016-10-05 19:47 ` Denis V. Lunev
2 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2016-10-05 18:55 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel
Cc: Denis Plotnikov, Marc-André Lureau, Michael Roth,
Stefan Weil
On 10/03/16 16:01, 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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 94 insertions(+), 3 deletions(-)
Just to educate myself (really, you can ignore my question as "review
comment", because it's not one): why is this necessary? In Windows 8 and
Windows Server 2012 R2, simply putting your NTFS filesystem on a SCSI
disk (such as virtio-scsi-pci / scsi-hd) or AHCI, and specifying
discard=on for the -drive, will result in the guest automatically
trimming the NTFS (with a little delay) after deleting files, and the
host getting those blocks back.
Thanks
Laszlo
> 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) {
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests
2016-10-05 18:55 ` Laszlo Ersek
@ 2016-10-05 19:47 ` Denis V. Lunev
2016-10-05 19:56 ` Laszlo Ersek
0 siblings, 1 reply; 8+ messages in thread
From: Denis V. Lunev @ 2016-10-05 19:47 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel
Cc: Denis Plotnikov, Marc-André Lureau, Michael Roth,
Stefan Weil
On 10/05/2016 09:55 PM, Laszlo Ersek wrote:
> On 10/03/16 16:01, 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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 94 insertions(+), 3 deletions(-)
> Just to educate myself (really, you can ignore my question as "review
> comment", because it's not one): why is this necessary? In Windows 8 and
> Windows Server 2012 R2, simply putting your NTFS filesystem on a SCSI
> disk (such as virtio-scsi-pci / scsi-hd) or AHCI, and specifying
> discard=on for the -drive, will result in the guest automatically
> trimming the NTFS (with a little delay) after deleting files, and the
> host getting those blocks back.
The same as for Linux. But if the one exact block has been freed
by half at one operation and by another half in the different operation
as far as I could understand it will not be freed.
This patch implements the ability to trim all the disc as done for Linux
to go over all the disc and discard all possible areas. I think that this
would be useful f.e. to prepare template images.
Den
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests
2016-10-05 19:47 ` Denis V. Lunev
@ 2016-10-05 19:56 ` Laszlo Ersek
0 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2016-10-05 19:56 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel
Cc: Denis Plotnikov, Marc-André Lureau, Michael Roth,
Stefan Weil
On 10/05/16 21:47, Denis V. Lunev wrote:
> On 10/05/2016 09:55 PM, Laszlo Ersek wrote:
>> On 10/03/16 16:01, 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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 94 insertions(+), 3 deletions(-)
>> Just to educate myself (really, you can ignore my question as "review
>> comment", because it's not one): why is this necessary? In Windows 8 and
>> Windows Server 2012 R2, simply putting your NTFS filesystem on a SCSI
>> disk (such as virtio-scsi-pci / scsi-hd) or AHCI, and specifying
>> discard=on for the -drive, will result in the guest automatically
>> trimming the NTFS (with a little delay) after deleting files, and the
>> host getting those blocks back.
> The same as for Linux. But if the one exact block has been freed
> by half at one operation and by another half in the different operation
> as far as I could understand it will not be freed.
>
> This patch implements the ability to trim all the disc as done for Linux
> to go over all the disc and discard all possible areas. I think that this
> would be useful f.e. to prepare template images.
Thank you for explaining. And, now I've learned about "defrag /L" too :)
Cheers
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread