Re: [Qemu-devel] [PATCH v8 07/10] qemu-ga: Add Windows VSS provider and requester as DLL

[Tomoki Sekiyama]

Hi Michael,
Thank you for the review.

>>-  if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
>>+  if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o
>>"$mingw32" = "yes" ] ; then
>>      if [ "$guest_agent" = "yes" ]; then
>>        tools="qemu-ga\$(EXESUF) $tools"
>>+      if [ "$mingw32" = "yes" ]; then
>>+        tools="qga/vss-win32-provider/qga-provider.dll
>>qga/vss-win32-provider/qga-provider.tlb $tools"
>>+      fi
>
>It looks like this hasn't been updated to reflect the change in directory
>and
>tlb/dll filenames from an earlier version.
>
>Also, when CONFIG_QGA_VSS=n, we shouldn't make the dll/tlb dependencies of
>$tools, else we'll get a build error.
>
>The following changes seem to fix things for me:
>
>-      if [ "$mingw32" = "yes" ]; then
>-        tools="qga/vss-win32-provider/qga-provider.dll
>qga/vss-win32-provider/qga-provider.tlb $tools"
>+      if [ "$mingw32" = "yes" -a "$guest_agent_with_vss" = "yes" ]; then
>+        tools="qga/vss-win32/qga-vss.dll qga/vss-win32/qga-vss.tlb
>$tools"

Ah, thanks for pointing this out. I will fix into this.

>>+
>>+ifeq ($(CONFIG_QGA_VSS),y)
>>+qga-vss-dll-obj-y += vss-win32/
>>+endif
>
>Small nit, but I think we generally prefer this form in qemu:
>
>qga-vss-dll-obj-$(CONFIG_QGA_VSS) += vss-win32

OK.

>> diff --git a/qga/vss-win32/Makefile.objs b/qga/vss-win32/Makefile.objs
>> new file mode 100644
>> index 0000000..4dad964
>> --- /dev/null
>> +++ b/qga/vss-win32/Makefile.objs
>> @@ -0,0 +1,23 @@
>> +# rules to build qga-vss.dll
>> +
>> +qga-vss-dll-obj-y += requester.o provider.o install.o
>> +
>> +obj-qga-vss-dll-obj-y = $(addprefix $(obj)/, $(qga-vss-dll-obj-y))
>> +$(obj-qga-vss-dll-obj-y): QEMU_CXXFLAGS = $(filter-out
>>-Wstrict-prototypes -Wmissing-prototypes -Wnested-externs
>>-Wold-style-declaration -Wold-style-definition -Wredundant-decls
>>-fstack-protector-all, $(QEMU_CFLAGS)) -Wno-unknown-pragmas
>>-Wno-delete-non-virtual-dtor
>> +
>> +$(obj)/qga-vss.dll: LDFLAGS = -shared
>>-Wl,--add-stdcall-alias,--enable-stdcall-fixup -lole32 -loleaut32
>>-lshlwapi -luuid -static
>> +$(obj)/qga-vss.dll: $(obj-qga-vss-dll-obj-y)
>>$(SRC_PATH)/$(obj)/qga-vss.def
>> +       $(call quiet-command,$(CXX) -o $@ $(qga-vss-dll-obj-y)
>>$(SRC_PATH)/qga/vss-win32/qga-vss.def $(CXXFLAGS) $(LDFLAGS),"  LINK
>>$(TARGET_DIR)$@")
>> +
>> +
>> +# rules to build qga-provider.tlb
>> +# Currently, only native build is supported because building .tlb
>> +# (TypeLibrary) from .idl requires WindowsSDK and MIDL (and cl.exe in
>>VC++).
>> +MIDL=$(WIN_SDK)/Bin/midl
>> +
>> +$(obj)/qga-vss.tlb: $(SRC_PATH)/$(obj)/qga-vss.idl
>> +ifeq ($(wildcard $(SRC_PATH)/$(obj)/qga-vss.tlb),)
>> +       $(call quiet-command,$(MIDL) -tlb $@ -I $(WIN_SDK)/Include $<,"
>> MIDL  $(TARGET_DIR)$@")
>
>Is the only way to execute this to delete qga-vss.tlb from the source
>directory? Not a big deal, but seems a bit awkward for users/builders.
>
>It looks like WIN_SDK exists only for this purpose, so perhaps just
>default
>WIN_SDK to "no". Then if they specify --with-win-sdk with no parameters,
>we do
>the current defaults in configure, and if they specify it explicitly we
>use that
>path. Basically the same thing we do for --with-vss-sdk/--without-vss-sdk
>currently.
>
>Then we do something like:
>
>  $(obj)/qga-vss.tlb: $(SRC_PATH)/$(obj)/qga-vss.idl
>  ifeq ($(WIN_SDK),)
>         $(call quiet-command,cp $(dir $<)qga-vss.tlb $@, "  COPY
>$(TARGET_DIR)$@")
>  else
>         $(call quiet-command,$(MIDL) -tlb $@ -I $(WIN_SDK)/Include $<,"
>MIDL  $(TARGET_DIR)$@")
>  Endif

OK, I will take this way.


>> diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
>> new file mode 100644
>> index 0000000..e39526a
>> --- /dev/null
>> +++ b/qga/vss-win32/provider.cpp
<snip>
>> +STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId)
>> +{
>> +    HRESULT hr = S_OK;
>> +    HANDLE hEventFrozen, hEventThaw;
>> +
>> +    hEventFrozen = OpenEvent(EVENT_ALL_ACCESS, FALSE,
>>EVENT_NAME_FROZEN);
>> +    if (hEventFrozen == INVALID_HANDLE_VALUE) {
>> +        return E_FAIL;
>> +    }
>> +
>> +    hEventThaw = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_THAW);
>> +    if (hEventThaw == INVALID_HANDLE_VALUE) {
>> +        CloseHandle(hEventFrozen);
>> +        return E_FAIL;
>> +    }
>> +
>> +    /* Send event to qemu-ga to notify filesystem is frozen */
>> +    SetEvent(hEventFrozen);
>> +
>> +    /* Wait until the snapshot is taken by the host. */
>> +    if (WaitForSingleObject(hEventThaw, VSS_TIMEOUT_MSEC) !=
>>WAIT_OBJECT_0) {
>> +        hr = E_ABORT;
>> +    }
>
>I see that we approximate both the 10 second fsfreeze and 60s writer
>freeze timeouts using local event timeouts, but I think there's a chance
>for some races there.
>
>We could maybe lower the timeouts a bit to be safe, but i don't really
>like the sound of that...would be nice if there was some way to
>
>a) ideally, guarantee the guest-fsfreeze-thaw will return an error if
>   the fsfreeze timed out before it was called, or
>b) guarantee at least that it will return an error if the vss writer
>   freeze timed out.

