[Qemu-devel] [PATCH 0/3] qga: vss-win32: Fix interference with other VSS requesters

[Qemu-devel] [PATCH 0/3] qga: vss-win32: Fix interference with other VSS requesters

[Tomoki Sekiyama]

Current functionarity of qemu-ga VSS provider is limited to implement
filesystems freeze, and doesn't support the creation of shadow copies
within the guest.
However, when no other hardware snapshot provider is installed, VSS may
choose qemu-ga VSS provider to create shadow copies and fail with
VSS_E_UNEXPECTED_PROVIDER_ERROR.
Similar issue occurs when the requester deletes shadow copies.

This patchset fix this issue by telling VSS that the volume is not
supported by qemu-ga VSS provider when it is kicked by other requesters.

It also fixes wrong error handling around OpenEvent/CreateEvent WinAPI,
which returns NULL instead of INVALID_HANDLE_VALUE on errors.

https://bugzilla.redhat.com/show_bug.cgi?id=1036341

---
Tomoki Sekiyama (3):
      qga: vss-win32: Use NULL as an invalid pointer for OpenEvent and CreateEvent
      qga: vss-win32: Fix interference with snapshot creation by other VSS requesters
      qga: vss-win32: Fix interference with snapshot deletion by other VSS request


 qga/vss-win32/provider.cpp  |   21 ++++++++++---
 qga/vss-win32/requester.cpp |   70 ++++++++++++++++++++-----------------------
 2 files changed, 49 insertions(+), 42 deletions(-)

-- 
Tomoki Sekiyama

[Qemu-devel] [PATCH 1/3] qga: vss-win32: Use NULL as an invalid pointer for OpenEvent and CreateEvent

[Tomoki Sekiyama]

OpenEvent and CreateEvent WinAPI return NULL when failed to open/create
events handles, instead of INVALID_HANDLE_VALUE (although their return
types are HANDLE).
This replaces INVALID_HANDLE_VALUE related to event handles with NULL.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
---
 qga/vss-win32/provider.cpp  |    6 +++---
 qga/vss-win32/requester.cpp |   24 ++++++++++--------------
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
index bf42b5e..c3030d8 100644
--- a/qga/vss-win32/provider.cpp
+++ b/qga/vss-win32/provider.cpp
@@ -342,18 +342,18 @@ STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId)
     HANDLE hEventFrozen, hEventThaw, hEventTimeout;
 
     hEventFrozen = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_FROZEN);
-    if (hEventFrozen == INVALID_HANDLE_VALUE) {
+    if (!hEventFrozen) {
         return E_FAIL;
     }
 
     hEventThaw = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_THAW);
-    if (hEventThaw == INVALID_HANDLE_VALUE) {
+    if (!hEventThaw) {
         CloseHandle(hEventFrozen);
         return E_FAIL;
     }
 
     hEventTimeout = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_TIMEOUT);
