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