When the writer freeze timeout is exceeded, VSS service will
automatically abort the snapshot sequence and report an error
(E_VSS_WRITES_TIMEOUT) to the requester, even if the provider
is still sleeping. This behavior guarantees the following
guest-fsfreeze-thaw return the following error:
  {"error": {"class": "GenericError", "desc": "couldn't hold writes:
  fsfreeze is limited up to 10 seconds: (error:80042314)"}}

VSS_TIMEOUT_MSEC here is just not to freeze the provider forever
if the guest-fsfreeze-thaw is not issued.

However, if the provider returns the E_ABORT by timeout, VSS reports
VSS_E_UNEXPECTED_PROVIDER_ERROR, and it results in different error
message returned to the host.
(Possibly your comment below about overwritten error intended to
 address this issue?)
To ensure we get the E_VSS_WRITES_TIMEOUT on VSS timeout, the timeout
could be longer (e.g., 120 seconds).


>> diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
>> new file mode 100644
>> index 0000000..1b91ca5
>> --- /dev/null
>> +++ b/qga/vss-win32/requester.cpp
<snip>
>> +    /* Need to call QueryStatus several times to make VSS provider
>>progress */
>> +    for (i = 0; i < VSS_TIMEOUT_FREEZE_MSEC/VSS_TIMEOUT_EVENT_MSEC;
>>i++) {
>> +        HRESULT hr2 = vss_ctx.pAsyncSnapshot->QueryStatus(&hr, NULL);
>> +        if (FAILED(hr2)) {
>> +            err_set(errset, hr, "failed to do snapshot set");
>> +            goto out;
>> +        }
>> +        if (hr != VSS_S_ASYNC_PENDING) {
>> +            err_set(errset, E_FAIL,
>> +                    "DoSnapshotSet exited without Frozen event");
>> +            goto out;
>> +        }
>> +        wait_status = WaitForSingleObject(vss_ctx.hEventFrozen,
>> +                                          VSS_TIMEOUT_EVENT_MSEC);
>> +        if (wait_status != WAIT_TIMEOUT) {
>> +            break;
>> +        }
>> +    }
>> +    if (wait_status != WAIT_OBJECT_0) {
>> +        err_set(errset, E_FAIL,
>> +                "Couldn't receive Frozen event from VSS provider");
>> +        goto out;
>> +    }
>
>One small issue I noticed was that this error will get overwritten
>with the VSS writer timeout error if we wait longer than 60s before
>calling guest-fsfreeze-thaw. It might give some users false assurances
>about what aspects of their snapshot may be volatile so it's
>probably worth addressing.

This is an error returned against guest-fsfreeze-freeze, when the
writers or filesystems take more than 60s to quiesce.
(CQGAVssProvider::CommitSnapshots that issues FrozenEvent is
called after this quiescing succeeded.) The VSS sequence is aborted
at "out:". If this happens, as the system remains thawed state, the
following guest-fsfreeze-thaw will just return 0.


<snip>
>> diff --git a/qga/vss-win32/requester.h b/qga/vss-win32/requester.h
>> new file mode 100644
>> index 0000000..ccd197c
>> --- /dev/null
>> +++ b/qga/vss-win32/requester.h
<snip>
>> +typedef void (*QGAVSSReuqesterFunc)(int *, ErrorSet *);
>
>Should be QGAVSSRequesterFunc

Oops, thank you for catching this.
Will fix these typo in the next version.


Thanks,
-- 
Tomoki Sekiyama