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

[Michael Roth]

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  > > 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 >
> > Signed-off-by: Denis Plotnikov  > >
> > CC: Michael Roth  > >
> > CC: Stefan Weil >
> > CC: Marc-André Lureau  > >
> >
> >
> > 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, _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,
> > +  _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,
> > );
> > +
> > +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));
> > +

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

[Denis V. Lunev]

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 
>> Signed-off-by: Denis Plotnikov 
>> CC: Michael Roth 
>> CC: Stefan Weil 
>> CC: Marc-André Lureau 
>> ---
>>  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

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

[Laszlo Ersek]

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 
>>> Signed-off-by: Denis Plotnikov 
>>> CC: Michael Roth 
>>> CC: Stefan Weil 
>>> CC: Marc-André Lureau 
>>> ---
>>>  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

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

[Laszlo Ersek]

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 
> Signed-off-by: Denis Plotnikov 
> CC: Michael Roth 
> CC: Stefan Weil 
> CC: Marc-André Lureau 
> ---
>  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, _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,
> +  _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, );
> +
> +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,
> +   /* stdout */, NULL /* stdin */,
> +  NULL, )) {
> +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
> +   (x8920) 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)
>  

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 
> Signed-off-by: Denis Plotnikov 
> CC: Michael Roth 
> CC: Stefan Weil 
> CC: Marc-André Lureau 
> ---
>  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, _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,
> +  _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, );
> +
> +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,
> +   /* stdout */, NULL /* stdin */,
> +  NULL, )) {
> +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
> +   (x8920) 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 

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  > 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 >
> Signed-off-by: Denis Plotnikov  >
> CC: Michael Roth  >
> CC: Stefan Weil >
> CC: Marc-André Lureau  >
>
>
> 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, _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,
> +  _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,
> );
> +
> +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,
> +   /* stdout */, NULL /* stdin */,
> +  NULL, )) {
> +res->has_error = true;
> +res->error = g_strdup(gerr->message);
> +g_error_free(gerr);
>
>
> It 

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

[Marc-André Lureau]

Hi

On Mon, Oct 3, 2016 at 6:01 PM 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 
> Signed-off-by: Denis Plotnikov 
> CC: Michael Roth 
> CC: Stefan Weil 
> CC: Marc-André Lureau 
>

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, _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,
> +  _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, );
> +
> +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,
> +   /* stdout */, NULL /* stdin */,
> +  NULL, )) {
> +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
> +   (x8920) 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;
>  }
>
>  

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

[Denis V. Lunev]

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 
Signed-off-by: Denis Plotnikov 
CC: Michael Roth 
CC: Stefan Weil 
CC: Marc-André Lureau 
---
 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, _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,
+  _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, );
+
+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,
+   /* stdout */, NULL /* stdin */,
+  NULL, )) {
+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
+   (x8920) 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