On 10/04/2016 04:43 PM, Marc-André Lureau wrote: > Hi > > On Mon, Oct 3, 2016 at 6:01 PM Denis V. Lunev <d...@openvz.org > <mailto:d...@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 <d...@openvz.org <mailto:d...@openvz.org>> > Signed-off-by: Denis Plotnikov <dplotni...@virtuozzo.com > <mailto:dplotni...@virtuozzo.com>> > CC: Michael Roth <mdr...@linux.vnet.ibm.com > <mailto:mdr...@linux.vnet.ibm.com>> > CC: Stefan Weil <s...@weilnetz.de <mailto:s...@weilnetz.de>> > CC: Marc-André Lureau <marcandre.lur...@gmail.com > <mailto:marcandre.lur...@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