[PATCH 0/3] qga/win/vss: add VSS backup type options

[PATCH 0/3] qga/win/vss: add VSS backup type options

[Kfir Manor]

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/775

The problem, VSS backup type VSS-FULL (the only available VSS backup type currently) can break other backups that use VSS-FULL(for example, Bareos-Fullbackup).

Fix, add other backup types.

Implementation, put the desired backup type number inside Regkey value HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\QEMU Guest Agent VSS Provider\VssOption, so that the program can query the desired backup type.

VSS backup types:
number   type
1        VSS_BT_FULL
2        VSS_BT_INCREMENTAL
3        VSS_BT_DIFFERENTIAL
4        VSS_BT_LOG
5        VSS_BT_COPY

for more information about the different backup types https://learn.microsoft.com/en-us/windows/win32/vss/vss-backup-state

Additionally, the program would work as before with VSS-FULL in cases where VssOption doesn't exist, or VssOption value isn't a known backup type.

Kfir Manor (3):
  add VssOption to installer
  query VSS backup type
  requester_freeze changes

 qga/installer/qemu-ga.wxs   |  4 ++++
 qga/vss-win32/requester.cpp | 41 ++++++++++++++++++++++++++++++++++++-
 qga/vss-win32/vss-handles.h |  3 +++
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH 1/3] qga/win/installer: add VssOption to installer

[Kfir Manor]

Adds registry value VssOption with value 1 to QEMU Guest Agent VSS Provider service registry key

Signed-off-by: Kfir Manor <kfir@daynix.com>
---
 qga/installer/qemu-ga.wxs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 51340f7ecc..f7f145aa05 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -121,6 +121,10 @@
               <RegistryValue Type="integer" Name="TypesSupported" Value="7" />
               <RegistryValue Type="string" Name="EventMessageFile" Value="[qemu_ga_directory]qemu-ga.exe" />
             </RegistryKey>
+            <RegistryKey Root="HKLM"
+                         Key="System\CurrentControlSet\Services\QEMU Guest Agent VSS Provider">
+              <RegistryValue Type="integer" Name="VssOption" Value="1" />
+            </RegistryKey>
           </Component>
         </Directory>
       </Directory>
-- 
2.38.1

[PATCH 2/3] qga/win/vss: query VSS backup type

[Kfir Manor]

Query VSS backup type number (DWORD) from QEMU Guest Agent VSS Provider registry key registry value VssOption

Translate the VSS backup type number (DWORD) into its VSS backup type (VSS_BACKUP_TYPE)

Returns the queried VSS backup type if the program encounters unexpected behaviors or values return default VSS backup type VSS_BT_FULL instead

Signed-off-by: Kfir Manor <kfir@daynix.com>
---
 qga/vss-win32/requester.cpp | 38 +++++++++++++++++++++++++++++++++++++
 qga/vss-win32/vss-handles.h |  3 +++
 2 files changed, 41 insertions(+)

diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index b371affeab..27fdb9236b 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -23,6 +23,8 @@
 /* Call QueryStatus every 10 ms while waiting for frozen event */
 #define VSS_TIMEOUT_EVENT_MSEC 10
 
+#define DEFAULT_VSS_BACKUP_TYPE VSS_BT_FULL
+
 #define err_set(e, err, fmt, ...)                                           \
     ((e)->error_setg_win32_wrapper((e)->errp, __FILE__, __LINE__, __func__, \
                                    err, fmt, ## __VA_ARGS__))
@@ -234,6 +236,42 @@ out:
     }
 }
 
