From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34239) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrXwC-00082j-Id for qemu-devel@nongnu.org; Tue, 25 Jun 2013 14:26:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UrXw9-000641-3K for qemu-devel@nongnu.org; Tue, 25 Jun 2013 14:25:56 -0400 Received: from usindpps05.hds.com ([207.126.252.18]:38227) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrXw8-00063x-OR for qemu-devel@nongnu.org; Tue, 25 Jun 2013 14:25:52 -0400 From: Tomoki Sekiyama Date: Tue, 25 Jun 2013 18:23:33 +0000 Message-ID: In-Reply-To: <51C9BF54.6000008@redhat.com> Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-ID: <907F9040E5BDE0449110439CDC5AEB56@hds.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v4 06/10] qemu-ga: Add Windows VSS provider to quiesce applications on fsfreeze List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: "libaiqing@huawei.com" , "mdroth@linux.vnet.ibm.com" , "stefanha@gmail.com" , "qemu-devel@nongnu.org" , "lcapitulino@redhat.com" , "vrozenfe@redhat.com" , "pbonzini@redhat.com" , Seiji Aguchi , "areis@redhat.com" Thanks for your review. On 6/25/13 12:03 , "Laszlo Ersek" wrote: >I'm trying to wrap my head around the build changes first. >> diff --git a/configure b/configure >> index cafe830..8e45fad 100755 >> --- a/configure >> +++ b/configure >> @@ -3490,9 +3490,12 @@ if test "$softmmu" =3D yes ; then >> virtfs=3Dno >> fi >> fi >> - 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 >> fi >> fi >> fi > >This adds three targets to "tools" on mingw32 -- "qemu-ga" hasn't been >there until now. Is this actually a small, unrelated bugfix? That is true, it is not necessary to build qemu-ga.exe. The intention was to do everything by just `make`. >I'm asking because I build upstream qemu-ga.exe on my RHEL-6 laptop as >follows -- I don't have pixman installed, and configure forces me to >specify --disable-tools as well: >=20 >$ ./configure --without-pixman --disable-system --disable-tools \ > --cross-prefix=3Di686-pc-mingw32- >$ make qemu-ga.exe > >Plain "make" after the above ./configure doesn't produce anything >useful. Will I have to specify the DLL and TLB targets too on the >command line? (Apologies, I can't try it out now, msiextract doesn't >work for me.) You don't have to specify DLL and TLB targets because qemu-ga.exe has dependencies to them. >> diff --git a/Makefile.objs b/Makefile.objs >> index 286ce06..b6c1505 100644 >> --- a/Makefile.objs >> +++ b/Makefile.objs >> @@ -102,6 +102,7 @@ common-obj-y +=3D disas/ >> # FIXME: a few definitions from qapi-types.o/qapi-visit.o are needed >> # by libqemuutil.a. These should be moved to a separate .json schema. >> qga-obj-y =3D qga/ qapi-types.o qapi-visit.o >> +qga-prv-obj-y =3D qga/ >> >> vl.o: QEMU_CFLAGS+=3D$(GPROF_CFLAGS) >> >> @@ -113,6 +114,7 @@ nested-vars +=3D \ >> stub-obj-y \ >> util-obj-y \ >> qga-obj-y \ >> + qga-prv-obj-y \ >> block-obj-y \ >> common-obj-y >> dummy :=3D $(call unnest-vars) > >What does this do? Does "qga-prv-obj-y" stand for "all objects under >'qga'"? No, these are objects for qga/vss-win32-provider/, used to build qga-provider.dll. It looks better to be fully spelled as Paolo said in another mail. >Or does it mean "look into qga/Makefile.objs for further objects"? Yes... >>diff --git a/qga/Makefile.objs b/qga/Makefile.objs >> index b8d7cd0..8d93866 100644 >> --- a/qga/Makefile.objs >> +++ b/qga/Makefile.objs >> @@ -3,3 +3,9 @@ qga-obj-$(CONFIG_POSIX) +=3D commands-posix.o >>channel-posix.o >> qga-obj-$(CONFIG_WIN32) +=3D commands-win32.o channel-win32.o >>service-win32.o >> qga-obj-y +=3D qapi-generated/qga-qapi-types.o >>qapi-generated/qga-qapi-visit.o >> qga-obj-y +=3D qapi-generated/qga-qmp-marshal.o >> + >> +ifeq ($(CONFIG_QGA_VSS),y) >> +QEMU_CFLAGS +=3D -DHAS_VSS_SDK > >Isn't this superfluous? First, I can find no other reference to >HAS_VSS_SDK in the series, second, CONFIG_QGA_VSS would be available >directly as a macro to source files. It is referenced in e.g. qga/commands-win32.c in patch 8/10. But as you say, I will omit this, by the second reason. >> +qga-obj-y +=3D vss-win32-provider/ >> +qga-prv-obj-y +=3D vss-win32-provider/ >> +endif > >So we're probably just saying "look into >vss-win32-provider/Makefile.objs for further object files", for both >variables. Yes... >> diff --git a/qga/vss-win32-provider/Makefile.objs >>b/qga/vss-win32-provider/Makefile.objs >> new file mode 100644 >> index 0000000..1fe1f8f >> --- /dev/null >> +++ b/qga/vss-win32-provider/Makefile.objs >> @@ -0,0 +1,24 @@ >> +# rules to build qga-provider.dll >> + >> +qga-obj-y +=3D qga-provider.dll >> +qga-prv-obj-y +=3D provider.o install.o >> + >> +obj-qga-prv-obj-y =3D $(addprefix $(obj)/, $(qga-prv-obj-y)) >> +$(obj-qga-prv-obj-y): QEMU_CXXFLAGS =3D $(filter-out -Wstrict-prototype= s >>-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-provider.dll: LDFLAGS =3D -shared >>-Wl,--add-stdcall-alias,--enable-stdcall-fixup -lole32 -loleaut32 >>-lshlwapi -luuid -static >> +$(obj)/qga-provider.dll: $(obj-qga-prv-obj-y) >>$(SRC_PATH)/$(obj)/qga-provider.def $(obj)/qga-provider.tlb >> + $(call quiet-command,$(CXX) -o $@ $(qga-prv-obj-y) >>$(SRC_PATH)/qga/vss-win32-provider/qga-provider.def $(CXXFLAGS) >>$(LDFLAGS)," LINK $(TARGET_DIR)$@") > >I'm having a hard time following this. > >Looks like "qga-prv-obj-y" consists of nothing more than "provider.o" >and "install.o", and that "qga-provider.dll" depends on them. In theory, >would it be possible *not* to introduce "qga-prv-obj-y" in the >higher-level Makefile.obj files, only here? Unfortunately, we need this at all levels to handle nested directories correctly, otherwise it fails on out-tree build. >Why does "qga-provider.dll" depend on ""qga-provider.tlb"? It is not >used in the link command. This was because .tlb is required to install .dll correctly. But it is not dependent on build time, so shouldn't be specified here. >Also, why is it necessary to collect the files "provider.o", "install.o" >and "qga-provider.def" into a DLL, and link qemu-ga.exe against that DLL >(via qga-obj-y)? Is this a VSS requirement? Can't we simply link these >objects into qemu-ga.exe statically, like the rest of "qga-obj-y"? > >Will some VSS service load the provider DLL independently of >qemu-ga.exe? Yes, VSS provider DLL is registered to VSS on installation of qemu-ga service (using .TLB), and loaded into VSS service component when snapshot is started. >If so, (a) how will they communicate Using COM interface. >(b) shouldn't "qga-obj-y" be independent of "qga-provider.dll" The DLL is linked to qemu-ga.exe because qemu-ga.exe kicks the registration of the DLL (COMRegister(), which depends on the DLL's Internal variable) at the installation of the qemu-ga service. >Could you please draw a dependency graph between these files? Like > > idl --> tlb > def --> dll <-- (provider.o, install.o) >=20 > Does one of "tlb" and "dll" depend on the other, or are they needed "in > parallel"? This graph is describing build dependencies correctly. At runtime, DLL needs TLB, otherwise it cannot be loaded into VSS. >... I'll try to continue here. I've peaked forward a little bit, and >CQGAVssProvider::CommitSnapshots() seems to be the central method. It >kicks the "frozen" event, then blocks until the "thaw" event is kicked >back. I wonder how those will translate to QMP communication with >libvirt. CommitSnapshots() is called by VSS, when the requestor of qemu-ga (introduced in next patch) initiates snapshot receiving "guest-fsfreeze-freeze" command. "frozen" event is used to tell qemu-ga.exe that the fs is frozen, then qemu-ga.exe will tell libvirt to "guest-fsfreeze-freeze" is done. "thaw" event is kicked by qemnu-ga.exe when it receives "guest-fsfreeze-thaw" command. The command is finished when VSS notify qeum-ga.exe requestor that VSS snapshot process is completed. >Thanks! >Laszlo Again, thank you for taking your time for this! Thanks, Tomoki Sekiyama