Thanks for your review. On 6/25/13 12:03 , "Laszlo Ersek" <ler...@redhat.com> 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" = yes ; then >> virtfs=no >> fi >> fi >> - 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 >> 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: > >$ ./configure --without-pixman --disable-system --disable-tools \ > --cross-prefix=i686-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 += 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 = qga/ qapi-types.o qapi-visit.o >> +qga-prv-obj-y = qga/ >> >> vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS) >> >> @@ -113,6 +114,7 @@ nested-vars += \ >> stub-obj-y \ >> util-obj-y \ >> qga-obj-y \ >> + qga-prv-obj-y \ >> block-obj-y \ >> common-obj-y >> dummy := $(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) += commands-posix.o >>channel-posix.o >> qga-obj-$(CONFIG_WIN32) += commands-win32.o channel-win32.o >>service-win32.o >> qga-obj-y += qapi-generated/qga-qapi-types.o >>qapi-generated/qga-qapi-visit.o >> qga-obj-y += qapi-generated/qga-qmp-marshal.o >> + >> +ifeq ($(CONFIG_QGA_VSS),y) >> +QEMU_CFLAGS += -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 += vss-win32-provider/ >> +qga-prv-obj-y += 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 += qga-provider.dll >> +qga-prv-obj-y += provider.o install.o >> + >> +obj-qga-prv-obj-y = $(addprefix $(obj)/, $(qga-prv-obj-y)) >> +$(obj-qga-prv-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-provider.dll: LDFLAGS = -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) > > 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