+DWORD get_reg_dword_value(HKEY baseKey, LPCSTR subKey, LPCSTR valueName,
+                          DWORD defaultData)
+{
+    DWORD regGetValueError;
+    DWORD dwordData;
+    DWORD dataSize = sizeof(DWORD);
+
+    regGetValueError = RegGetValue(baseKey, subKey, valueName, RRF_RT_DWORD,
+                                   NULL, &dwordData, &dataSize);
+    if (regGetValueError  != ERROR_SUCCESS) {
+        return defaultData;
+    }
+    return dwordData;
+}
+
+bool is_valid_vss_backup_type(VSS_BACKUP_TYPE vssBT)
+{
+    return (vssBT > VSS_BT_UNDEFINED && vssBT < VSS_BT_OTHER);
+}
+
+VSS_BACKUP_TYPE get_vss_backup_type(
+    VSS_BACKUP_TYPE defaultVssBT = DEFAULT_VSS_BACKUP_TYPE)
+{
+    VSS_BACKUP_TYPE vssBackupType;
+
+    vssBackupType = static_cast<VSS_BACKUP_TYPE>(
+                            get_reg_dword_value(HKEY_LOCAL_MACHINE,
+                                                QGA_PROVIDER_REGISTRY_ADDRESS,
+                                                "VssOption",
+                                                defaultVssBT));
+    if (!is_valid_vss_backup_type(vssBackupType)) {
+        return defaultVssBT;
+    }
+    return vssBackupType;
+}
+
 void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
 {
     COMPointer<IVssAsync> pAsync;
diff --git a/qga/vss-win32/vss-handles.h b/qga/vss-win32/vss-handles.h
index 0f8a741ad2..1a7d842129 100644
--- a/qga/vss-win32/vss-handles.h
+++ b/qga/vss-win32/vss-handles.h
@@ -6,6 +6,9 @@
 #define QGA_PROVIDER_NAME "QEMU Guest Agent VSS Provider"
 #define QGA_PROVIDER_LNAME L(QGA_PROVIDER_NAME)
 #define QGA_PROVIDER_VERSION L(QEMU_VERSION)
+#define QGA_PROVIDER_REGISTRY_ADDRESS "SYSTEM\\CurrentControlSet"\
+                                      "\\Services"\
+                                      "\\" QGA_PROVIDER_NAME
 
 #define EVENT_NAME_FROZEN  "Global\\QGAVSSEvent-frozen"
 #define EVENT_NAME_THAW    "Global\\QGAVSSEvent-thaw"
-- 
2.38.1

[PATCH 3/3] qga/win/vss: requester_freeze changes

[Kfir Manor]

Change requester_freeze so that the VSS backup type queried from the registry

Signed-off-by: Kfir Manor <kfir@daynix.com>
---
 qga/vss-win32/requester.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 27fdb9236b..163b3870b9 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -285,6 +285,7 @@ void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
     DWORD wait_status;
     int num_fixed_drives = 0, i;
     int num_mount_points = 0;
+    VSS_BACKUP_TYPE vss_bt = get_vss_backup_type();
 
     if (vss_ctx.pVssbc) { /* already frozen */
         *num_vols = 0;
@@ -332,7 +333,7 @@ void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
         goto out;
     }
 
-    hr = vss_ctx.pVssbc->SetBackupState(true, true, VSS_BT_FULL, false);
+    hr = vss_ctx.pVssbc->SetBackupState(true, true, vss_bt, false);
     if (FAILED(hr)) {
         err_set(errset, hr, "failed to set backup state");
         goto out;
-- 
2.38.1

Re: [PATCH 0/3] qga/win/vss: add VSS backup type options

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

Hi Marc-André,

Can you please review this patch set?

Best Regards,
Konstantin Kostiuk.


On Thu, Feb 9, 2023 at 10:50 AM Kfir Manor <kfir@daynix.com> wrote:

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/775
>
> The problem, VSS backup type VSS-FULL (the only available VSS backup type
> currently) can break other backups that use VSS-FULL(for example,
> Bareos-Fullbackup).
>
> Fix, add other backup types.
>
> Implementation, put the desired backup type number inside Regkey value
> HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\QEMU Guest Agent VSS
> Provider\VssOption, so that the program can query the desired backup type.
>
> VSS backup types:
> number   type
> 1        VSS_BT_FULL
> 2        VSS_BT_INCREMENTAL
> 3        VSS_BT_DIFFERENTIAL
> 4        VSS_BT_LOG
> 5        VSS_BT_COPY
>
> for more information about the different backup types
> https://learn.microsoft.com/en-us/windows/win32/vss/vss-backup-state
>
> Additionally, the program would work as before with VSS-FULL in cases
> where VssOption doesn't exist, or VssOption value isn't a known backup type.
>
> Kfir Manor (3):
>   add VssOption to installer
>   query VSS backup type
>   requester_freeze changes
>
>  qga/installer/qemu-ga.wxs   |  4 ++++
>  qga/vss-win32/requester.cpp | 41 ++++++++++++++++++++++++++++++++++++-
>  qga/vss-win32/vss-handles.h |  3 +++
>  3 files changed, 47 insertions(+), 1 deletion(-)
>
> --
> 2.38.1
>
>

[-- Attachment #2: Type: text/html, Size: 2258 bytes --]

Re: [PATCH 0/3] qga/win/vss: add VSS backup type options

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

Hi

On Mon, Feb 13, 2023 at 8:20 PM Konstantin Kostiuk <kkostiuk@redhat.com>
wrote:

> Hi Marc-André,
>
> Can you please review this patch set?
>
> Best Regards,
> Konstantin Kostiuk.
>
>
> On Thu, Feb 9, 2023 at 10:50 AM Kfir Manor <kfir@daynix.com> wrote:
>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/775
>>
>> The problem, VSS backup type VSS-FULL (the only available VSS backup type
>> currently) can break other backups that use VSS-FULL(for example,
>> Bareos-Fullbackup).
>>
>> Fix, add other backup types.
>>
>> Implementation, put the desired backup type number inside Regkey value
>> HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\QEMU Guest Agent VSS
>> Provider\VssOption, so that the program can query the desired backup type.
>>
>> VSS backup types:
>> number   type
>> 1        VSS_BT_FULL
>> 2        VSS_BT_INCREMENTAL
>> 3        VSS_BT_DIFFERENTIAL
>> 4        VSS_BT_LOG
>> 5        VSS_BT_COPY
>>
>> for more information about the different backup types
>> https://learn.microsoft.com/en-us/windows/win32/vss/vss-backup-state
>>
>> Additionally, the program would work as before with VSS-FULL in cases
>> where VssOption doesn't exist, or VssOption value isn't a known backup type.
>>
>
The patch series looks ok (just minor stylistic changes could be made), but
I do not fully understand the way qga-vss.dll works in details for
freeze/thaw.

My understanding is that FIFREEZE do not exist on win32, so we call VSS to
tell (some) apps to flush/freeze pretending a backup is going on, then we
get notified on completion by our own provider (CommitSnapshots) and wait
there for thaw (1 min while VM is suspended?).

But I don't understand how this interacts with other providers (real backup
solutions), and why they are involved/conflict as described in
https://gitlab.com/qemu-project/qemu/-/issues/775.




>
>> Kfir Manor (3):
>>   add VssOption to installer
>>   query VSS backup type
>>   requester_freeze changes
>>
>>  qga/installer/qemu-ga.wxs   |  4 ++++
>>  qga/vss-win32/requester.cpp | 41 ++++++++++++++++++++++++++++++++++++-
>>  qga/vss-win32/vss-handles.h |  3 +++
>>  3 files changed, 47 insertions(+), 1 deletion(-)
>>
>> --
>> 2.38.1
>>
>>

[-- Attachment #2: Type: text/html, Size: 3716 bytes --]

Re: [PATCH 0/3] qga/win/vss: add VSS backup type options

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 2824 bytes --]

On Tue, Feb 14, 2023 at 3:20 AM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> Hi
>
> On Mon, Feb 13, 2023 at 8:20 PM Konstantin Kostiuk <kkostiuk@redhat.com>
> wrote:
>
>> Hi Marc-André,
>>
>> Can you please review this patch set?
>>
>> Best Regards,
>> Konstantin Kostiuk.
>>
>>
>> On Thu, Feb 9, 2023 at 10:50 AM Kfir Manor <kfir@daynix.com> wrote:
>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/775
>>>
>>> The problem, VSS backup type VSS-FULL (the only available VSS backup
>>> type currently) can break other backups that use VSS-FULL(for example,
>>> Bareos-Fullbackup).
>>>
>>> Fix, add other backup types.
>>>
>>> Implementation, put the desired backup type number inside Regkey value
>>> HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\QEMU Guest Agent VSS
>>> Provider\VssOption, so that the program can query the desired backup type.
>>>
>>> VSS backup types:
>>> number   type
>>> 1        VSS_BT_FULL
>>> 2        VSS_BT_INCREMENTAL
>>> 3        VSS_BT_DIFFERENTIAL
>>> 4        VSS_BT_LOG
>>> 5        VSS_BT_COPY
>>>
>>> for more information about the different backup types
>>> https://learn.microsoft.com/en-us/windows/win32/vss/vss-backup-state
>>>
>>> Additionally, the program would work as before with VSS-FULL in cases
>>> where VssOption doesn't exist, or VssOption value isn't a known backup type.
>>>
>>
> The patch series looks ok (just minor stylistic changes could be made),
> but I do not fully understand the way qga-vss.dll works in details for
> freeze/thaw.
>
> My understanding is that FIFREEZE do not exist on win32, so we call VSS to
> tell (some) apps to flush/freeze pretending a backup is going on, then we
> get notified on completion by our own provider (CommitSnapshots) and wait
> there for thaw (1 min while VM is suspended?).
>
> But I don't understand how this interacts with other providers (real
> backup solutions), and why they are involved/conflict as described in
> https://gitlab.com/qemu-project/qemu/-/issues/775.
>

This is a very good question. To understand the reason for the conflict we
should dig deeper into VSS.
For some reason, MS provides different VSS backup types. Currently, we
don't use other types but we got
a request from the community. I think this is ok, to add more freedom of
configuration if someone wants it.
Anyway, the default behavior is not changed.


>
>
>>
>>> Kfir Manor (3):
>>>   add VssOption to installer
>>>   query VSS backup type
>>>   requester_freeze changes
>>>
>>>  qga/installer/qemu-ga.wxs   |  4 ++++
>>>  qga/vss-win32/requester.cpp | 41 ++++++++++++++++++++++++++++++++++++-
>>>  qga/vss-win32/vss-handles.h |  3 +++
>>>  3 files changed, 47 insertions(+), 1 deletion(-)
>>>
>>> --
>>> 2.38.1
>>>
>>>

[-- Attachment #2: Type: text/html, Size: 4754 bytes --]

Re: [PATCH 1/3] qga/win/installer: add VssOption to installer

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Thu, Feb 9, 2023 at 2:50 AM Kfir Manor <kfir@daynix.com> wrote:

> Adds registry value VssOption with value 1 to QEMU Guest Agent VSS
> Provider service registry key
>
> Signed-off-by: Kfir Manor <kfir@daynix.com>
> ---
>  qga/installer/qemu-ga.wxs | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
> index 51340f7ecc..f7f145aa05 100644
> --- a/qga/installer/qemu-ga.wxs
> +++ b/qga/installer/qemu-ga.wxs
> @@ -121,6 +121,10 @@
>                <RegistryValue Type="integer" Name="TypesSupported"
> Value="7" />
>                <RegistryValue Type="string" Name="EventMessageFile"
> Value="[qemu_ga_directory]qemu-ga.exe" />
>              </RegistryKey>
> +            <RegistryKey Root="HKLM"
> +                         Key="System\CurrentControlSet\Services\QEMU
> Guest Agent VSS Provider">
> +              <RegistryValue Type="integer" Name="VssOption" Value="1" />
> +            </RegistryKey>
>            </Component>
>          </Directory>
>        </Directory>
> --
> 2.38.1
>
>

[-- Attachment #2: Type: text/html, Size: 1819 bytes --]

Re: [PATCH 2/3] qga/win/vss: query VSS backup type

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Thu, Feb 9, 2023 at 2:50 AM Kfir Manor <kfir@daynix.com> wrote:

> Query VSS backup type number (DWORD) from QEMU Guest Agent VSS Provider
> registry key registry value VssOption
>
> Translate the VSS backup type number (DWORD) into its VSS backup type
> (VSS_BACKUP_TYPE)
>
> Returns the queried VSS backup type if the program encounters unexpected
> behaviors or values return default VSS backup type VSS_BT_FULL instead
>
> Signed-off-by: Kfir Manor <kfir@daynix.com>
> ---
>  qga/vss-win32/requester.cpp | 38 +++++++++++++++++++++++++++++++++++++
>  qga/vss-win32/vss-handles.h |  3 +++
>  2 files changed, 41 insertions(+)
>
> diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
> index b371affeab..27fdb9236b 100644
> --- a/qga/vss-win32/requester.cpp
> +++ b/qga/vss-win32/requester.cpp
> @@ -23,6 +23,8 @@
>  /* Call QueryStatus every 10 ms while waiting for frozen event */
>  #define VSS_TIMEOUT_EVENT_MSEC 10
>
> +#define DEFAULT_VSS_BACKUP_TYPE VSS_BT_FULL
> +
>  #define err_set(e, err, fmt, ...)
>    \
>      ((e)->error_setg_win32_wrapper((e)->errp, __FILE__, __LINE__,
> __func__, \
>                                     err, fmt, ## __VA_ARGS__))
> @@ -234,6 +236,42 @@ out:
>      }
>  }
>
> +DWORD get_reg_dword_value(HKEY baseKey, LPCSTR subKey, LPCSTR valueName,
> +                          DWORD defaultData)
> +{
> +    DWORD regGetValueError;
> +    DWORD dwordData;
> +    DWORD dataSize = sizeof(DWORD);
> +
> +    regGetValueError = RegGetValue(baseKey, subKey, valueName,
> RRF_RT_DWORD,
> +                                   NULL, &dwordData, &dataSize);
> +    if (regGetValueError  != ERROR_SUCCESS) {
> +        return defaultData;
> +    }
> +    return dwordData;
> +}
> +
> +bool is_valid_vss_backup_type(VSS_BACKUP_TYPE vssBT)
> +{
> +    return (vssBT > VSS_BT_UNDEFINED && vssBT < VSS_BT_OTHER);
> +}
> +
> +VSS_BACKUP_TYPE get_vss_backup_type(
> +    VSS_BACKUP_TYPE defaultVssBT = DEFAULT_VSS_BACKUP_TYPE)
> +{
> +    VSS_BACKUP_TYPE vssBackupType;
> +
> +    vssBackupType = static_cast<VSS_BACKUP_TYPE>(
> +                            get_reg_dword_value(HKEY_LOCAL_MACHINE,
> +
> QGA_PROVIDER_REGISTRY_ADDRESS,
> +                                                "VssOption",
> +                                                defaultVssBT));
> +    if (!is_valid_vss_backup_type(vssBackupType)) {
> +        return defaultVssBT;
> +    }
> +    return vssBackupType;
> +}
> +
>  void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
>  {
>      COMPointer<IVssAsync> pAsync;
> diff --git a/qga/vss-win32/vss-handles.h b/qga/vss-win32/vss-handles.h
> index 0f8a741ad2..1a7d842129 100644
> --- a/qga/vss-win32/vss-handles.h
> +++ b/qga/vss-win32/vss-handles.h
> @@ -6,6 +6,9 @@
>  #define QGA_PROVIDER_NAME "QEMU Guest Agent VSS Provider"
>  #define QGA_PROVIDER_LNAME L(QGA_PROVIDER_NAME)
>  #define QGA_PROVIDER_VERSION L(QEMU_VERSION)
> +#define QGA_PROVIDER_REGISTRY_ADDRESS "SYSTEM\\CurrentControlSet"\
> +                                      "\\Services"\
> +                                      "\\" QGA_PROVIDER_NAME
>
>  #define EVENT_NAME_FROZEN  "Global\\QGAVSSEvent-frozen"
>  #define EVENT_NAME_THAW    "Global\\QGAVSSEvent-thaw"
> --
> 2.38.1
>
>

[-- Attachment #2: Type: text/html, Size: 4403 bytes --]

Re: [PATCH 3/3] qga/win/vss: requester_freeze changes

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Thu, Feb 9, 2023 at 2:50 AM Kfir Manor <kfir@daynix.com> wrote:

> Change requester_freeze so that the VSS backup type queried from the
> registry
>
> Signed-off-by: Kfir Manor <kfir@daynix.com>
> ---
>  qga/vss-win32/requester.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
> index 27fdb9236b..163b3870b9 100644
> --- a/qga/vss-win32/requester.cpp
> +++ b/qga/vss-win32/requester.cpp
> @@ -285,6 +285,7 @@ void requester_freeze(int *num_vols, void
> *mountpoints, ErrorSet *errset)
>      DWORD wait_status;
>      int num_fixed_drives = 0, i;
>      int num_mount_points = 0;
> +    VSS_BACKUP_TYPE vss_bt = get_vss_backup_type();
>
>      if (vss_ctx.pVssbc) { /* already frozen */
>          *num_vols = 0;
> @@ -332,7 +333,7 @@ void requester_freeze(int *num_vols, void
> *mountpoints, ErrorSet *errset)
>          goto out;
>      }
>
> -    hr = vss_ctx.pVssbc->SetBackupState(true, true, VSS_BT_FULL, false);
> +    hr = vss_ctx.pVssbc->SetBackupState(true, true, vss_bt, false);
>      if (FAILED(hr)) {
>          err_set(errset, hr, "failed to set backup state");
>          goto out;
> --
> 2.38.1
>
>

[-- Attachment #2: Type: text/html, Size: 1812 bytes --]