Re: [Qemu-devel] [RFC PATCH v3 05/11] qemu-ga: Add Windows VSS provider to quiesce applications on fsfreeze

[Jeff Cody]

On Thu, May 23, 2013 at 06:36:35PM +0000, Tomoki Sekiyama wrote:
> On 5/23/13 8:22 , "Stefan Hajnoczi" <address@hidden> wrote:
> 
> >On Tue, May 21, 2013 at 11:33:52AM -0400, Tomoki Sekiyama wrote:
> >> Implements a basic stub of software VSS provider. Currently, this
> >>modules
> >> only provides a relay function of events between qemu-guest-agent and
> >> Windows VSS when VSS finished filesystem freeze and when qemu snapshot
> >> is done.
> >> 
> >> In the future, this module could be extended to support the other VSS
> >> functions, such as query for snapshot volumes and recovery.
> >> 
> >> Signed-off-by: Tomoki Sekiyama <address@hidden>
> >> ---
> >>  Makefile.objs                           |    2
> >>  configure                               |    5
> >>  qga/Makefile.objs                       |    6
> >>  qga/vss-win32-provider.h                |   26 ++
> >>  qga/vss-win32-provider/Makefile.objs    |   21 +
> >>  qga/vss-win32-provider/install.cpp      |  494
> >>+++++++++++++++++++++++++++++++
> >>  qga/vss-win32-provider/provider.cpp     |  474
> >>++++++++++++++++++++++++++++++
> >>  qga/vss-win32-provider/qga-provider.def |   10 +
> >>  qga/vss-win32-provider/qga-provider.idl |   20 +
> >>  qga/vss-win32.h                         |   85 +++++
> >>  10 files changed, 1142 insertions(+), 1 deletion(-)
> >>  create mode 100644 qga/vss-win32-provider.h
> >>  create mode 100644 qga/vss-win32-provider/Makefile.objs
> >>  create mode 100644 qga/vss-win32-provider/install.cpp
> >>  create mode 100644 qga/vss-win32-provider/provider.cpp
> >>  create mode 100644 qga/vss-win32-provider/qga-provider.def
> >>  create mode 100644 qga/vss-win32-provider/qga-provider.idl
> >>  create mode 100644 qga/vss-win32.h
> >
> >Please run scripts/checkpatch.pl and use QEMU coding style (see
> >./CODING_STYLE and ./HACKING).
> 
> Ah, actually this passes checkpatch.pl, but I notice that
> checkpatch.pl skips some checks for .cpp files ...
> I will enable them for .cpp files and fix some style errors.
> 
> >> diff --git a/qga/vss-win32-provider/Makefile.objs
> >>b/qga/vss-win32-provider/Makefile.objs
> >> new file mode 100644
> >> index 0000000..73ef752
> >> --- /dev/null
> >> +++ b/qga/vss-win32-provider/Makefile.objs
> >> @@ -0,0 +1,21 @@
> >> +# 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)$@")
> >> +
> >> +
> >> +# rules to build qga-provider.tlb
> >> +# Currently, only native build is supported because building .tlb
> >> +# (TypeLibrary) from .idl requires WindowsSDK and MIDL (included in
> >>VC++).
> >> +MIDL=midl
> >> +WINSDK="C:\\Program Files\\Microsoft SDKs\\Windows\\v7.1\\Include"
> >
> >This needs to be a ./configure option.
> 
> OK. I will add something like --with-winsdk="\\path\to\windows\sdk" to
> configure this.
> 

Patch 9/11 adds a binary tlb in, so that midl is not required. I would
like to see patch 09/11 just be squashed into this patch, so the build
will always work without requiring midl.  Otherwise, from this patch
up through patch 9, qemu-ga.exe will fail to build unless you have the
Windows SDK installed.

And if that is done, I don't know if this needs a ./configure option
or not, because you will only build the .tlb if you explicitly purge
the existing binary.  Stefan, would you still like to see it in
./configure option, in that case?


> >> diff --git a/qga/vss-win32.h b/qga/vss-win32.h
> >> new file mode 100644
> >> index 0000000..7600087
> >> --- /dev/null
> >> +++ b/qga/vss-win32.h
> >> @@ -0,0 +1,85 @@
> >> +/*
> >> + * QEMU Guest Agent win32 VSS common declarations
> >> + *
> >> + * Copyright Hitachi Data Systems Corp. 2013
> >> + *
> >> + * Authors:
> >> + *  Tomoki Sekiyama   <address@hidden>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or
> >>later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#ifndef VSS_WIN32_H
> >> +#define VSS_WIN32_H
> >> +
> >> +#define __MIDL_user_allocate_free_DEFINED__
> >> +#include "config-host.h"
> >> +#include <windows.h>
> >> +#include <shlwapi.h>
> >> +
> >> +/* Reduce warnings to include vss.h */
> >> +#define __in  IN
> >> +#define __out OUT
> >> +#define __RPC_unique_pointer
> >> +#define __RPC_string
> >> +#define __RPC__deref_inout_opt
> >> +#define __RPC__out
> >> +#ifndef __RPC__out_ecount_part
> >> +#define __RPC__out_ecount_part(x, y)
> >> +#endif
> >> +#define _declspec(x)
> >> +#undef uuid
> >> +#define uuid(x)
> >
> >This looks hacky.  Why are you stubbing out macros that vss.h uses?
> 
> Because these macros are internally defined/used by windows SDK's
> headers and not fully covered by mingw (some of them is only
> meaningful with Microsoft IDE), so it don't compile without
> these define's.
> 
> This could be better if mingw supports these VSS headers, but
> I have no idea how to achieve that...
> 
> Thanks,
> Tomoki Sekiyama
> 
>