From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzBXj-0007dP-3W for qemu-devel@nongnu.org; Tue, 25 Oct 2016 19:54:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzBXf-0004oY-4p for qemu-devel@nongnu.org; Tue, 25 Oct 2016 19:54:07 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:60664) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bzBXe-0004oL-Se for qemu-devel@nongnu.org; Tue, 25 Oct 2016 19:54:03 -0400 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u9PNrwJP101295 for ; Tue, 25 Oct 2016 19:54:02 -0400 Received: from e33.co.us.ibm.com (e33.co.us.ibm.com [32.97.110.151]) by mx0a-001b2d01.pphosted.com with ESMTP id 26aexjpmty-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 25 Oct 2016 19:54:01 -0400 Received: from localhost by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 25 Oct 2016 17:54:01 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: References: <1475503285-9021-1-git-send-email-den@openvz.org> Date: Tue, 25 Oct 2016 18:53:51 -0500 Message-Id: <20161025235351.10426.90549@loki> Subject: Re: [Qemu-devel] [PATCH v4 1/1] qga: minimal support for fstrim for Windows guests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , =?utf-8?q?Marc-Andr=C3=A9_Lureau?= , qemu-devel@nongnu.org Cc: Denis Plotnikov , Stefan Weil Quoting Denis V. Lunev (2016-10-05 06:13:12) > On 10/04/2016 04:43 PM, Marc-Andr=C3=A9 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=C3=A9 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] =3D L""; > > + > > + handle =3D FindFirstVolumeW(guid, ARRAYSIZE(guid)); > > + if (handle =3D=3D INVALID_HANDLE_VALUE) { > > + error_setg_win32(errp, GetLastError(), "failed to find > > any volume"); > > + return NULL; > > + } > > + > > + resp =3D g_new0(GuestFilesystemTrimResponse, 1); > > + > > + do { > > + GuestFilesystemTrimResult *res; > > + GuestFilesystemTrimResultList *list; > > + PWCHAR uc_path; > > + DWORD char_count =3D 0; > > + char *path, *out; > > + GError *gerr =3D NULL; > > + gchar * argv[4]; > > + > > + GetVolumePathNamesForVolumeNameW(guid, NULL, 0, &char_coun= t); > > + > > > > > > It assumes GetVolumePathNamesForVolumeNameW() =3D=3D 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() !=3D ERROR_MORE_DATA) { > > > > > > Would it be useful to log the error in this case? > > = > > > > + continue; > > + } > > + if (GetDriveTypeW(guid) !=3D DRIVE_FIXED) { > > + continue; > > + } > > + > > + uc_path =3D 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 =3D=3D 2 */ > > > > > > What size? > > = > with char_count =3D=3D 2 > = > > Same remark regarding logging error. > > > > + g_free(uc_path); > > + continue; > > + } > > + > > + res =3D g_new0(GuestFilesystemTrimResult, 1); > > + > > + path =3D g_utf16_to_utf8(uc_path, char_count, NULL, NULL, > > &gerr); > > + > > + g_free(uc_path); > > + > > + if (gerr !=3D NULL && gerr->code) { > > > > > > Why check gerr->code? To be consistent with error checking code, I > > would check if path =3D=3D NULL instead, which by glib doc says that ge= rr > > 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 =3D true; > > + res->error =3D g_strdup(gerr->message); > > + g_error_free(gerr); > > + break; > > + } > > + > > + res->path =3D path; > > + > > + list =3D g_new0(GuestFilesystemTrimResultList, 1); > > + list->value =3D res; > > + list->next =3D resp->paths; > > + > > + resp->paths =3D list; > > + > > + memset(argv, 0, sizeof(argv)); > > + argv[0] =3D (gchar *)"defrag.exe"; > > + argv[1] =3D (gchar *)"/L"; > > + argv[2] =3D path; > > + > > + if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, > > NULL, NULL, > > + &out /* stdout */, NULL /* stdin */, > > + NULL, &gerr)) { > > + res->has_error =3D true; > > + res->error =3D 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 =3D g_strsplit(out, "\r\n", 0); > > + g_free(out); > > + > > + for (i =3D 0; lines[i] !=3D NULL; i++) { > > + if (g_strstr_len(lines[i], -1, "(0x") =3D=3D NULL)= { > > + continue; > > + } > > + res->has_error =3D true; > > + res->error =3D 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 =3D (char **)list_unsupported; > > > > while (*p) { > > -- > > 2.7.4 > > > > -- = > > Marc-Andr=C3=A9 Lureau > = >=20