[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [resend][PATCH] qga-win: add support for qmp_guest_fsfr
From: |
Chen Hanxiao |
Subject: |
Re: [Qemu-devel] [resend][PATCH] qga-win: add support for qmp_guest_fsfreeze_freeze_list |
Date: |
Fri, 31 Aug 2018 11:08:35 +0800 (CST) |
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