From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46100) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4Fhc-0007us-EB for qemu-devel@nongnu.org; Tue, 30 Jul 2013 15:35:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4FhT-0000Wt-9T for qemu-devel@nongnu.org; Tue, 30 Jul 2013 15:35:24 -0400 Received: from mail-ob0-x235.google.com ([2607:f8b0:4003:c01::235]:46769) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4FhT-0000Wb-0k for qemu-devel@nongnu.org; Tue, 30 Jul 2013 15:35:15 -0400 Received: by mail-ob0-f181.google.com with SMTP id dn14so12371428obc.26 for ; Tue, 30 Jul 2013 12:35:13 -0700 (PDT) Sender: fluxion Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: References: Message-ID: <20130730193506.14585.70095@loki> Date: Tue, 30 Jul 2013 14:35:06 -0500 Subject: Re: [Qemu-devel] [PATCH v8 07/10] qemu-ga: Add Windows VSS provider and requester as DLL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tomoki Sekiyama , "qemu-devel@nongnu.org" Cc: "libaiqing@huawei.com" , "ghammer@redhat.com" , "stefanha@gmail.com" , "lcapitulino@redhat.com" , "vrozenfe@redhat.com" , "pbonzini@redhat.com" , Seiji Aguchi , "lersek@redhat.com" , "areis@redhat.com" Quoting Tomoki Sekiyama (2013-07-29 13:32:20) > Hi Michael, > Thank you for the review. > = > >>- if [ "$linux" =3D "yes" -o "$bsd" =3D "yes" -o "$solaris" =3D "yes" = ] ; then > >>+ if [ "$linux" =3D "yes" -o "$bsd" =3D "yes" -o "$solaris" =3D "yes" = -o > >>"$mingw32" =3D "yes" ] ; then > >> if [ "$guest_agent" =3D "yes" ]; then > >> tools=3D"qemu-ga\$(EXESUF) $tools" > >>+ if [ "$mingw32" =3D "yes" ]; then > >>+ tools=3D"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=3Dn, we shouldn't make the dll/tlb dependencie= s of > >$tools, else we'll get a build error. > > > >The following changes seem to fix things for me: > > > >- if [ "$mingw32" =3D "yes" ]; then > >- tools=3D"qga/vss-win32-provider/qga-provider.dll > >qga/vss-win32-provider/qga-provider.tlb $tools" > >+ if [ "$mingw32" =3D "yes" -a "$guest_agent_with_vss" =3D "yes" ];= then > >+ tools=3D"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 +=3D vss-win32/ > >>+endif > > > >Small nit, but I think we generally prefer this form in qemu: > > > >qga-vss-dll-obj-$(CONFIG_QGA_VSS) +=3D 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 +=3D requester.o provider.o install.o > >> + > >> +obj-qga-vss-dll-obj-y =3D $(addprefix $(obj)/, $(qga-vss-dll-obj-y)) > >> +$(obj-qga-vss-dll-obj-y): QEMU_CXXFLAGS =3D $(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 =3D -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=3D$(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 > > >> +STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId) > >> +{ > >> + HRESULT hr =3D S_OK; > >> + HANDLE hEventFrozen, hEventThaw; > >> + > >> + hEventFrozen =3D OpenEvent(EVENT_ALL_ACCESS, FALSE, > >>EVENT_NAME_FROZEN); > >> + if (hEventFrozen =3D=3D INVALID_HANDLE_VALUE) { > >> + return E_FAIL; > >> + } > >> + > >> + hEventThaw =3D OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_THAW= ); > >> + if (hEventThaw =3D=3D 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) !=3D > >>WAIT_OBJECT_0) { > >> + hr =3D 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 > > >> + /* Need to call QueryStatus several times to make VSS provider > >>progress */ > >> + for (i =3D 0; i < VSS_TIMEOUT_FREEZE_MSEC/VSS_TIMEOUT_EVENT_MSEC; > >>i++) { > >> + HRESULT hr2 =3D vss_ctx.pAsyncSnapshot->QueryStatus(&hr, NULL= ); > >> + if (FAILED(hr2)) { > >> + err_set(errset, hr, "failed to do snapshot set"); > >> + goto out; > >> + } > >> + if (hr !=3D VSS_S_ASYNC_PENDING) { > >> + err_set(errset, E_FAIL, > >> + "DoSnapshotSet exited without Frozen event"); > >> + goto out; > >> + } > >> + wait_status =3D WaitForSingleObject(vss_ctx.hEventFrozen, > >> + VSS_TIMEOUT_EVENT_MSEC); > >> + if (wait_status !=3D WAIT_TIMEOUT) { > >> + break; > >> + } > >> + } > >> + if (wait_status !=3D 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: (e= rror: 8004230f)"}} It this seems to be because CommitSnapshot returns in the latter instance d= ue 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 user= s. > = > = > > >> 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 > > >> +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