At 2018-08-24 04:22:08, "Michael Roth" <mdr...@linux.vnet.ibm.com> wrote: >Quoting Chen Hanxiao (2018-08-09 20:13:48) >> From: Chen Hanxiao <chenhanx...@gmail.com> >> >> This patch add support for freeze specified fs. >> >> The valid mountpoints list member are [1]: >> >> The path of a mounted folder, for example, Y:\MountX\ >> A drive letter, for example, D:\ >> A volume GUID path of the form \\?\Volume{GUID}\, >> where GUID identifies the volume >> A UNC path that specifies a remote file share, >> for example, \\Clusterx\Share1\ >> >> [1] >> https://docs.microsoft.com/en-us/windows/desktop/api/vsbackup/nf-vsbackup-ivssbackupcomponents-addtosnapshotset >> >> Cc: Michael Roth <mdr...@linux.vnet.ibm.com> >> Signed-off-by: Chen Hanxiao <chenhanx...@gmail.com> > >Some small nits below, but looks good otherwise. > >> --- >> qga/commands-win32.c | 21 ++++++-------- >> qga/main.c | 2 +- [...] >> diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp >> index 3d9c9716c0..0bd170eddc 100644 >> --- a/qga/vss-win32/requester.cpp >> +++ b/qga/vss-win32/requester.cpp >> @@ -234,7 +234,7 @@ out: >> } >> } >> >> -void requester_freeze(int *num_vols, ErrorSet *errset) >> +void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset) > >Does this need to be declared as void *, or can we use the direct VolList* >type? Maybe even const VolList * const, since it is read-only? That may change the current structure a lot. The caller qga_vss_fsfreeze also serve requester_thaw. We'd better keep its declaration as it used to be. > >> { >> COMPointer<IVssAsync> pAsync; >> HANDLE volume; >> @@ -245,7 +245,9 @@ void requester_freeze(int *num_vols, ErrorSet *errset) >> SECURITY_ATTRIBUTES sa; >> WCHAR short_volume_name[64], *display_name = short_volume_name; >> DWORD wait_status; >> + PWCHAR WStr = NULL; >> int num_fixed_drives = 0, i; >> + int num_mount_points = 0; >> >> if (vss_ctx.pVssbc) { /* already frozen */ >> *num_vols = 0; >> @@ -337,12 +339,42 @@ void requester_freeze(int *num_vols, ErrorSet *errset) >> goto out; >> } >> >> - volume = FindFirstVolumeW(short_volume_name, sizeof(short_volume_name)); >> - if (volume == INVALID_HANDLE_VALUE) { >> - err_set(errset, hr, "failed to find first volume"); >> + if (mountpoints) { >> + for (volList *list = (volList *)mountpoints; list; list = >> list->next) { >> + size_t len = strlen(list->value) + 1; >> + size_t converted = 0; >> + VSS_ID pid; >> + >> + WStr = new wchar_t[len]; >> + mbstowcs_s(&converted, WStr, len, list->value, _TRUNCATE); >> + >> + hr = vss_ctx.pVssbc->AddToSnapshotSet(WStr, >> + g_gProviderId, &pid); >> + if (FAILED(hr)) { >> + err_set(errset, hr, "failed to add %S to snapshot set", >> WStr); >> + goto out; > >WStr is only needed within this block, so I'd prefer declaring it here >and then just adding an additional "delete WStr;" before the "goto out;". > >Also I know the APIs force us to differ from the QEMU coding guidelines >with regard to CamelCase and whatnot, but I'd still prefer we stick to >it when possible. WStr also isn't very descriptive, maybe >volume_name_wchar? Sure, will be fixed in v2. > >> + } >> + num_mount_points++; >> + >> + delete WStr; >> + } >> + } >> + >> + if (mountpoints && num_mount_points == 0) { >> + /* If there is no valid mount points, just exit. */ >> goto out; >> } > >Logic might be a little cleaner if you move this block after the for >loop above. Done. > >> - for (;;) { >> + >> + >> + if (!mountpoints) { >> + volume = FindFirstVolumeW(short_volume_name, >> sizeof(short_volume_name)); >> + if (volume == INVALID_HANDLE_VALUE) { >> + err_set(errset, hr, "failed to find first volume"); >> + goto out; >> + } >> + } >> + >> + while (!mountpoints) { > >mountpoints is read-only in this function, so this is basically an >unconditional loop. I'd rather we do for(;;) like it was previously >and nest that in the "if (!mountpoints)" above to keep the logic >more contained. More clear. > >> if (GetDriveTypeW(short_volume_name) == DRIVE_FIXED) { >> VSS_ID pid; >> hr = vss_ctx.pVssbc->AddToSnapshotSet(short_volume_name, >> @@ -355,7 +387,7 @@ void requester_freeze(int *num_vols, ErrorSet *errset) >> display_name = volume_path_name; >> } >> err_set(errset, hr, "failed to add %S to snapshot set", >> - display_name); >> + display_name); >> FindVolumeClose(volume); >> goto out; >> } >> @@ -368,7 +400,7 @@ void requester_freeze(int *num_vols, ErrorSet *errset) >> } >> } >> >> - if (num_fixed_drives == 0) { >> + if (!mountpoints && num_fixed_drives == 0) { >> goto out; /* If there is no fixed drive, just exit. */ >> } > >and also nest this the same "if (!mountpoints)" block above. > Sure, I'll post v2 soon. Regards, - Chen