Re: [Qemu-devel] [PATCH v4 06/10] qemu-ga: Add Windows VSS provider to quiesce applications on fsfreeze

[Tomoki Sekiyama]

Thanks for your review.

On 6/25/13 12:03 , "Laszlo Ersek" <address@hidden> 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