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

[Michael Roth]

Quoting Tomoki Sekiyama (2013-07-29 13:32:20)
> 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)"}}

I see, I was assuming we were using the events to track status, but
see now that we collect final status in the requester via
WaitForAsync(vss_ctx.pAsyncSnapshot).

That's great, so the VSS_E_HOLD_WRITES_TIMEOUT check allows us to
provide the same guarantees as the posix fsfreeze implementation.

Nice!

> 
> 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.

This is the example I'm referring to:

{'execute':'guest-fsfreeze-freeze'}
{"return": 2}
/* wait 10+ seconds */
{'execute':'guest-fsfreeze-thaw'}
{"error": {"class": "GenericError", "desc": "couldn't hold writes: fsfreeze is 
limited up to 10 seconds:  (error: 80042314)"}}
{'execute':'guest-fsfreeze-freeze'}
{"return": 2}
/* wait 60+ seconds */
{'execute':'guest-fsfreeze-thaw'}
{"error": {"class": "GenericError", "desc": "failed to do snapshot set:  
(error: 8004230f)"}}

It this seems to be because CommitSnapshot returns in the latter instance due
to VSS_TIMEOUT_MSEC wait and it's E_ABORT error message overwrites the
VSS_E_HOLD_WRITES_TIMEOUT from earlier. Perhaps we could just have
CommitSnapshot return VSS_E_HOLD_WRITES_TIMEOUT if it doesn't get the thaw
event in time? I think that error message is much more informative for users.

> 
> 
> <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