Re: [Qemu-devel] [resend][PATCH] qga-win: add support for qmp_guest_fsfreeze_freeze_list

[Chen Hanxiao]

At 2018-08-24 04:22:08, "Michael Roth" <address@hidden> wrote:
>Quoting Chen Hanxiao (2018-08-09 20:13:48)
>> From: Chen Hanxiao <address@hidden>
>> 
>> 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 <address@hidden>
>> Signed-off-by: Chen Hanxiao <address@hidden>
>
>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