-    if (hEventTimeout == INVALID_HANDLE_VALUE) {
+    if (!hEventTimeout) {
         CloseHandle(hEventFrozen);
         CloseHandle(hEventThaw);
         return E_FAIL;
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 1e8dd3d..0a55447 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -50,10 +50,6 @@ static struct QGAVSSContext {
 
 STDAPI requester_init(void)
 {
-    vss_ctx.hEventFrozen =  INVALID_HANDLE_VALUE;
-    vss_ctx.hEventThaw = INVALID_HANDLE_VALUE;
-    vss_ctx.hEventTimeout = INVALID_HANDLE_VALUE;
-
     COMInitializer initializer; /* to call CoInitializeSecurity */
     HRESULT hr = CoInitializeSecurity(
         NULL, -1, NULL, NULL, RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
@@ -94,17 +90,17 @@ STDAPI requester_init(void)
 
 static void requester_cleanup(void)
 {
-    if (vss_ctx.hEventFrozen != INVALID_HANDLE_VALUE) {
+    if (vss_ctx.hEventFrozen) {
         CloseHandle(vss_ctx.hEventFrozen);
-        vss_ctx.hEventFrozen = INVALID_HANDLE_VALUE;
+        vss_ctx.hEventFrozen = NULL;
     }
-    if (vss_ctx.hEventThaw != INVALID_HANDLE_VALUE) {
+    if (vss_ctx.hEventThaw) {
         CloseHandle(vss_ctx.hEventThaw);
-        vss_ctx.hEventThaw = INVALID_HANDLE_VALUE;
+        vss_ctx.hEventThaw = NULL;
     }
-    if (vss_ctx.hEventTimeout != INVALID_HANDLE_VALUE) {
+    if (vss_ctx.hEventTimeout) {
         CloseHandle(vss_ctx.hEventTimeout);
-        vss_ctx.hEventTimeout = INVALID_HANDLE_VALUE;
+        vss_ctx.hEventTimeout = NULL;
     }
     if (vss_ctx.pAsyncSnapshot) {
         vss_ctx.pAsyncSnapshot->Release();
@@ -374,19 +370,19 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
     sa.bInheritHandle = FALSE;
 
     vss_ctx.hEventFrozen = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_FROZEN);
-    if (vss_ctx.hEventFrozen == INVALID_HANDLE_VALUE) {
+    if (!vss_ctx.hEventFrozen) {
         err_set(errset, GetLastError(), "failed to create event %s",
                 EVENT_NAME_FROZEN);
         goto out;
     }
     vss_ctx.hEventThaw = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_THAW);
-    if (vss_ctx.hEventThaw == INVALID_HANDLE_VALUE) {
+    if (!vss_ctx.hEventThaw) {
         err_set(errset, GetLastError(), "failed to create event %s",
                 EVENT_NAME_THAW);
         goto out;
     }
     vss_ctx.hEventTimeout = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_TIMEOUT);
-    if (vss_ctx.hEventTimeout == INVALID_HANDLE_VALUE) {
+    if (!vss_ctx.hEventTimeout) {
         err_set(errset, GetLastError(), "failed to create event %s",
                 EVENT_NAME_TIMEOUT);
         goto out;
@@ -443,7 +439,7 @@ void requester_thaw(int *num_vols, ErrorSet *errset)
 {
     COMPointer<IVssAsync> pAsync;
 
-    if (vss_ctx.hEventThaw == INVALID_HANDLE_VALUE) {
+    if (!vss_ctx.hEventThaw) {
         /*
          * In this case, DoSnapshotSet is aborted or not started,
          * and no volumes must be frozen. We return without an error.

[Qemu-devel] [PATCH 2/3] qga: vss-win32: Fix interference with snapshot creation by other VSS requesters

[Tomoki Sekiyama]

When a VSS requester such as vshadow.exe or diskshadow.exe requests to
create disk snapshots, Windows may choose qemu-ga VSS provider if it is
only provider registered on the system. However, because it provides only a
function to freeze the filesystem, the snapshotting fails.

This patch adds a check into CQGAVssProvider::IsVolumeSupported() to reject
the request from other VSS requesters, so that the other provider is chosen.

The check of requester is done by confirming event channels between
qemu-ga's requester and provider established. To ensure that the events are
initialized when CQGAVssProvider::IsVolumeSupported() is called, it moves
the initialization earlier.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
---
 qga/vss-win32/provider.cpp  |   11 ++++++++-
 qga/vss-win32/requester.cpp |   52 ++++++++++++++++++++++---------------------
 2 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
index c3030d8..b233646 100644
--- a/qga/vss-win32/provider.cpp
+++ b/qga/vss-win32/provider.cpp
@@ -291,8 +291,17 @@ STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot(
 STDMETHODIMP CQGAVssProvider::IsVolumeSupported(
     VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider)
 {
-    *pbSupportedByThisProvider = TRUE;
+    HANDLE hEventFrozen;
+
+    /* Check if a requester is qemu-ga by whether an event is created */
+    hEventFrozen = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_FROZEN);
+    if (!hEventFrozen) {
+        *pbSupportedByThisProvider = FALSE;
+        return S_OK;
+    }
+    CloseHandle(hEventFrozen);
 
+    *pbSupportedByThisProvider = TRUE;
     return S_OK;
 }
 
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 0a55447..922e74d 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -252,6 +252,32 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
 
     CoInitialize(NULL);
 
+    /* Allow unrestricted access to events */
+    InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION);
+    SetSecurityDescriptorDacl(&sd, TRUE, NULL, FALSE);
+    sa.nLength = sizeof(sa);
+    sa.lpSecurityDescriptor = &sd;
+    sa.bInheritHandle = FALSE;
+
+    vss_ctx.hEventFrozen = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_FROZEN);
+    if (!vss_ctx.hEventFrozen) {
+        err_set(errset, GetLastError(), "failed to create event %s",
+                EVENT_NAME_FROZEN);
+        goto out;
+    }
+    vss_ctx.hEventThaw = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_THAW);
+    if (!vss_ctx.hEventThaw) {
+        err_set(errset, GetLastError(), "failed to create event %s",
+                EVENT_NAME_THAW);
+        goto out;
+    }
+    vss_ctx.hEventTimeout = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_TIMEOUT);
+    if (!vss_ctx.hEventTimeout) {
+        err_set(errset, GetLastError(), "failed to create event %s",
+                EVENT_NAME_TIMEOUT);
+        goto out;
+    }
+
     assert(pCreateVssBackupComponents != NULL);
     hr = pCreateVssBackupComponents(&vss_ctx.pVssbc);
     if (FAILED(hr)) {
@@ -362,32 +388,6 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
         goto out;
     }
 
-    /* Allow unrestricted access to events */
-    InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION);
-    SetSecurityDescriptorDacl(&sd, TRUE, NULL, FALSE);
-    sa.nLength = sizeof(sa);
-    sa.lpSecurityDescriptor = &sd;
-    sa.bInheritHandle = FALSE;
-
-    vss_ctx.hEventFrozen = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_FROZEN);
-    if (!vss_ctx.hEventFrozen) {
-        err_set(errset, GetLastError(), "failed to create event %s",
-                EVENT_NAME_FROZEN);
-        goto out;
-    }
-    vss_ctx.hEventThaw = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_THAW);
-    if (!vss_ctx.hEventThaw) {
-        err_set(errset, GetLastError(), "failed to create event %s",
-                EVENT_NAME_THAW);
-        goto out;
-    }
-    vss_ctx.hEventTimeout = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_TIMEOUT);
-    if (!vss_ctx.hEventTimeout) {
-        err_set(errset, GetLastError(), "failed to create event %s",
-                EVENT_NAME_TIMEOUT);
-        goto out;
-    }
-
     /*
      * Start VSS quiescing operations.
      * CQGAVssProvider::CommitSnapshots will kick vss_ctx.hEventFrozen

[Qemu-devel] [PATCH 3/3] qga: vss-win32: Fix interference with snapshot deletion by other VSS request

[Tomoki Sekiyama]

When a VSS requester such as vshadow.exe or diskshadow.exe requests to
delete snapshots, qemu-ga VSS provider's DeleteSnapshots() is also called
and returns E_NOTIMPL, that makes the deletion fail.
To avoid this issue, return S_OK and set values that represent no snapshots
are deleted by qemu-ga VSS provider.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
---
 qga/vss-win32/provider.cpp |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
index b233646..d5129f8 100644
--- a/qga/vss-win32/provider.cpp
+++ b/qga/vss-win32/provider.cpp
@@ -278,7 +278,9 @@ STDMETHODIMP CQGAVssProvider::DeleteSnapshots(
     VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
     BOOL bForceDelete, LONG *plDeletedSnapshots, VSS_ID *pNondeletedSnapshotID)
 {
-    return E_NOTIMPL;
+    *plDeletedSnapshots = 0;
+    *pNondeletedSnapshotID = SourceObjectId;
+    return S_OK;
 }
 
 STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot(

Re: [Qemu-devel] [PATCH 1/3] qga: vss-win32: Use NULL as an invalid pointer for OpenEvent and CreateEvent

[Gal Hammer]

On 13/01/2014 19:25, Tomoki Sekiyama wrote:
> OpenEvent and CreateEvent WinAPI return NULL when failed to open/create
> events handles, instead of INVALID_HANDLE_VALUE (although their return
> types are HANDLE).
> This replaces INVALID_HANDLE_VALUE related to event handles with NULL.
>
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>

Reviewed-by: Gal Hammer <ghammer@redhat.com>

     Gal.

Re: [Qemu-devel] [PATCH 2/3] qga: vss-win32: Fix interference with snapshot creation by other VSS requesters

[Gal Hammer]

On 13/01/2014 19:25, Tomoki Sekiyama wrote:
> When a VSS requester such as vshadow.exe or diskshadow.exe requests to
> create disk snapshots, Windows may choose qemu-ga VSS provider if it is
> only provider registered on the system. However, because it provides only a
> function to freeze the filesystem, the snapshotting fails.
>
> This patch adds a check into CQGAVssProvider::IsVolumeSupported() to reject
> the request from other VSS requesters, so that the other provider is chosen.
>
> The check of requester is done by confirming event channels between
> qemu-ga's requester and provider established. To ensure that the events are
> initialized when CQGAVssProvider::IsVolumeSupported() is called, it moves
> the initialization earlier.
>
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>

Reviewed-by: Gal Hammer <ghammer@redhat.com>

     Gal.

Re: [Qemu-devel] [PATCH 3/3] qga: vss-win32: Fix interference with snapshot deletion by other VSS request

[Gal Hammer]

On 13/01/2014 19:25, Tomoki Sekiyama wrote:
> When a VSS requester such as vshadow.exe or diskshadow.exe requests to
> delete snapshots, qemu-ga VSS provider's DeleteSnapshots() is also called
> and returns E_NOTIMPL, that makes the deletion fail.
> To avoid this issue, return S_OK and set values that represent no snapshots
> are deleted by qemu-ga VSS provider.
>
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>

Reviewed-by: Gal Hammer <ghammer@redhat.com>

     Gal.

Re: [Qemu-devel] [PATCH 3/3] qga: vss-win32: Fix interference with snapshot deletion by other VSS request

[Yan Vugenfirer]

On Jan 13, 2014, at 7:25 PM, Tomoki Sekiyama <tomoki.sekiyama@hds.com> wrote:

> When a VSS requester such as vshadow.exe or diskshadow.exe requests to
> delete snapshots, qemu-ga VSS provider's DeleteSnapshots() is also called
> and returns E_NOTIMPL, that makes the deletion fail.
> To avoid this issue, return S_OK and set values that represent no snapshots
> are deleted by qemu-ga VSS provider.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> ---
> qga/vss-win32/provider.cpp |    4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
> index b233646..d5129f8 100644
> --- a/qga/vss-win32/provider.cpp
> +++ b/qga/vss-win32/provider.cpp
> @@ -278,7 +278,9 @@ STDMETHODIMP CQGAVssProvider::DeleteSnapshots(
>     VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
>     BOOL bForceDelete, LONG *plDeletedSnapshots, VSS_ID *pNondeletedSnapshotID)
> {
> -    return E_NOTIMPL;
> +    *plDeletedSnapshots = 0;
> +    *pNondeletedSnapshotID = SourceObjectId;
> +    return S_OK;
> }
> 
> STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot(
> 
> 

Reviewed-by: Yan Vugenfirer <yvugenfi@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] qga: vss-win32: Fix interference with snapshot creation by other VSS requesters

[Yan Vugenfirer]

On Jan 13, 2014, at 7:25 PM, Tomoki Sekiyama <tomoki.sekiyama@hds.com> wrote:

> When a VSS requester such as vshadow.exe or diskshadow.exe requests to
> create disk snapshots, Windows may choose qemu-ga VSS provider if it is
> only provider registered on the system. However, because it provides only a
> function to freeze the filesystem, the snapshotting fails.
> 
> This patch adds a check into CQGAVssProvider::IsVolumeSupported() to reject
> the request from other VSS requesters, so that the other provider is chosen.
> 
> The check of requester is done by confirming event channels between
> qemu-ga's requester and provider established. To ensure that the events are
> initialized when CQGAVssProvider::IsVolumeSupported() is called, it moves
> the initialization earlier.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> ---
> qga/vss-win32/provider.cpp  |   11 ++++++++-
> qga/vss-win32/requester.cpp |   52 ++++++++++++++++++++++---------------------
> 2 files changed, 36 insertions(+), 27 deletions(-)
> 
> diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
> index c3030d8..b233646 100644
> --- a/qga/vss-win32/provider.cpp
> +++ b/qga/vss-win32/provider.cpp
> @@ -291,8 +291,17 @@ STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot(
> STDMETHODIMP CQGAVssProvider::IsVolumeSupported(
>     VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider)
> {
> -    *pbSupportedByThisProvider = TRUE;
> +    HANDLE hEventFrozen;
> +
> +    /* Check if a requester is qemu-ga by whether an event is created */
> +    hEventFrozen = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_FROZEN);
> +    if (!hEventFrozen) {
> +        *pbSupportedByThisProvider = FALSE;
> +        return S_OK;
> +    }
> +    CloseHandle(hEventFrozen);
> 
> +    *pbSupportedByThisProvider = TRUE;
>     return S_OK;
> }
> 
> diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
> index 0a55447..922e74d 100644
> --- a/qga/vss-win32/requester.cpp
> +++ b/qga/vss-win32/requester.cpp
> @@ -252,6 +252,32 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
> 
>     CoInitialize(NULL);
> 
> +    /* Allow unrestricted access to events */
> +    InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION);
> +    SetSecurityDescriptorDacl(&sd, TRUE, NULL, FALSE);
> +    sa.nLength = sizeof(sa);
> +    sa.lpSecurityDescriptor = &sd;
> +    sa.bInheritHandle = FALSE;
> +
> +    vss_ctx.hEventFrozen = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_FROZEN);
> +    if (!vss_ctx.hEventFrozen) {
> +        err_set(errset, GetLastError(), "failed to create event %s",
> +                EVENT_NAME_FROZEN);
> +        goto out;
> +    }
> +    vss_ctx.hEventThaw = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_THAW);
> +    if (!vss_ctx.hEventThaw) {
> +        err_set(errset, GetLastError(), "failed to create event %s",
> +                EVENT_NAME_THAW);
> +        goto out;
> +    }
> +    vss_ctx.hEventTimeout = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_TIMEOUT);
> +    if (!vss_ctx.hEventTimeout) {
> +        err_set(errset, GetLastError(), "failed to create event %s",
> +                EVENT_NAME_TIMEOUT);
> +        goto out;
> +    }
> +
>     assert(pCreateVssBackupComponents != NULL);
>     hr = pCreateVssBackupComponents(&vss_ctx.pVssbc);
>     if (FAILED(hr)) {
> @@ -362,32 +388,6 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
>         goto out;
>     }
> 
> -    /* Allow unrestricted access to events */
> -    InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION);
> -    SetSecurityDescriptorDacl(&sd, TRUE, NULL, FALSE);
> -    sa.nLength = sizeof(sa);
> -    sa.lpSecurityDescriptor = &sd;
> -    sa.bInheritHandle = FALSE;
> -
> -    vss_ctx.hEventFrozen = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_FROZEN);
> -    if (!vss_ctx.hEventFrozen) {
> -        err_set(errset, GetLastError(), "failed to create event %s",
> -                EVENT_NAME_FROZEN);
> -        goto out;
> -    }
> -    vss_ctx.hEventThaw = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_THAW);
> -    if (!vss_ctx.hEventThaw) {
> -        err_set(errset, GetLastError(), "failed to create event %s",
> -                EVENT_NAME_THAW);
> -        goto out;
> -    }
> -    vss_ctx.hEventTimeout = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_TIMEOUT);
> -    if (!vss_ctx.hEventTimeout) {
> -        err_set(errset, GetLastError(), "failed to create event %s",
> -                EVENT_NAME_TIMEOUT);
> -        goto out;
> -    }
> -
>     /*
>      * Start VSS quiescing operations.
>      * CQGAVssProvider::CommitSnapshots will kick vss_ctx.hEventFrozen
> 
> 

Reviewed-by: Yan Vugenfirer <yvugenfi@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] qga: vss-win32: Use NULL as an invalid pointer for OpenEvent and CreateEvent

[Yan Vugenfirer]

On Jan 13, 2014, at 7:25 PM, Tomoki Sekiyama <tomoki.sekiyama@hds.com> wrote:

> OpenEvent and CreateEvent WinAPI return NULL when failed to open/create
> events handles, instead of INVALID_HANDLE_VALUE (although their return
> types are HANDLE).
> This replaces INVALID_HANDLE_VALUE related to event handles with NULL.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> ---
> qga/vss-win32/provider.cpp  |    6 +++---
> qga/vss-win32/requester.cpp |   24 ++++++++++--------------
> 2 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
> index bf42b5e..c3030d8 100644
> --- a/qga/vss-win32/provider.cpp
> +++ b/qga/vss-win32/provider.cpp
> @@ -342,18 +342,18 @@ STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId)
>     HANDLE hEventFrozen, hEventThaw, hEventTimeout;
> 
>     hEventFrozen = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_FROZEN);
> -    if (hEventFrozen == INVALID_HANDLE_VALUE) {
> +    if (!hEventFrozen) {
>         return E_FAIL;
>     }
> 
>     hEventThaw = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_THAW);
> -    if (hEventThaw == INVALID_HANDLE_VALUE) {
> +    if (!hEventThaw) {
>         CloseHandle(hEventFrozen);
>         return E_FAIL;
>     }
> 
>     hEventTimeout = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_TIMEOUT);
> -    if (hEventTimeout == INVALID_HANDLE_VALUE) {
> +    if (!hEventTimeout) {
>         CloseHandle(hEventFrozen);
>         CloseHandle(hEventThaw);
>         return E_FAIL;
> diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
> index 1e8dd3d..0a55447 100644
> --- a/qga/vss-win32/requester.cpp
> +++ b/qga/vss-win32/requester.cpp
> @@ -50,10 +50,6 @@ static struct QGAVSSContext {
> 
> STDAPI requester_init(void)
> {
> -    vss_ctx.hEventFrozen =  INVALID_HANDLE_VALUE;
> -    vss_ctx.hEventThaw = INVALID_HANDLE_VALUE;
> -    vss_ctx.hEventTimeout = INVALID_HANDLE_VALUE;
> -
>     COMInitializer initializer; /* to call CoInitializeSecurity */
>     HRESULT hr = CoInitializeSecurity(
>         NULL, -1, NULL, NULL, RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
> @@ -94,17 +90,17 @@ STDAPI requester_init(void)
> 
> static void requester_cleanup(void)
> {
> -    if (vss_ctx.hEventFrozen != INVALID_HANDLE_VALUE) {
> +    if (vss_ctx.hEventFrozen) {
>         CloseHandle(vss_ctx.hEventFrozen);
> -        vss_ctx.hEventFrozen = INVALID_HANDLE_VALUE;
> +        vss_ctx.hEventFrozen = NULL;
>     }
> -    if (vss_ctx.hEventThaw != INVALID_HANDLE_VALUE) {
> +    if (vss_ctx.hEventThaw) {
>         CloseHandle(vss_ctx.hEventThaw);
> -        vss_ctx.hEventThaw = INVALID_HANDLE_VALUE;
> +        vss_ctx.hEventThaw = NULL;
>     }
> -    if (vss_ctx.hEventTimeout != INVALID_HANDLE_VALUE) {
> +    if (vss_ctx.hEventTimeout) {
>         CloseHandle(vss_ctx.hEventTimeout);
> -        vss_ctx.hEventTimeout = INVALID_HANDLE_VALUE;
> +        vss_ctx.hEventTimeout = NULL;
>     }
>     if (vss_ctx.pAsyncSnapshot) {
>         vss_ctx.pAsyncSnapshot->Release();
> @@ -374,19 +370,19 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
>     sa.bInheritHandle = FALSE;
> 
>     vss_ctx.hEventFrozen = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_FROZEN);
> -    if (vss_ctx.hEventFrozen == INVALID_HANDLE_VALUE) {
> +    if (!vss_ctx.hEventFrozen) {
>         err_set(errset, GetLastError(), "failed to create event %s",
>                 EVENT_NAME_FROZEN);
>         goto out;
>     }
>     vss_ctx.hEventThaw = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_THAW);
> -    if (vss_ctx.hEventThaw == INVALID_HANDLE_VALUE) {
> +    if (!vss_ctx.hEventThaw) {
>         err_set(errset, GetLastError(), "failed to create event %s",
>                 EVENT_NAME_THAW);
>         goto out;
>     }
>     vss_ctx.hEventTimeout = CreateEvent(&sa, TRUE, FALSE, EVENT_NAME_TIMEOUT);
> -    if (vss_ctx.hEventTimeout == INVALID_HANDLE_VALUE) {
> +    if (!vss_ctx.hEventTimeout) {
>         err_set(errset, GetLastError(), "failed to create event %s",
>                 EVENT_NAME_TIMEOUT);
>         goto out;
> @@ -443,7 +439,7 @@ void requester_thaw(int *num_vols, ErrorSet *errset)
> {
>     COMPointer<IVssAsync> pAsync;
> 
> -    if (vss_ctx.hEventThaw == INVALID_HANDLE_VALUE) {
> +    if (!vss_ctx.hEventThaw) {
>         /*
>          * In this case, DoSnapshotSet is aborted or not started,
>          * and no volumes must be frozen. We return without an error.
> 
> 

Reviewed-by: Yan Vugenfirer <yvugenfi@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] qga: vss-win32: Fix interference with other VSS requesters

[Michael Roth]

Quoting Tomoki Sekiyama (2014-01-13 11:25:14)
> Current functionarity of qemu-ga VSS provider is limited to implement
> filesystems freeze, and doesn't support the creation of shadow copies
> within the guest.
> However, when no other hardware snapshot provider is installed, VSS may
> choose qemu-ga VSS provider to create shadow copies and fail with
> VSS_E_UNEXPECTED_PROVIDER_ERROR.
> Similar issue occurs when the requester deletes shadow copies.
> 
> This patchset fix this issue by telling VSS that the volume is not
> supported by qemu-ga VSS provider when it is kicked by other requesters.
> 
> It also fixes wrong error handling around OpenEvent/CreateEvent WinAPI,
> which returns NULL instead of INVALID_HANDLE_VALUE on errors.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1036341
> 
> ---
> Tomoki Sekiyama (3):
>       qga: vss-win32: Use NULL as an invalid pointer for OpenEvent and CreateEvent
>       qga: vss-win32: Fix interference with snapshot creation by other VSS requesters
>       qga: vss-win32: Fix interference with snapshot deletion by other VSS request

Thanks, applied to qga tree:

  https://github.com/mdroth/qemu/commits/qga 

> 
> 
>  qga/vss-win32/provider.cpp  |   21 ++++++++++---
>  qga/vss-win32/requester.cpp |   70 ++++++++++++++++++++-----------------------
>  2 files changed, 49 insertions(+), 42 deletions(-)
> 
> -- 
> Tomoki Sekiyama