Patchwork [v4,06/10] qemu-ga: Add Windows VSS provider to quiesce applications on fsfreeze

login
register
mail settings
Submitter Tomoki Sekiyama
Date June 6, 2013, 3:06 p.m.
Message ID <20130606150645.10486.23215.stgit@hds.com>
Download mbox | patch
Permalink /patch/249449/
State New
Headers show

Comments

Tomoki Sekiyama - June 6, 2013, 3:06 p.m.
Implements a basic stub of software VSS provider. Currently, this module
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.

To build type library (.tlb) for qga-provider.dll from COM IDL (.idl),
VisualC++, MIDL and stdole2.tlb in Windows SDK are required.

This patch also adds pre-compiled .tlb file in the repository in order to
enable cross-compile qemu-ga.exe for Windows with VSS support.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
---
 Makefile                                |    2 
 Makefile.objs                           |    2 
 configure                               |    5 
 qga/Makefile.objs                       |    6 
 qga/vss-win32-provider.h                |   26 ++
 qga/vss-win32-provider/Makefile.objs    |   24 ++
 qga/vss-win32-provider/install.cpp      |  493 +++++++++++++++++++++++++++++++
 qga/vss-win32-provider/provider.cpp     |  479 ++++++++++++++++++++++++++++++
 qga/vss-win32-provider/qga-provider.def |   10 +
 qga/vss-win32-provider/qga-provider.idl |   20 +
 qga/vss-win32-provider/qga-provider.tlb |  Bin
 qga/vss-win32.h                         |   86 +++++
 12 files changed, 1151 insertions(+), 2 deletions(-)
 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-provider/qga-provider.tlb
 create mode 100644 qga/vss-win32.h
Laszlo Ersek - June 25, 2013, 4:03 p.m.
I'm trying to wrap my head around the build changes first.

On 06/06/13 17:06, Tomoki Sekiyama wrote:

> Implements a basic stub of software VSS provider. Currently, this
> module 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.
>
> To build type library (.tlb) for qga-provider.dll from COM IDL (.idl),
> VisualC++, MIDL and stdole2.tlb in Windows SDK are required.
>
> This patch also adds pre-compiled .tlb file in the repository in order
> to enable cross-compile qemu-ga.exe for Windows with VSS support.
>
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> ---
>  configure                               |    5
>  Makefile                                |    2
>  Makefile.objs                           |    2
>  qga/Makefile.objs                       |    6
>  qga/vss-win32-provider/Makefile.objs    |   24 ++
>  create mode 100644 qga/vss-win32-provider/Makefile.objs

> 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?

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.)


> diff --git a/Makefile b/Makefile
> index 4851ba0..636358d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -235,7 +235,7 @@ clean:
>  	rm -f qemu-options.def
>  	find . -name '*.[oda]' -type f -exec rm -f {} +
>  	find . -name '*.l[oa]' -type f -exec rm -f {} +
> -	rm -f $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
> +	rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
>  	rm -Rf .libs
>  	rm -f qemu-img-cmds.h
>  	@# May not be present in GENERATED_HEADERS

OK I guess.


> 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'"?

Or does it mean "look into qga/Makefile.objs for further objects"?


> 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.


> +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.


> 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?

Why does "qga-provider.dll" depend on ""qga-provider.tlb"? It is not
used in the link command.

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? If so, (a) how will they communicate, (b) shouldn't
"qga-obj-y" be independent of "qga-provider.dll" (and that would explain
the high-level introduction of "qga-prv-obj-y")?


> +
> +
> +# 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=$(WIN_SDK)/Bin/midl
> +
> +$(obj)/qga-provider.tlb: $(SRC_PATH)/$(obj)/qga-provider.idl
> +ifeq ($(wildcard $(SRC_PATH)/$(obj)/qga-provider.tlb),)
> +	$(call quiet-command,$(MIDL) -tlb $@ -I $(WIN_SDK)/Include $<,"  MIDL  $(TARGET_DIR)$@")
> +else
> +	$(call quiet-command,cp $(dir $<)qga-provider.tlb $@, "  COPY  $(TARGET_DIR)$@")
> +endif

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"?

... 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.

Thanks!
Laszlo


>  qga/vss-win32-provider.h                |   26 ++
>  qga/vss-win32-provider/install.cpp      |  493 +++++++++++++++++++++++++++++++
>  qga/vss-win32-provider/provider.cpp     |  479 ++++++++++++++++++++++++++++++
>  qga/vss-win32-provider/qga-provider.def |   10 +
>  qga/vss-win32-provider/qga-provider.idl |   20 +
>  qga/vss-win32-provider/qga-provider.tlb |  Bin
>  qga/vss-win32.h                         |   86 +++++
>  12 files changed, 1151 insertions(+), 2 deletions(-)
>  create mode 100644 qga/vss-win32-provider.h
>  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-provider/qga-provider.tlb
>  create mode 100644 qga/vss-win32.h
>

> diff --git a/qga/vss-win32-provider.h b/qga/vss-win32-provider.h
> new file mode 100644
> index 0000000..a437e71
> --- /dev/null
> +++ b/qga/vss-win32-provider.h
> @@ -0,0 +1,26 @@
> +/*
> + * QEMU Guest Agent win32 VSS provider declarations
> + *
> + * Copyright Hitachi Data Systems Corp. 2013
> + *
> + * Authors:
> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
> + *
> + * 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_PROVIDER_H
> +#define VSS_WIN32_PROVIDER_H
> +
> +#include <windows.h>
> +
> +STDAPI VSSCheckOSVersion(void);
> +
> +STDAPI COMRegister(void);
> +STDAPI COMUnregister(void);
> +
> +STDAPI DllRegisterServer(void);
> +STDAPI DllUnregisterServer(void);
> +
> +#endif

> diff --git a/qga/vss-win32-provider/install.cpp b/qga/vss-win32-provider/install.cpp
> new file mode 100644
> index 0000000..d6f1d55
> --- /dev/null
> +++ b/qga/vss-win32-provider/install.cpp
> @@ -0,0 +1,493 @@
> +/*
> + * QEMU Guest Agent win32 VSS Provider installer
> + *
> + * Copyright Hitachi Data Systems Corp. 2013
> + *
> + * Authors:
> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "../vss-win32.h"
> +#include "inc/win2003/vscoordint.h"
> +#include "../vss-win32-provider.h"
> +
> +#include <comadmin.h>
> +#include <wbemidl.h>
> +#include <comutil.h>
> +
> +extern HINSTANCE g_hinstDll;
> +
> +const GUID CLSID_COMAdminCatalog = { 0xF618C514, 0xDFB8, 0x11d1,
> +    {0xA2, 0xCF, 0x00, 0x80, 0x5F, 0xC7, 0x92, 0x35} };
> +const GUID IID_ICOMAdminCatalog = { 0xDD662187, 0xDFC2, 0x11d1,
> +    {0xA2, 0xCF, 0x00, 0x80, 0x5F, 0xC7, 0x92, 0x35} };
> +const GUID CLSID_WbemLocator = { 0x4590f811, 0x1d3a, 0x11d0,
> +    {0x89, 0x1f, 0x00, 0xaa, 0x00, 0x4b, 0x2e, 0x24} };
> +const GUID IID_IWbemLocator = { 0xdc12a687, 0x737f, 0x11cf,
> +    {0x88, 0x4d, 0x00, 0xaa, 0x00, 0x4b, 0x2e, 0x24} };
> +
> +static void errmsg(DWORD err, const char *text)
> +{
> +    char *msg = NULL, *nul = strchr(text, '(');
> +    int len = nul ? nul - text : -1;
> +
> +    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
> +                  FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
> +                  NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +                  (char *)&msg, 0, NULL);
> +    printf("%.*s. (Error: %lx) %s\n", len, text, err, msg);
> +    LocalFree(msg);
> +}
> +
> +static void errmsg_dialog(DWORD err, const char *text, const char *opt = "")
> +{
> +    char *msg, buf[512];
> +
> +    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
> +                  FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
> +                  NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +                  (char *)&msg, 0, NULL);
> +    snprintf(buf, sizeof(buf), "%s%s. (Error: %lx) %s\n", text, opt, err, msg);
> +    MessageBox(NULL, buf, "Error from " QGA_PROVIDER_NAME, MB_OK|MB_ICONERROR);
> +    LocalFree(msg);
> +}
> +
> +#define _chk(hr, status, msg, err_label)        \
> +    do {                                        \
> +        hr = (status);                          \
> +        if (FAILED(hr)) {                       \
> +            errmsg(hr, msg);                    \
> +            goto err_label;                     \
> +        }                                       \
> +    } while (0)
> +
> +#define chk(status) _chk(hr, status, "Failed to " #status, out)
> +
> +template<class T>
> +HRESULT put_Value(ICatalogObject *pObj, LPCWSTR name, T val)
> +{
> +    return pObj->put_Value(_bstr_t(name), _variant_t(val));
> +}
> +
> +/* Check whether this OS version supports VSS providers */
> +STDAPI VSSCheckOSVersion(void)
> +{
> +    OSVERSIONINFO OSver;
> +
> +    OSver.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> +    GetVersionEx(&OSver);
> +    if ((OSver.dwMajorVersion == 5 && OSver.dwMinorVersion >= 2) ||
> +       OSver.dwMajorVersion > 5) {
> +        return S_OK;
> +    }
> +    return S_FALSE;
> +}
> +
> +/* Lookup Administrators group name from winmgmt */
> +static HRESULT GetAdminName(_bstr_t &name)
> +{
> +    HRESULT hr;
> +    IWbemLocator *pLoc = NULL;
> +    IWbemServices *pSvc = NULL;
> +    IEnumWbemClassObject *pEnum = NULL;
> +    IWbemClassObject *pWobj = NULL;
> +    ULONG returned;
> +    _variant_t var;
> +
> +    chk(CoCreateInstance(CLSID_WbemLocator, NULL, CLSCTX_INPROC_SERVER,
> +                         IID_IWbemLocator, (LPVOID *)&pLoc));
> +    chk(pLoc->ConnectServer(_bstr_t(L"ROOT\\CIMV2"), NULL, NULL, NULL,
> +                            0, 0, 0, &pSvc));
> +    chk(CoSetProxyBlanket(pSvc, RPC_C_AUTHN_WINNT, RPC_C_AUTHZ_NONE,
> +                          NULL, RPC_C_AUTHN_LEVEL_CALL,
> +                          RPC_C_IMP_LEVEL_IMPERSONATE, NULL, EOAC_NONE));
> +    chk(pSvc->ExecQuery(_bstr_t(L"WQL"),
> +                        _bstr_t(L"select * from Win32_Account where "
> +                                "SID='S-1-5-32-544' and localAccount=TRUE"),
> +                        WBEM_FLAG_RETURN_IMMEDIATELY | WBEM_FLAG_FORWARD_ONLY,
> +                        NULL, &pEnum));
> +    if (!pEnum) {
> +        errmsg(E_FAIL, "Failed to query for Administrators");
> +        goto out;
> +    }
> +    chk(pEnum->Next(WBEM_INFINITE, 1, &pWobj, &returned));
> +    if (returned == 0) {
> +        errmsg(E_FAIL, "Failed to query for Administrators");
> +        goto out;
> +    }
> +
> +    chk(pWobj->Get(_bstr_t(L"Name"), 0, &var, 0, 0));
> +    name = var;
> +out:
> +    if (pLoc) {
> +        pLoc->Release();
> +    }
> +    if (pSvc) {
> +        pSvc->Release();
> +    }
> +    if (pEnum) {
> +        pEnum->Release();
> +    }
> +    if (pWobj) {
> +        pWobj->Release();
> +    }
> +    return hr;
> +}
> +
> +/* Register this module to COM+ Applications Catalog */
> +STDAPI COMRegister(void)
> +{
> +    HRESULT hr = E_FAIL;
> +    IUnknown *pUnknown = NULL;
> +    ICOMAdminCatalog *pCatalog = NULL;
> +    ICatalogCollection *pApps = NULL, *pRoles = NULL, *pUsersInRole = NULL;
> +    ICatalogObject *pObj = NULL;
> +    long n;
> +    _bstr_t name;
> +    _variant_t key;
> +    CHAR dllPath[MAX_PATH], tlbPath[MAX_PATH];
> +
> +    if (!g_hinstDll) {
> +        errmsg(E_FAIL, "Failed to initialize DLL");
> +        goto out;
> +    }
> +
> +    if (VSSCheckOSVersion() == S_FALSE) {
> +        printf("VSS provider is not supported in this OS version.\n");
> +        return S_FALSE; /* VSS feature is disabled */
> +    }
> +
> +    COMUnregister();
> +
> +    chk(CoInitialize(NULL));
> +    chk(CoCreateInstance(CLSID_COMAdminCatalog, NULL, CLSCTX_INPROC_SERVER,
> +                         IID_IUnknown, (void **)&pUnknown));
> +    chk(pUnknown->QueryInterface(IID_ICOMAdminCatalog, (void **)&pCatalog));
> +
> +    /* Install COM+ Component */
> +
> +    chk(pCatalog->GetCollection(_bstr_t(L"Applications"),
> +                                (IDispatch **)&pApps));
> +    chk(pApps->Populate());
> +    chk(pApps->Add((IDispatch **)&pObj));
> +    chk(put_Value(pObj, L"Name",        QGA_PROVIDER_LNAME));
> +    chk(put_Value(pObj, L"Description", QGA_PROVIDER_LNAME));
> +    chk(put_Value(pObj, L"ApplicationAccessChecksEnabled", true));
> +    chk(put_Value(pObj, L"Authentication",                 short(6)));
> +    chk(put_Value(pObj, L"AuthenticationCapability",       short(2)));
> +    chk(put_Value(pObj, L"ImpersonationLevel",             short(2)));
> +    chk(pApps->SaveChanges(&n));
> +    chk(pObj->get_Key(&key));
> +
> +    pObj->Release();
> +    pObj = NULL;
> +
> +    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
> +        errmsg(GetLastError(), "GetModuleFileName failed");
> +        goto out;
> +    }
> +    n = strlen(dllPath);
> +    if (n < 3) {
> +        errmsg(E_FAIL, "Failed to lookup dll");
> +    }
> +    strcpy(tlbPath, dllPath);
> +    strcpy(tlbPath+n-3, "TLB");
> +    printf("Registering " QGA_PROVIDER_NAME ":\n");
> +    printf("  %s\n", dllPath);
> +    printf("  %s\n", tlbPath);
> +    if (!PathFileExists(tlbPath)) {
> +        errmsg(ERROR_FILE_NOT_FOUND, "Failed to lookup tlb");
> +        goto out;
> +    }
> +
> +    chk(pCatalog->InstallComponent(_bstr_t(QGA_PROVIDER_LNAME),
> +                                   _bstr_t(dllPath), _bstr_t(tlbPath),
> +                                   _bstr_t("")));
> +
> +    /* Setup roles of the applicaion */
> +
> +    chk(pApps->GetCollection(_bstr_t(L"Roles"), key,
> +                             (IDispatch **)&pRoles));
> +    chk(pRoles->Populate());
> +    chk(pRoles->Add((IDispatch **)&pObj));
> +    chk(put_Value(pObj, L"Name",        L"Administrators"));
> +    chk(put_Value(pObj, L"Description", L"Administrators group"));
> +    chk(pRoles->SaveChanges(&n));
> +    chk(pObj->get_Key(&key));
> +
> +    pObj->Release();
> +    pObj = NULL;
> +
> +    /* Setup users in the role */
> +
> +    chk(pRoles->GetCollection(_bstr_t(L"UsersInRole"), key,
> +                              (IDispatch **)&pUsersInRole));
> +    chk(pUsersInRole->Populate());
> +
> +    chk(pUsersInRole->Add((IDispatch **)&pObj));
> +    chk(GetAdminName(name));
> +    chk(put_Value(pObj, L"User", _bstr_t(".\\") + name));
> +
> +    pObj->Release();
> +    pObj = NULL;
> +
> +    chk(pUsersInRole->Add((IDispatch **)&pObj));
> +    chk(put_Value(pObj, L"User", L"SYSTEM"));
> +    chk(pUsersInRole->SaveChanges(&n));
> +
> +out:
> +    if (pUnknown) {
> +        pUnknown->Release();
> +    }
> +    if (pCatalog) {
> +        pCatalog->Release();
> +    }
> +    if (pApps) {
> +        pApps->Release();
> +    }
> +    if (pRoles) {
> +        pRoles->Release();
> +    }
> +    if (pUsersInRole) {
> +        pUsersInRole->Release();
> +    }
> +    if (pObj) {
> +        pObj->Release();
> +    }
> +
> +    if (FAILED(hr)) {
> +        COMUnregister();
> +    }
> +    CoUninitialize();
> +
> +    return hr;
> +}
> +
> +/* Unregister this module from COM+ Applications Catalog */
> +STDAPI COMUnregister(void)
> +{
> +    HRESULT hr;
> +    IUnknown *pUnknown = NULL;
> +    ICOMAdminCatalog *pCatalog = NULL;
> +    ICatalogCollection *pColl = NULL;
> +    ICatalogObject *pObj = NULL;
> +    _variant_t var;
> +    long i, n;
> +
> +    if (VSSCheckOSVersion() == S_FALSE) {
> +        printf("VSS provider is not supported in this OS version.\n");
> +        return S_FALSE; /* VSS feature is disabled */
> +    }
> +
> +    chk(DllUnregisterServer());
> +
> +    chk(CoInitialize(NULL));
> +    chk(CoCreateInstance(CLSID_COMAdminCatalog, NULL, CLSCTX_INPROC_SERVER,
> +                         IID_IUnknown, (void **)&pUnknown));
> +    chk(pUnknown->QueryInterface(IID_ICOMAdminCatalog, (void **)&pCatalog));
> +
> +    chk(pCatalog->GetCollection(_bstr_t(L"Applications"),
> +                                (IDispatch **)&pColl));
> +    chk(pColl->Populate());
> +
> +    chk(pColl->get_Count(&n));
> +    for (i = n - 1; i >= 0; i--) {
> +        chk(pColl->get_Item(i, (IDispatch **)&pObj));
> +        chk(pObj->get_Value(_bstr_t(L"Name"), &var));
> +        if (var == _variant_t(QGA_PROVIDER_LNAME)) {
> +            printf("Removing COM+ Application: %S\n", (wchar_t *)_bstr_t(var));
> +            chk(pColl->Remove(i));
> +        }
> +    }
> +    chk(pColl->SaveChanges(&n));
> +
> +out:
> +    if (pUnknown) {
> +        pUnknown->Release();
> +    }
> +    if (pCatalog) {
> +        pCatalog->Release();
> +    }
> +    if (pColl) {
> +        pColl->Release();
> +    }
> +    if (pObj) {
> +        pObj->Release();
> +    }
> +    CoUninitialize();
> +
> +    return hr;
> +}
> +
> +
> +static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR data)
> +{
> +    HKEY  hKey;
> +    LONG  ret;
> +    DWORD size;
> +
> +    ret = RegCreateKeyEx(HKEY_CLASSES_ROOT, key, 0, NULL,
> +        REG_OPTION_NON_VOLATILE, KEY_WRITE, NULL, &hKey, NULL);
> +    if (ret != ERROR_SUCCESS) {
> +        goto out;
> +    }
> +
> +    if (data != NULL) {
> +        size = (lstrlen(data) + 1) * sizeof(TCHAR);
> +    } else {
> +        size = 0;
> +    }
> +
> +    ret = RegSetValueEx(hKey, value, 0, REG_SZ, (LPBYTE)data, size);
> +    RegCloseKey(hKey);
> +
> +out:
> +    if (ret != ERROR_SUCCESS) {
> +        /* We cannot printf here, and need MessageBox to report an error. */
> +        errmsg_dialog(ret, "Cannot add registry ", key);
> +        return FALSE;
> +    }
> +    return TRUE;
> +}
> +
> +/* Register this dll as a VSS provider */
> +STDAPI DllRegisterServer(void)
> +{
> +    IVssAdmin *pVssAdmin = NULL;
> +    HRESULT hr = E_FAIL;
> +    char dllPath[MAX_PATH];
> +    char key[256];
> +
> +    CoInitialize(NULL);
> +
> +    if (!g_hinstDll) {
> +        errmsg_dialog(hr, "Module instance is not available");
> +        goto out;
> +    }
> +
> +    /* Add this module to registery */
> +
> +    sprintf(key, "CLSID\\%s", g_szClsid);
> +    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
> +        goto out;
> +    }
> +
> +    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
> +        errmsg_dialog(GetLastError(), "GetModuleFileName failed");
> +        goto out;
> +    }
> +    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
> +    if (!CreateRegistryKey(key, NULL, dllPath)) {
> +        goto out;
> +    }
> +
> +    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
> +    if (!CreateRegistryKey(key, "ThreadingModel", "Apartment")) {
> +        goto out;
> +    }
> +
> +    sprintf(key, "CLSID\\%s\\ProgID", g_szClsid);
> +    if (!CreateRegistryKey(key, NULL, g_szProgid)) {
> +        goto out;
> +    }
> +
> +    if (!CreateRegistryKey(g_szProgid, NULL, QGA_PROVIDER_NAME)) {
> +        goto out;
> +    }
> +
> +    sprintf(key, "%s\\CLSID", g_szProgid);
> +    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
> +        goto out;
> +    }
> +
> +    hr = CoCreateInstance(CLSID_VSSCoordinator,
> +        NULL, CLSCTX_ALL, IID_IVssAdmin, (void **)&pVssAdmin);
> +    if (FAILED(hr)) {
> +        errmsg_dialog(hr, "CoCreateInstance(VSSCoordinator) failed");
> +        goto out;
> +    }
> +
> +    hr = pVssAdmin->RegisterProvider(
> +        g_gProviderId, CLSID_QGAVSSProvider,
> +        const_cast<WCHAR*>(QGA_PROVIDER_LNAME), VSS_PROV_SOFTWARE,
> +        const_cast<WCHAR*>(QGA_PROVIDER_VERSION), g_gProviderVersion);
> +    if (FAILED(hr)) {
> +        errmsg_dialog(hr, "RegisterProvider failed");
> +        goto out;
> +    }
> +
> +out:
> +    if (pVssAdmin) {
> +        pVssAdmin->Release();
> +    }
> +    CoUninitialize();
> +
> +    if (FAILED(hr)) {
> +        DllUnregisterServer();
> +    }
> +
> +    return hr;
> +}
> +
> +/* Unregister this VSS hardware provider from the system */
> +STDAPI DllUnregisterServer(void)
> +{
> +    TCHAR key[256];
> +    IVssAdmin *pVssAdmin = NULL;
> +
> +    CoInitialize(NULL);
> +
> +    HRESULT hr = CoCreateInstance(CLSID_VSSCoordinator,
> +         NULL, CLSCTX_ALL, IID_IVssAdmin, (void **)&pVssAdmin);
> +    if (SUCCEEDED(hr)) {
> +        hr = pVssAdmin->UnregisterProvider(g_gProviderId);
> +        pVssAdmin->Release();
> +    } else {
> +        errmsg_dialog(hr, "CoCreateInstance(VSSCoordinator) failed");
> +    }
> +
> +    sprintf(key, "CLSID\\%s", g_szClsid);
> +    SHDeleteKey(HKEY_CLASSES_ROOT, key);
> +    SHDeleteKey(HKEY_CLASSES_ROOT, g_szProgid);
> +
> +    CoUninitialize();
> +
> +    return S_OK; /* Uninstall should never fail */
> +}
> +
> +
> +/* Support functions for _bstr_t in MinGW: Originally written by: Diaa Sami */
> +
> +void __stdcall _com_issue_error(HRESULT hr)
> +{
> +    printf("_com_issue_error() called with parameter HRESULT = %lu", hr);
> +}
> +
> +namespace _com_util
> +{
> +    char * __stdcall ConvertBSTRToString(BSTR bstr)
> +    {
> +        const unsigned int stringLength = lstrlenW(bstr);
> +        char *const ascii = new char [stringLength + 1];
> +
> +        wcstombs(ascii, bstr, stringLength + 1);
> +
> +        return ascii;
> +    }
> +
> +    BSTR __stdcall ConvertStringToBSTR(const char *const ascii)
> +    {
> +        const unsigned int stringLength = lstrlenA(ascii);
> +        BSTR bstr = SysAllocStringLen(NULL, stringLength);
> +
> +        mbstowcs(bstr, ascii, stringLength + 1);
> +
> +        return bstr;
> +    }
> +}

> diff --git a/qga/vss-win32-provider/provider.cpp b/qga/vss-win32-provider/provider.cpp
> new file mode 100644
> index 0000000..90a3d25
> --- /dev/null
> +++ b/qga/vss-win32-provider/provider.cpp
> @@ -0,0 +1,479 @@
> +/*
> + * QEMU Guest Agent win32 VSS Provider implementations
> + *
> + * Copyright Hitachi Data Systems Corp. 2013
> + *
> + * Authors:
> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <stdio.h>
> +#include "../vss-win32.h"
> +#include "inc/win2003/vscoordint.h"
> +#include "inc/win2003/vsprov.h"
> +
> +static long g_nComObjsInUse;
> +HINSTANCE g_hinstDll;
> +
> +/* VSS common GUID's */
> +
> +const CLSID CLSID_VSSCoordinator = { 0xE579AB5F, 0x1CC4, 0x44b4,
> +    {0xBE, 0xD9, 0xDE, 0x09, 0x91, 0xFF, 0x06, 0x23} };
> +const IID IID_IVssAdmin = { 0x77ED5996, 0x2F63, 0x11d3,
> +    {0x8A, 0x39, 0x00, 0xC0, 0x4F, 0x72, 0xD8, 0xE3} };
> +
> +const IID IID_IVssHardwareSnapshotProvider = { 0x9593A157, 0x44E9, 0x4344,
> +    {0xBB, 0xEB, 0x44, 0xFB, 0xF9, 0xB0, 0x6B, 0x10} };
> +const IID IID_IVssSoftwareSnapshotProvider = { 0x609e123e, 0x2c5a, 0x44d3,
> +    {0x8f, 0x01, 0x0b, 0x1d, 0x9a, 0x47, 0xd1, 0xff} };
> +const IID IID_IVssProviderCreateSnapshotSet = { 0x5F894E5B, 0x1E39, 0x4778,
> +    {0x8E, 0x23, 0x9A, 0xBA, 0xD9, 0xF0, 0xE0, 0x8C} };
> +const IID IID_IVssProviderNotifications = { 0xE561901F, 0x03A5, 0x4afe,
> +    {0x86, 0xD0, 0x72, 0xBA, 0xEE, 0xCE, 0x70, 0x04} };
> +
> +const IID IID_IVssEnumObject = { 0xAE1C7110, 0x2F60, 0x11d3,
> +    {0x8A, 0x39, 0x00, 0xC0, 0x4F, 0x72, 0xD8, 0xE3} };
> +
> +
> +void LockModule(BOOL block)
> +{
> +    if (block) {
> +        InterlockedIncrement(&g_nComObjsInUse);
> +    } else {
> +        InterlockedDecrement(&g_nComObjsInUse);
> +    }
> +}
> +
> +/* Empty enumerator for VssObject */
> +
> +class CQGAVSSEnumObject : public IVssEnumObject
> +{
> +public:
> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppObj);
> +    STDMETHODIMP_(ULONG) AddRef();
> +    STDMETHODIMP_(ULONG) Release();
> +
> +    /* IVssEnumObject Methods */
> +    STDMETHODIMP Next(
> +        ULONG celt, VSS_OBJECT_PROP *rgelt, ULONG *pceltFetched);
> +    STDMETHODIMP Skip(ULONG celt);
> +    STDMETHODIMP Reset(void);
> +    STDMETHODIMP Clone(IVssEnumObject **ppenum);
> +
> +    /* CQGAVSSEnumObject Methods */
> +    CQGAVSSEnumObject();
> +    ~CQGAVSSEnumObject();
> +
> +private:
> +    long m_nRefCount;
> +};
> +
> +CQGAVSSEnumObject::CQGAVSSEnumObject()
> +{
> +    m_nRefCount = 0;
> +    LockModule(TRUE);
> +}
> +
> +CQGAVSSEnumObject::~CQGAVSSEnumObject()
> +{
> +    LockModule(FALSE);
> +}
> +
> +STDMETHODIMP CQGAVSSEnumObject::QueryInterface(REFIID riid, void **ppObj)
> +{
> +    if (riid == IID_IUnknown || riid == IID_IVssEnumObject) {
> +        *ppObj = static_cast<void*>(static_cast<IVssEnumObject*>(this));
> +        AddRef();
> +        return S_OK;
> +    }
> +    ppObj = NULL;
> +    return E_NOINTERFACE;
> +}
> +
> +STDMETHODIMP_(ULONG) CQGAVSSEnumObject::AddRef()
> +{
> +    return InterlockedIncrement(&m_nRefCount);
> +}
> +
> +STDMETHODIMP_(ULONG) CQGAVSSEnumObject::Release()
> +{
> +    long nRefCount = InterlockedDecrement(&m_nRefCount);
> +    if (m_nRefCount == 0) {
> +        delete this;
> +    }
> +    return nRefCount;
> +}
> +
> +STDMETHODIMP CQGAVSSEnumObject::Next(
> +    ULONG celt, VSS_OBJECT_PROP *rgelt, ULONG *pceltFetched)
> +{
> +    *pceltFetched = 0;
> +    return S_FALSE;
> +}
> +
> +STDMETHODIMP CQGAVSSEnumObject::Skip(ULONG celt)
> +{
> +    return S_FALSE;
> +}
> +
> +STDMETHODIMP CQGAVSSEnumObject::Reset(void)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVSSEnumObject::Clone(IVssEnumObject **ppenum)
> +{
> +    return E_NOTIMPL;
> +}
> +
> +
> +/* QGAVssProvider */
> +
> +class CQGAVssProvider :
> +    public IVssSoftwareSnapshotProvider,
> +    public IVssProviderCreateSnapshotSet,
> +    public IVssProviderNotifications
> +{
> +public:
> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppObj);
> +    STDMETHODIMP_(ULONG) AddRef();
> +    STDMETHODIMP_(ULONG) Release();
> +
> +    /* IVssSoftwareSnapshotProvider Methods */
> +    STDMETHODIMP SetContext(LONG lContext);
> +    STDMETHODIMP GetSnapshotProperties(
> +        VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp);
> +    STDMETHODIMP Query(
> +        VSS_ID QueriedObjectId, VSS_OBJECT_TYPE eQueriedObjectType,
> +        VSS_OBJECT_TYPE eReturnedObjectsType, IVssEnumObject **ppEnum);
> +    STDMETHODIMP DeleteSnapshots(
> +        VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
> +        BOOL bForceDelete, LONG *plDeletedSnapshots,
> +        VSS_ID *pNondeletedSnapshotID);
> +    STDMETHODIMP BeginPrepareSnapshot(
> +        VSS_ID SnapshotSetId, VSS_ID SnapshotId,
> +        VSS_PWSZ pwszVolumeName, LONG lNewContext);
> +    STDMETHODIMP IsVolumeSupported(
> +        VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider);
> +    STDMETHODIMP IsVolumeSnapshotted(
> +        VSS_PWSZ pwszVolumeName, BOOL *pbSnapshotsPresent,
> +        LONG *plSnapshotCompatibility);
> +    STDMETHODIMP SetSnapshotProperty(
> +        VSS_ID SnapshotId, VSS_SNAPSHOT_PROPERTY_ID eSnapshotPropertyId,
> +        VARIANT vProperty);
> +    STDMETHODIMP RevertToSnapshot(VSS_ID SnapshotId);
> +    STDMETHODIMP QueryRevertStatus(VSS_PWSZ pwszVolume, IVssAsync **ppAsync);
> +
> +    /* IVssProviderCreateSnapshotSet Methods */
> +    STDMETHODIMP EndPrepareSnapshots(VSS_ID SnapshotSetId);
> +    STDMETHODIMP PreCommitSnapshots(VSS_ID SnapshotSetId);
> +    STDMETHODIMP CommitSnapshots(VSS_ID SnapshotSetId);
> +    STDMETHODIMP PostCommitSnapshots(
> +        VSS_ID SnapshotSetId, LONG lSnapshotsCount);
> +    STDMETHODIMP PreFinalCommitSnapshots(VSS_ID SnapshotSetId);
> +    STDMETHODIMP PostFinalCommitSnapshots(VSS_ID SnapshotSetId);
> +    STDMETHODIMP AbortSnapshots(VSS_ID SnapshotSetId);
> +
> +    /* IVssProviderNotifications Methods */
> +    STDMETHODIMP OnLoad(IUnknown *pCallback);
> +    STDMETHODIMP OnUnload(BOOL bForceUnload);
> +
> +    /* CQGAVssProvider Methods */
> +    CQGAVssProvider();
> +    ~CQGAVssProvider();
> +
> +private:
> +    long m_nRefCount;
> +};
> +
> +CQGAVssProvider::CQGAVssProvider()
> +{
> +    m_nRefCount = 0;
> +    LockModule(TRUE);
> +}
> +
> +CQGAVssProvider::~CQGAVssProvider()
> +{
> +    LockModule(FALSE);
> +}
> +
> +STDMETHODIMP CQGAVssProvider::QueryInterface(REFIID riid, void **ppObj)
> +{
> +    if (riid == IID_IUnknown) {
> +        *ppObj = static_cast<void*>(this);
> +        AddRef();
> +        return S_OK;
> +    } else if (riid == IID_IVssSoftwareSnapshotProvider) {
> +        *ppObj = static_cast<void*>(
> +            static_cast<IVssSoftwareSnapshotProvider*>(this));
> +        AddRef();
> +        return S_OK;
> +    } else if (riid == IID_IVssProviderCreateSnapshotSet) {
> +        *ppObj = static_cast<void*>(
> +            static_cast<IVssProviderCreateSnapshotSet*>(this));
> +        AddRef();
> +        return S_OK;
> +    } else if (riid == IID_IVssProviderNotifications) {
> +        *ppObj = static_cast<void*>(
> +            static_cast<IVssProviderNotifications*>(this));
> +        AddRef();
> +        return S_OK;
> +    }
> +    *ppObj = NULL;
> +    return E_NOINTERFACE;
> +}
> +
> +STDMETHODIMP_(ULONG) CQGAVssProvider::AddRef()
> +{
> +    return InterlockedIncrement(&m_nRefCount);
> +}
> +
> +STDMETHODIMP_(ULONG) CQGAVssProvider::Release()
> +{
> +    long nRefCount = InterlockedDecrement(&m_nRefCount);
> +    if (m_nRefCount == 0) {
> +        delete this;
> +    }
> +    return nRefCount;
> +}
> +
> +
> +/*
> + * IVssSoftwareSnapshotProvider methods
> + */
> +
> +STDMETHODIMP CQGAVssProvider::SetContext(LONG lContext)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::GetSnapshotProperties(
> +    VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp)
> +{
> +    return VSS_E_OBJECT_NOT_FOUND;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::Query(
> +    VSS_ID QueriedObjectId, VSS_OBJECT_TYPE eQueriedObjectType,
> +    VSS_OBJECT_TYPE eReturnedObjectsType, IVssEnumObject **ppEnum)
> +{
> +    *ppEnum = new CQGAVSSEnumObject;
> +    (*ppEnum)->AddRef();
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::DeleteSnapshots(
> +    VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
> +    BOOL bForceDelete, LONG *plDeletedSnapshots, VSS_ID *pNondeletedSnapshotID)
> +{
> +    return E_NOTIMPL;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot(
> +    VSS_ID SnapshotSetId, VSS_ID SnapshotId,
> +    VSS_PWSZ pwszVolumeName, LONG lNewContext)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::IsVolumeSupported(
> +    VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider)
> +{
> +    *pbSupportedByThisProvider = TRUE;
> +
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::IsVolumeSnapshotted(VSS_PWSZ pwszVolumeName,
> +    BOOL *pbSnapshotsPresent, LONG *plSnapshotCompatibility)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::SetSnapshotProperty(VSS_ID SnapshotId,
> +    VSS_SNAPSHOT_PROPERTY_ID eSnapshotPropertyId, VARIANT vProperty)
> +{
> +    return E_NOTIMPL;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::RevertToSnapshot(VSS_ID SnapshotId)
> +{
> +    return E_NOTIMPL;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::QueryRevertStatus(
> +    VSS_PWSZ pwszVolume, IVssAsync **ppAsync)
> +{
> +    return S_OK;
> +}
> +
> +
> +/*
> + * IVssProviderCreateSnapshotSet methods
> + */
> +
> +STDMETHODIMP CQGAVssProvider::EndPrepareSnapshots(VSS_ID SnapshotSetId)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::PreCommitSnapshots(VSS_ID SnapshotSetId)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId)
> +{
> +    HRESULT hr = S_OK;
> +    HANDLE hEvent, hEvent2;
> +
> +    hEvent = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_FROZEN);
> +    if (hEvent == INVALID_HANDLE_VALUE) {
> +        hr = E_FAIL;
> +        goto out;
> +    }
> +
> +    hEvent2 = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_THAW);
> +    if (hEvent == INVALID_HANDLE_VALUE) {
> +        CloseHandle(hEvent);
> +        hr = E_FAIL;
> +        goto out;
> +    }
> +
> +    SetEvent(hEvent);
> +    if (WaitForSingleObject(hEvent2, 60*1000) != WAIT_OBJECT_0) {
> +        hr = E_ABORT;
> +    }
> +
> +    CloseHandle(hEvent2);
> +    CloseHandle(hEvent);
> +out:
> +    return hr;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::PostCommitSnapshots(
> +    VSS_ID SnapshotSetId, LONG lSnapshotsCount)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::PreFinalCommitSnapshots(VSS_ID SnapshotSetId)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::PostFinalCommitSnapshots(VSS_ID SnapshotSetId)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::AbortSnapshots(VSS_ID SnapshotSetId)
> +{
> +    return S_OK;
> +}
> +
> +/*
> + * IVssProviderNotifications methods
> + */
> +
> +STDMETHODIMP CQGAVssProvider::OnLoad(IUnknown *pCallback)
> +{
> +    return S_OK;
> +}
> +
> +STDMETHODIMP CQGAVssProvider::OnUnload(BOOL bForceUnload)
> +{
> +    return S_OK;
> +}
> +
> +
> +/*
> + * CQGAVssProviderFactory class
> + */
> +
> +class CQGAVssProviderFactory : public IClassFactory
> +{
> +public:
> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppv);
> +    STDMETHODIMP_(ULONG) AddRef();
> +    STDMETHODIMP_(ULONG) Release();
> +    STDMETHODIMP CreateInstance(
> +        IUnknown *pUnknownOuter, REFIID iid, void **ppv);
> +    STDMETHODIMP LockServer(BOOL bLock) { return E_NOTIMPL; }
> +private:
> +    long m_nRefCount;
> +};
> +
> +STDMETHODIMP CQGAVssProviderFactory::QueryInterface(REFIID riid, void **ppv)
> +{
> +    if (riid == IID_IUnknown || riid == IID_IClassFactory) {
> +        *ppv = static_cast<void*>(this);
> +        AddRef();
> +        return S_OK;
> +    }
> +    *ppv = NULL;
> +    return E_NOINTERFACE;
> +}
> +
> +STDMETHODIMP_(ULONG) CQGAVssProviderFactory::AddRef()
> +{
> +    LockModule(TRUE);
> +    return 2;
> +}
> +
> +STDMETHODIMP_(ULONG) CQGAVssProviderFactory::Release()
> +{
> +    LockModule(FALSE);
> +    return 1;
> +}
> +
> +STDMETHODIMP CQGAVssProviderFactory::CreateInstance(
> +    IUnknown *pUnknownOuter, REFIID iid, void **ppv)
> +{
> +    if (pUnknownOuter) {
> +        return CLASS_E_NOAGGREGATION;
> +    }
> +    CQGAVssProvider *pObj = new CQGAVssProvider;
> +    if (!pObj) {
> +        return E_OUTOFMEMORY;
> +    }
> +    return pObj->QueryInterface(iid, ppv);
> +}
> +
> +
> +/*
> + * DLL functions
> + */
> +
> +STDAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID *ppv)
> +{
> +    static CQGAVssProviderFactory factory;
> +
> +    *ppv = NULL;
> +    if (IsEqualCLSID(rclsid, CLSID_QGAVSSProvider)) {
> +        return factory.QueryInterface(riid, ppv);
> +    }
> +    return CLASS_E_CLASSNOTAVAILABLE;
> +}
> +
> +STDAPI DllCanUnloadNow()
> +{
> +    return g_nComObjsInUse == 0 ? S_OK : S_FALSE;
> +}
> +
> +EXTERN_C
> +BOOL WINAPI DllMain(HINSTANCE hinstDll, DWORD dwReason, LPVOID lpReserved)
> +{
> +    switch (dwReason) {
> +
> +    case DLL_PROCESS_ATTACH:
> +        g_hinstDll = hinstDll;
> +        DisableThreadLibraryCalls(hinstDll);
> +        break;
> +    }
> +
> +    return TRUE;
> +}

> diff --git a/qga/vss-win32-provider/qga-provider.def b/qga/vss-win32-provider/qga-provider.def
> new file mode 100644
> index 0000000..9f3afc8
> --- /dev/null
> +++ b/qga/vss-win32-provider/qga-provider.def
> @@ -0,0 +1,10 @@
> +LIBRARY      "QGA-PROVIDER.DLL"
> +
> +EXPORTS
> +	VSSCheckOSVersion	PRIVATE
> +	COMRegister		PRIVATE
> +	COMUnregister		PRIVATE
> +	DllCanUnloadNow		PRIVATE
> +	DllGetClassObject	PRIVATE
> +	DllRegisterServer	PRIVATE
> +	DllUnregisterServer	PRIVATE

> diff --git a/qga/vss-win32-provider/qga-provider.idl b/qga/vss-win32-provider/qga-provider.idl
> new file mode 100644
> index 0000000..17abca0
> --- /dev/null
> +++ b/qga/vss-win32-provider/qga-provider.idl
> @@ -0,0 +1,20 @@
> +import "oaidl.idl";
> +import "ocidl.idl";
> +
> +[
> +    uuid(103B8142-6CE5-48A7-BDE1-794D3192FCF1),
> +    version(1.0),
> +    helpstring("QGAVSSProvider Type Library")
> +]
> +library QGAVSSHWProviderLib
> +{
> +    importlib("stdole2.tlb");
> +    [
> +        uuid(6E6A3492-8D4D-440C-9619-5E5D0CC31CA8),
> +        helpstring("QGAVSSProvider Class")
> +    ]
> +    coclass QGAVSSHWProvider
> +    {
> +        [default] interface IUnknown;
> +    };
> +};

> diff --git a/qga/vss-win32-provider/qga-provider.tlb b/qga/vss-win32-provider/qga-provider.tlb
> new file mode 100644
> index 0000000000000000000000000000000000000000..226452a1861371ffe0cad1019cf90fdfdcd5ef49
> GIT binary patch
> literal 1528
> zcmeYbb_-!*U}OLRP8Kl5;0UB3A_y8H!@$4<WGF*9|A9aP$W{R21|SCUVfs9Pj1;IC
> zKahR`)X0Ox{{ZC6An~sN`2tA%H9-9hNPHcj{0byK4>OPh6a(1_GM|T)fx!kz-UG<D
> zK;nbc0l9GX===tt`Vb`fD?q*q5+7YXI$u?Zf#C;G4-9~uhYKVCC4f!`hZ{(Z0*HVD
> zkhwswGqAt}ki;hd*$F@lQbP%-z+r|5P#hGW*vvKniaRx03p~wP?y>h_rLW<nKOg@=
> z6{hYgzgH7@QE<^MU>KC!yoBjb#vz`9Lwu4+R-SJ!kIOX4xLBUUGN9-NyTyP76j}@n
> z2f!qQ8-xepfXD+7rW+{SKz4&@7#qX~^1#sn3O|tFK>%ciE<<riN`6kNkzPqoQh0bc
> zNbM*XJ|Un0jALSb15^rEE6h+Y8tCpA798vm9#E8DmYI@T<dc~c4pSpwQKEn@FU<fE
> zfvHyrsVqoU0O~4AEUE;iEfI8i=bXgi;_z?|20Ng!&PAz-C8;S2NtFt|o-RHLWvNBQ
> znfZAN=6VJOdIqMZrV5EA3T{Q23NES13Py$shQ?OLW>&_Q3PuKoMqI)S5zj9Ngog_=
> gXfrXehlhjmFbIJB4$8MKU>*YlD1Z9^F{m5{03Vre%>V!Z
>
> literal 0
> HcmV?d00001
>

> diff --git a/qga/vss-win32.h b/qga/vss-win32.h
> new file mode 100644
> index 0000000..21655cf
> --- /dev/null
> +++ b/qga/vss-win32.h
> @@ -0,0 +1,86 @@
> +/*
> + * QEMU Guest Agent win32 VSS common declarations
> + *
> + * Copyright Hitachi Data Systems Corp. 2013
> + *
> + * Authors:
> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
> + *
> + * 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 */
> +
> +/* Ignore annotations for MS IDE */
> +#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)
> +
> +/* Undef some duplicated error codes redefined in vss.h */
> +#undef VSS_E_BAD_STATE
> +#undef VSS_E_PROVIDER_NOT_REGISTERED
> +#undef VSS_E_PROVIDER_VETO
> +#undef VSS_E_OBJECT_NOT_FOUND
> +#undef VSS_E_VOLUME_NOT_SUPPORTED
> +#undef VSS_E_VOLUME_NOT_SUPPORTED_BY_PROVIDER
> +#undef VSS_E_OBJECT_ALREADY_EXISTS
> +#undef VSS_E_UNEXPECTED_PROVIDER_ERROR
> +#undef VSS_E_INVALID_XML_DOCUMENT
> +#undef VSS_E_MAXIMUM_NUMBER_OF_VOLUMES_REACHED
> +#undef VSS_E_MAXIMUM_NUMBER_OF_SNAPSHOTS_REACHED
> +
> +/*
> + * VSS headers must be installed from Microsoft VSS SDK 7.2 available at:
> + * http://www.microsoft.com/en-us/download/details.aspx?id=23490
> + */
> +#include "inc/win2003/vss.h"
> +
> +/* Macros to convert char definitions to wchar */
> +#define _L(a) L##a
> +#define L(a) _L(a)
> +
> +/* Constants for QGA VSS Provider */
> +
> +#define QGA_PROVIDER_NAME "QEMU Guest Agent VSS Provider"
> +#define QGA_PROVIDER_LNAME L(QGA_PROVIDER_NAME)
> +#define QGA_PROVIDER_VERSION L(QEMU_VERSION)
> +
> +#define EVENT_NAME_FROZEN "Global\\QGAVSSEvent-frozen"
> +#define EVENT_NAME_THAW   "Global\\QGAVSSEvent-thaw"
> +
> +const GUID g_gProviderId = { 0x3629d4ed, 0xee09, 0x4e0e,
> +    {0x9a, 0x5c, 0x6d, 0x8b, 0xa2, 0x87, 0x2a, 0xef} };
> +const GUID g_gProviderVersion = { 0x11ef8b15, 0xcac6, 0x40d6,
> +    {0x8d, 0x5c, 0x8f, 0xfc, 0x16, 0x3f, 0x24, 0xca} };
> +
> +const CLSID CLSID_QGAVSSProvider = { 0x6e6a3492, 0x8d4d, 0x440c,
> +    {0x96, 0x19, 0x5e, 0x5d, 0x0c, 0xc3, 0x1c, 0xa8} };
> +
> +const TCHAR g_szClsid[] = TEXT("{6E6A3492-8D4D-440C-9619-5E5D0CC31CA8}");
> +const TCHAR g_szProgid[] = TEXT("QGAVSSProvider");
> +
> +/* Enums undefined in VSS SDK 7.2 but defined in newer Windows SDK */
> +enum __VSS_VOLUME_SNAPSHOT_ATTRIBUTES {
> +    VSS_VOLSNAP_ATTR_NO_AUTORECOVERY       = 0x00000002,
> +    VSS_VOLSNAP_ATTR_TXF_RECOVERY          = 0x02000000
> +};
> +
> +#endif
>
>
Paolo Bonzini - June 25, 2013, 4:19 p.m.
Il 25/06/2013 18:03, Laszlo Ersek ha scritto:
>> > @@ -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'"?
> 
> Or does it mean "look into qga/Makefile.objs for further objects"?

The latter.  I'd rather have it spelled fully, though
(qga-win32-provider-obj-y).

> > +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.

> 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?

No, you need to specify it at all levels.

> Why does "qga-provider.dll" depend on ""qga-provider.tlb"? It is not
> used in the link command.
> 
> 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"?

I think dynamic linking is needed because VSS is loading the provider as
a COM component here:

+            hr = pVssbc->AddToSnapshotSet(buf, g_gProviderId, &pid);

But I don't know why you need to link the DLL to qemu-ga.exe.

Paolo
Tomoki Sekiyama - June 25, 2013, 6:23 p.m.
Thanks for your review.

On 6/25/13 12:03 , "Laszlo Ersek" <lersek@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
Paolo Bonzini - June 25, 2013, 6:59 p.m.
Il 25/06/2013 20:23, Tomoki Sekiyama ha scritto:
>> >(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.
> 

Does the DLL also support DllRegisterServer?  I don't know Windows very
much, but if so I wonder if LoadLibrary/GetProcAddress is preferrable to
linking with the DLL.

Paolo
Tomoki Sekiyama - June 25, 2013, 7:28 p.m.
>Il 25/06/2013 20:23, Tomoki Sekiyama ha scritto:
>>> >(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.
> 
> Does the DLL also support DllRegisterServer?  I don't know Windows very
> much, but if so I wonder if LoadLibrary/GetProcAddress is preferrable to
> linking with the DLL.
> 
> Paolo

I think that is feasible.

That will cut off qemu-ga.exe from qga-provider.{dll,tlb}, so then,
we should build them separately, like
`make qemu-ga.exe qga/vss-win32-provider/qga-provider.{dll,tlb}`.

Thanks,
Tomoki Sekiyama
Laszlo Ersek - June 25, 2013, 7:52 p.m.
On 06/25/13 20:23, Tomoki Sekiyama wrote:
> On 6/25/13 12:03 , "Laszlo Ersek" <lersek@redhat.com> wrote:

>>> +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.

Ah sorry. "ack" (that I sometimes use for recursive grepping, outside of
git clones) didn't return any hits in the directory where I had saved
your patch emails (for git-am), that's why I said "no references in the
series".

Perhaps "ack" ignored the .eml files due to their CRLF line endings...
It did find the references with option "--all".

(Of course I should have noticed something was fishy, since "ack" didn't
find the very occurrence I was questioning! :))

> But as you say, I will omit this, by the second reason.

OK, thanks.

>> 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.

[...]

>> ... 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.

Very interesting. I'm curious about the requestor in the next patch.

Based on the VSS description in MSDN, I thought the requestor would make
some kind of blocking call (like a normal function call) into VSS that
could not return until the full process was done (ie. filesystems were
re-thawed).

This likely isn't the case, as CommitSnapshots() appears to signal
(wake) qemu-ga.exe asynchronously -- the requestor (qemu-ga) and the
provider (the DLL in the VSS component) seem to run in parallel.

The more I think I understand of your design the more I like it.

I'll attempt to continue the review tomorrow.

Thanks!
Laszlo
Paolo Bonzini - June 25, 2013, 9:15 p.m.
Il 06/06/2013 17:06, Tomoki Sekiyama ha scritto:
> +STDAPI VSSCheckOSVersion(void);
> +
> +STDAPI COMRegister(void);
> +STDAPI COMUnregister(void);
> +
> +STDAPI DllRegisterServer(void);
> +STDAPI DllUnregisterServer(void);

Can you explain the difference between COMRegister/COMUnregister and
DllRegisterServer/DllUnregisterServer (and why the COM+ part need not be
done by regsvr32)?  Also, why does COMUnregister call
DllUnregisterServer but COMRegister does not call DllRegisterServer?

Also, is it needed to call VSSCheckOSVersion from the requestor?  I
would think that checking VSSAPI.DLL is stronger than checking the
version, and indeed you do that check too.

Sorry for the probably stupid questions, COM stuff is way beyond my
Windows knowledge.

Paolo
Tomoki Sekiyama - June 25, 2013, 10:31 p.m.
From: Paolo Bonzini [paolo.bonzini@gmail.com] on behalf of Paolo Bonzini [pbonzini@redhat.com]
> Il 06/06/2013 17:06, Tomoki Sekiyama ha scritto:
>> +STDAPI VSSCheckOSVersion(void);
>> +
>> +STDAPI COMRegister(void);
>> +STDAPI COMUnregister(void);
>> +
>> +STDAPI DllRegisterServer(void);
>> +STDAPI DllUnregisterServer(void);
>
> Can you explain the difference between COMRegister/COMUnregister and
> DllRegisterServer/DllUnregisterServer (and why the COM+ part need not be
> done by regsvr32)?  Also, why does COMUnregister call
> DllUnregisterServer but COMRegister does not call DllRegisterServer?

COMRegister and COMUnregister are called by`qemu-ga -s install`,
to register/unregister the DLL into/from COM+ application catalogue.

DllRegisterServer is automatically called inside
pCatalog->InstallComponent() (like regsvr32 does), and register
this DLL as a VSS provider. DllUnregisterServer will do the oposite.

ICOMAdminCatalog (pCatalog) does not provide a method to uninstall
component, so COMUnregister calls DllUnregisterServer by itself.

> Also, is it needed to call VSSCheckOSVersion from the requestor?  I
> would think that checking VSSAPI.DLL is stronger than checking the
> version, and indeed you do that check too.

In Windows XP, VSSAPI.DLL exists but it has different functionality
and interfaces from newer Windows. 
http://msdn.microsoft.com/en-us/library/windows/desktop/aa384627(v=vs.85).aspx

It is checking the OS version because this patchset only supports
Windows 2003 or later.

Thanks,
Tomoki Sekiyama
Paolo Bonzini - June 26, 2013, 5:59 a.m.
Il 26/06/2013 00:31, Tomoki Sekiyama ha scritto:
> From: Paolo Bonzini [paolo.bonzini@gmail.com] on behalf of Paolo Bonzini [pbonzini@redhat.com]
>> Il 06/06/2013 17:06, Tomoki Sekiyama ha scritto:
>>> +STDAPI VSSCheckOSVersion(void);
>>> +
>>> +STDAPI COMRegister(void);
>>> +STDAPI COMUnregister(void);
>>> +
>>> +STDAPI DllRegisterServer(void);
>>> +STDAPI DllUnregisterServer(void);
>>
>> Can you explain the difference between COMRegister/COMUnregister and
>> DllRegisterServer/DllUnregisterServer (and why the COM+ part need not be
>> done by regsvr32)?  Also, why does COMUnregister call
>> DllUnregisterServer but COMRegister does not call DllRegisterServer?
> 
> COMRegister and COMUnregister are called by`qemu-ga -s install`,
> to register/unregister the DLL into/from COM+ application catalogue.
> 
> DllRegisterServer is automatically called inside
> pCatalog->InstallComponent() (like regsvr32 does), and register
> this DLL as a VSS provider. DllUnregisterServer will do the oposite.
> 
> ICOMAdminCatalog (pCatalog) does not provide a method to uninstall
> component, so COMUnregister calls DllUnregisterServer by itself.

Understood, thanks.  Just one question remains: why is the COM+ part not
needed when you invoke regsvr32?

Thanks,

Paolo

>> Also, is it needed to call VSSCheckOSVersion from the requestor?  I
>> would think that checking VSSAPI.DLL is stronger than checking the
>> version, and indeed you do that check too.
> 
> In Windows XP, VSSAPI.DLL exists but it has different functionality
> and interfaces from newer Windows. 
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa384627(v=vs.85).aspx
> 
> It is checking the OS version because this patchset only supports
> Windows 2003 or later.
> 
> Thanks,
> Tomoki Sekiyama
>
Tomoki Sekiyama - June 26, 2013, 2:13 p.m.
On 6/26/13 1:59 , "Paolo Bonzini" <pbonzini@redhat.com> wrote:

>Il 26/06/2013 00:31, Tomoki Sekiyama ha scritto:
>> From: Paolo Bonzini [paolo.bonzini@gmail.com] on behalf of Paolo
>>Bonzini [pbonzini@redhat.com]
>>> Il 06/06/2013 17:06, Tomoki Sekiyama ha scritto:
>>>> +STDAPI VSSCheckOSVersion(void);
>>>> +
>>>> +STDAPI COMRegister(void);
>>>> +STDAPI COMUnregister(void);
>>>> +
>>>> +STDAPI DllRegisterServer(void);
>>>> +STDAPI DllUnregisterServer(void);
>>>
>>> Can you explain the difference between COMRegister/COMUnregister and
>>> DllRegisterServer/DllUnregisterServer (and why the COM+ part need not
>>>be
>>> done by regsvr32)?  Also, why does COMUnregister call
>>> DllUnregisterServer but COMRegister does not call DllRegisterServer?
>> 
>> COMRegister and COMUnregister are called by`qemu-ga -s install`,
>> to register/unregister the DLL into/from COM+ application catalogue.
>> 
>> DllRegisterServer is automatically called inside
>> pCatalog->InstallComponent() (like regsvr32 does), and register
>> this DLL as a VSS provider. DllUnregisterServer will do the oposite.
>> 
>> ICOMAdminCatalog (pCatalog) does not provide a method to uninstall
>> component, so COMUnregister calls DllUnregisterServer by itself.
>
>Understood, thanks.  Just one question remains: why is the COM+ part not
>needed when you invoke regsvr32?

A VSS Provider is implemented as a COM+ application.

To register COM+ applications, regsvr32 is not enough.
It only register COM component, which is a part of COM+ application.
We need to use a specialized installer (COMRegister() in this series)
to the whole COM+ application to the catalogue.

Thanks,
Tomoki Sekiyama
Laszlo Ersek - June 26, 2013, 2:32 p.m.
On 06/25/13 18:03, Laszlo Ersek wrote:
> On 06/06/13 17:06, Tomoki Sekiyama wrote:

Comparing the .def file and the provider header file:

>> diff --git a/qga/vss-win32-provider/qga-provider.def b/qga/vss-win32-provider/qga-provider.def
>> new file mode 100644
>> index 0000000..9f3afc8
>> --- /dev/null
>> +++ b/qga/vss-win32-provider/qga-provider.def
>> @@ -0,0 +1,10 @@
>> +LIBRARY      "QGA-PROVIDER.DLL"
>> +
>> +EXPORTS
>> +	VSSCheckOSVersion	PRIVATE
>> +	COMRegister		PRIVATE
>> +	COMUnregister		PRIVATE
>> +	DllCanUnloadNow		PRIVATE
>> +	DllGetClassObject	PRIVATE
>> +	DllRegisterServer	PRIVATE
>> +	DllUnregisterServer	PRIVATE

>> diff --git a/qga/vss-win32-provider.h b/qga/vss-win32-provider.h
>> new file mode 100644
>> index 0000000..a437e71
>> --- /dev/null
>> +++ b/qga/vss-win32-provider.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * QEMU Guest Agent win32 VSS provider declarations
>> + *
>> + * Copyright Hitachi Data Systems Corp. 2013
>> + *
>> + * Authors:
>> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
>> + *
>> + * 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_PROVIDER_H
>> +#define VSS_WIN32_PROVIDER_H
>> +
>> +#include <windows.h>
>> +
>> +STDAPI VSSCheckOSVersion(void);
>> +
>> +STDAPI COMRegister(void);
>> +STDAPI COMUnregister(void);
>> +
>> +STDAPI DllRegisterServer(void);
>> +STDAPI DllUnregisterServer(void);
>> +
>> +#endif

(a) what is the reason for not listing DllCanUnloadNow() and
DllGetClassObject() in the header file?

(b) Does STDAPI imply a return type? Or are you just going with the
implicit "int"? (Based on my encounters with EFIAPI, I think it's the
latter.)


>> diff --git a/qga/vss-win32.h b/qga/vss-win32.h
>> new file mode 100644
>> index 0000000..21655cf
>> --- /dev/null
>> +++ b/qga/vss-win32.h
>> @@ -0,0 +1,86 @@
>> +/*
>> + * QEMU Guest Agent win32 VSS common declarations
>> + *
>> + * Copyright Hitachi Data Systems Corp. 2013
>> + *
>> + * Authors:
>> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
>> + *
>> + * 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 */
>> +
>> +/* Ignore annotations for MS IDE */
>> +#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)
>> +
>> +/* Undef some duplicated error codes redefined in vss.h */
>> +#undef VSS_E_BAD_STATE
>> +#undef VSS_E_PROVIDER_NOT_REGISTERED
>> +#undef VSS_E_PROVIDER_VETO
>> +#undef VSS_E_OBJECT_NOT_FOUND
>> +#undef VSS_E_VOLUME_NOT_SUPPORTED
>> +#undef VSS_E_VOLUME_NOT_SUPPORTED_BY_PROVIDER
>> +#undef VSS_E_OBJECT_ALREADY_EXISTS
>> +#undef VSS_E_UNEXPECTED_PROVIDER_ERROR
>> +#undef VSS_E_INVALID_XML_DOCUMENT
>> +#undef VSS_E_MAXIMUM_NUMBER_OF_VOLUMES_REACHED
>> +#undef VSS_E_MAXIMUM_NUMBER_OF_SNAPSHOTS_REACHED
>> +
>> +/*
>> + * VSS headers must be installed from Microsoft VSS SDK 7.2 available at:
>> + * http://www.microsoft.com/en-us/download/details.aspx?id=23490
>> + */
>> +#include "inc/win2003/vss.h"
>> +
>> +/* Macros to convert char definitions to wchar */
>> +#define _L(a) L##a
>> +#define L(a) _L(a)

(Sigh. The more windows code I'm looking at the more it convinces me
that EFI was 100% meant for windows originally. Sad.)


>> +
>> +/* Constants for QGA VSS Provider */
>> +
>> +#define QGA_PROVIDER_NAME "QEMU Guest Agent VSS Provider"
>> +#define QGA_PROVIDER_LNAME L(QGA_PROVIDER_NAME)
>> +#define QGA_PROVIDER_VERSION L(QEMU_VERSION)
>> +
>> +#define EVENT_NAME_FROZEN "Global\\QGAVSSEvent-frozen"
>> +#define EVENT_NAME_THAW   "Global\\QGAVSSEvent-thaw"

No idea what I'm talking about, but since we're qualifying the second
component with the "QGAVSSEvent" prefix, shouldn't we just replace the
first component ("Global") with it? Or would it effect "event routing"?
(Tangential question, anyway.)


>> +
>> +const GUID g_gProviderId = { 0x3629d4ed, 0xee09, 0x4e0e,
>> +    {0x9a, 0x5c, 0x6d, 0x8b, 0xa2, 0x87, 0x2a, 0xef} };
>> +const GUID g_gProviderVersion = { 0x11ef8b15, 0xcac6, 0x40d6,
>> +    {0x8d, 0x5c, 0x8f, 0xfc, 0x16, 0x3f, 0x24, 0xca} };
>> +
>> +const CLSID CLSID_QGAVSSProvider = { 0x6e6a3492, 0x8d4d, 0x440c,
>> +    {0x96, 0x19, 0x5e, 0x5d, 0x0c, 0xc3, 0x1c, 0xa8} };
>> +
>> +const TCHAR g_szClsid[] = TEXT("{6E6A3492-8D4D-440C-9619-5E5D0CC31CA8}");

OK, these match each other and "qga-provider.idl".


>> +const TCHAR g_szProgid[] = TEXT("QGAVSSProvider");
>> +
>> +/* Enums undefined in VSS SDK 7.2 but defined in newer Windows SDK */
>> +enum __VSS_VOLUME_SNAPSHOT_ATTRIBUTES {
>> +    VSS_VOLSNAP_ATTR_NO_AUTORECOVERY       = 0x00000002,
>> +    VSS_VOLSNAP_ATTR_TXF_RECOVERY          = 0x02000000
>> +};
>> +
>> +#endif
>>
>>

OK.


>> diff --git a/qga/vss-win32-provider/install.cpp b/qga/vss-win32-provider/install.cpp
>> new file mode 100644
>> index 0000000..d6f1d55
>> --- /dev/null
>> +++ b/qga/vss-win32-provider/install.cpp
>> @@ -0,0 +1,493 @@
>> +/*
>> + * QEMU Guest Agent win32 VSS Provider installer
>> + *
>> + * Copyright Hitachi Data Systems Corp. 2013
>> + *
>> + * Authors:
>> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <string.h>
>> +
>> +#include "../vss-win32.h"
>> +#include "inc/win2003/vscoordint.h"
>> +#include "../vss-win32-provider.h"
>> +
>> +#include <comadmin.h>
>> +#include <wbemidl.h>
>> +#include <comutil.h>
>> +
>> +extern HINSTANCE g_hinstDll;
>> +
>> +const GUID CLSID_COMAdminCatalog = { 0xF618C514, 0xDFB8, 0x11d1,
>> +    {0xA2, 0xCF, 0x00, 0x80, 0x5F, 0xC7, 0x92, 0x35} };
>> +const GUID IID_ICOMAdminCatalog = { 0xDD662187, 0xDFC2, 0x11d1,
>> +    {0xA2, 0xCF, 0x00, 0x80, 0x5F, 0xC7, 0x92, 0x35} };
>> +const GUID CLSID_WbemLocator = { 0x4590f811, 0x1d3a, 0x11d0,
>> +    {0x89, 0x1f, 0x00, 0xaa, 0x00, 0x4b, 0x2e, 0x24} };
>> +const GUID IID_IWbemLocator = { 0xdc12a687, 0x737f, 0x11cf,
>> +    {0x88, 0x4d, 0x00, 0xaa, 0x00, 0x4b, 0x2e, 0x24} };

Would it be possible to make these static? Not too important of course.


>> +
>> +static void errmsg(DWORD err, const char *text)
>> +{
>> +    char *msg = NULL, *nul = strchr(text, '(');
>> +    int len = nul ? nul - text : -1;
>> +
>> +    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
>> +                  FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
>> +                  NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
>> +                  (char *)&msg, 0, NULL);

(The FormatMessage() specification in MSDN is simply incredible.
"(char *)&msg", seriously? Your code is correct, but the interface is
unbelievable.)


>> +    printf("%.*s. (Error: %lx) %s\n", len, text, err, msg);

(a) Since we're printing an error message here, I suspect it should go
to stderr.

(b) In case "text" doesn't contain an opening paren, "len" is set to -1.
"A negative precision is taken as if the precision were omitted", IOW
the entire string will be printed. Clever!

(c) Does DWORD expand to "uint32_t"? In that case "%lx" is too big on
64-bit (LP64). We should probably use "%x" or "%"PRIx32. (Or do we build
qemu-ga.exe only for 32-bit? I seem to remember something like that.)


>> +    LocalFree(msg);
>> +}
>> +
>> +static void errmsg_dialog(DWORD err, const char *text, const char *opt = "")
>> +{
>> +    char *msg, buf[512];
>> +
>> +    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
>> +                  FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
>> +                  NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
>> +                  (char *)&msg, 0, NULL);
>> +    snprintf(buf, sizeof(buf), "%s%s. (Error: %lx) %s\n", text, opt, err, msg);
>> +    MessageBox(NULL, buf, "Error from " QGA_PROVIDER_NAME, MB_OK|MB_ICONERROR);

Do you need a trailing newline for a message box?


>> +    LocalFree(msg);
>> +}
>> +
>> +#define _chk(hr, status, msg, err_label)        \
>> +    do {                                        \
>> +        hr = (status);                          \
>> +        if (FAILED(hr)) {                       \
>> +            errmsg(hr, msg);                    \
>> +            goto err_label;                     \
>> +        }                                       \
>> +    } while (0)
>> +
>> +#define chk(status) _chk(hr, status, "Failed to " #status, out)
>> +
>> +template<class T>
>> +HRESULT put_Value(ICatalogObject *pObj, LPCWSTR name, T val)
>> +{
>> +    return pObj->put_Value(_bstr_t(name), _variant_t(val));
>> +}
>> +
>> +/* Check whether this OS version supports VSS providers */
>> +STDAPI VSSCheckOSVersion(void)
>> +{
>> +    OSVERSIONINFO OSver;
>> +
>> +    OSver.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
>> +    GetVersionEx(&OSver);
>> +    if ((OSver.dwMajorVersion == 5 && OSver.dwMinorVersion >= 2) ||
>> +       OSver.dwMajorVersion > 5) {
>> +        return S_OK;
>> +    }
>> +    return S_FALSE;
>> +}
>> +

Seems OK.


>> +/* Lookup Administrators group name from winmgmt */
>> +static HRESULT GetAdminName(_bstr_t &name)
>> +{

I can see that you want to call GetAdminName() in chk(), hence you must
return a HRESULT and store the value through the parameter list.
However, can we keep the C++ features to a minimum? I'd prefer if the
non-const reference were replaced with a pointer-to-_bstr_t.
(Tangential, change only if you respin anyway and don't mind it.)


>> +    HRESULT hr;
>> +    IWbemLocator *pLoc = NULL;
>> +    IWbemServices *pSvc = NULL;
>> +    IEnumWbemClassObject *pEnum = NULL;
>> +    IWbemClassObject *pWobj = NULL;
>> +    ULONG returned;
>> +    _variant_t var;
>> +
>> +    chk(CoCreateInstance(CLSID_WbemLocator, NULL, CLSCTX_INPROC_SERVER,
>> +                         IID_IWbemLocator, (LPVOID *)&pLoc));
>> +    chk(pLoc->ConnectServer(_bstr_t(L"ROOT\\CIMV2"), NULL, NULL, NULL,
>> +                            0, 0, 0, &pSvc));
>> +    chk(CoSetProxyBlanket(pSvc, RPC_C_AUTHN_WINNT, RPC_C_AUTHZ_NONE,
>> +                          NULL, RPC_C_AUTHN_LEVEL_CALL,
>> +                          RPC_C_IMP_LEVEL_IMPERSONATE, NULL, EOAC_NONE));
>> +    chk(pSvc->ExecQuery(_bstr_t(L"WQL"),
>> +                        _bstr_t(L"select * from Win32_Account where "
>> +                                "SID='S-1-5-32-544' and localAccount=TRUE"),
>> +                        WBEM_FLAG_RETURN_IMMEDIATELY | WBEM_FLAG_FORWARD_ONLY,
>> +                        NULL, &pEnum));

(signal to noise ratio converging to zero... :) I don't doubt this is
the official way though.)

Furthermore, aren't you missing a CoInitialize(NULL) call before
CoCreateInstance()? Other CoCreateInstance() calls are preceded by it.

... Well this is called only from COMRegister(), after such an init
call, so it should be OK.


>> +    if (!pEnum) {
>> +        errmsg(E_FAIL, "Failed to query for Administrators");
>> +        goto out;

I think you forgot to set "hr" to something ugly -- the code below the
"out" label will simply return it, and "hr" is currently something nice.
(The last chk() succeeded.)


>> +    }

This branch is probably for the case when ExecQuery() succeeds, but
doesn't return any rows, right?


>> +    chk(pEnum->Next(WBEM_INFINITE, 1, &pWobj, &returned));
>> +    if (returned == 0) {
>> +        errmsg(E_FAIL, "Failed to query for Administrators");
>> +        goto out;
>> +    }
>> +
>> +    chk(pWobj->Get(_bstr_t(L"Name"), 0, &var, 0, 0));
>> +    name = var;

This assigns a _variant_t to a _bstr_t (... for which I proposed *name =
var above, but it's irrelevant now).

Can this conversion (or the assignment operator) fail? If so, does
either throw an exception?


>> +out:
>> +    if (pLoc) {
>> +        pLoc->Release();
>> +    }
>> +    if (pSvc) {
>> +        pSvc->Release();
>> +    }
>> +    if (pEnum) {
>> +        pEnum->Release();
>> +    }
>> +    if (pWobj) {
>> +        pWobj->Release();
>> +    }
>> +    return hr;
>> +}

I'm sure this is idiomatic code, but do these pointers point to static /
long-term instances? I'm missing the equivalent of "delete pLoc" and so
on. Each Release() member function gets the implicit "this" parameter
here, but do they free it too? (Honestly, I can't decide which would be
worse, if they did or they didn't.)

... Ah, it's probably reference counting. OK.


>> +
>> +/* Register this module to COM+ Applications Catalog */
>> +STDAPI COMRegister(void)
>> +{
>> +    HRESULT hr = E_FAIL;
>> +    IUnknown *pUnknown = NULL;
>> +    ICOMAdminCatalog *pCatalog = NULL;
>> +    ICatalogCollection *pApps = NULL, *pRoles = NULL, *pUsersInRole = NULL;
>> +    ICatalogObject *pObj = NULL;
>> +    long n;
>> +    _bstr_t name;
>> +    _variant_t key;
>> +    CHAR dllPath[MAX_PATH], tlbPath[MAX_PATH];
>> +
>> +    if (!g_hinstDll) {
>> +        errmsg(E_FAIL, "Failed to initialize DLL");
>> +        goto out;

OK, "hr" is E_FAIL here.


>> +    }
>> +
>> +    if (VSSCheckOSVersion() == S_FALSE) {
>> +        printf("VSS provider is not supported in this OS version.\n");

I suggest stderr.


>> +        return S_FALSE; /* VSS feature is disabled */

Somewhat inconsistent with the above, you issue "return" although you
used "goto out" before. No resources are leaked so it's no problem in
practice. (You could have written "return" above.)


>> +    }
>> +
>> +    COMUnregister();

What requires / justifies this? (I can see another call below, on the
error path.) I think whatever COMUnregister() would release depends
first on the success of COMRegister(), doesn't it?

You've told Paolo,

On 06/26/13 00:31, Tomoki Sekiyama wrote:
> COMRegister and COMUnregister are called by`qemu-ga -s install`,
> to register/unregister the DLL into/from COM+ application catalogue.

and indeed I can see a COMRegister() call in 09/10. Nonetheless that
doesn't seem to justify this call to COMUnregister() -- if the DLL has
been previously registered in the application catalog, a new
registration attempt should fail.


>> +
>> +    chk(CoInitialize(NULL));

Can this fail in practice? If so, we could have error handling trouble,
because "out:" will call CoUninitialize() without any successful
initialization.


>> +    chk(CoCreateInstance(CLSID_COMAdminCatalog, NULL, CLSCTX_INPROC_SERVER,
>> +                         IID_IUnknown, (void **)&pUnknown));
>> +    chk(pUnknown->QueryInterface(IID_ICOMAdminCatalog, (void **)&pCatalog));
>> +
>> +    /* Install COM+ Component */
>> +
>> +    chk(pCatalog->GetCollection(_bstr_t(L"Applications"),
>> +                                (IDispatch **)&pApps));
>> +    chk(pApps->Populate());
>> +    chk(pApps->Add((IDispatch **)&pObj));
>> +    chk(put_Value(pObj, L"Name",        QGA_PROVIDER_LNAME));
>> +    chk(put_Value(pObj, L"Description", QGA_PROVIDER_LNAME));
>> +    chk(put_Value(pObj, L"ApplicationAccessChecksEnabled", true));
>> +    chk(put_Value(pObj, L"Authentication",                 short(6)));
>> +    chk(put_Value(pObj, L"AuthenticationCapability",       short(2)));
>> +    chk(put_Value(pObj, L"ImpersonationLevel",             short(2)));
>> +    chk(pApps->SaveChanges(&n));

This looks like a globally visible change that is not rolled back if
something fails later in this function. Is the COMUnregister() call on
the error handling path supposed to undo these? Complicated :)

Then, should we check "n", or does the retval check suffice?


>> +    chk(pObj->get_Key(&key));
>> +
>> +    pObj->Release();
>> +    pObj = NULL;

Ah OK. You're reusing pObj below.


>> +
>> +    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
>> +        errmsg(GetLastError(), "GetModuleFileName failed");
>> +        goto out;

"hr" has not been set to a failure code.


>> +    }
>> +    n = strlen(dllPath);
>> +    if (n < 3) {
>> +        errmsg(E_FAIL, "Failed to lookup dll");

Presumably you wanted to set "hr" and jump to "out" as well.


>> +    }
>> +    strcpy(tlbPath, dllPath);
>> +    strcpy(tlbPath+n-3, "TLB");
>> +    printf("Registering " QGA_PROVIDER_NAME ":\n");
>> +    printf("  %s\n", dllPath);
>> +    printf("  %s\n", tlbPath);

These are arguably diagnostic messages (destined for stderr), but I
won't press it :)


>> +    if (!PathFileExists(tlbPath)) {
>> +        errmsg(ERROR_FILE_NOT_FOUND, "Failed to lookup tlb");
>> +        goto out;

"hr" not set properly.


>> +    }
>> +
>> +    chk(pCatalog->InstallComponent(_bstr_t(QGA_PROVIDER_LNAME),
>> +                                   _bstr_t(dllPath), _bstr_t(tlbPath),
>> +                                   _bstr_t("")));
>> +
>> +    /* Setup roles of the applicaion */
>> +
>> +    chk(pApps->GetCollection(_bstr_t(L"Roles"), key,
>> +                             (IDispatch **)&pRoles));
>> +    chk(pRoles->Populate());
>> +    chk(pRoles->Add((IDispatch **)&pObj));
>> +    chk(put_Value(pObj, L"Name",        L"Administrators"));
>> +    chk(put_Value(pObj, L"Description", L"Administrators group"));
>> +    chk(pRoles->SaveChanges(&n));
>> +    chk(pObj->get_Key(&key));
>> +
>> +    pObj->Release();
>> +    pObj = NULL;
>> +
>> +    /* Setup users in the role */
>> +
>> +    chk(pRoles->GetCollection(_bstr_t(L"UsersInRole"), key,
>> +                              (IDispatch **)&pUsersInRole));
>> +    chk(pUsersInRole->Populate());
>> +
>> +    chk(pUsersInRole->Add((IDispatch **)&pObj));
>> +    chk(GetAdminName(name));
>> +    chk(put_Value(pObj, L"User", _bstr_t(".\\") + name));
>> +
>> +    pObj->Release();
>> +    pObj = NULL;
>> +
>> +    chk(pUsersInRole->Add((IDispatch **)&pObj));
>> +    chk(put_Value(pObj, L"User", L"SYSTEM"));
>> +    chk(pUsersInRole->SaveChanges(&n));
>> +
>> +out:
>> +    if (pUnknown) {
>> +        pUnknown->Release();
>> +    }
>> +    if (pCatalog) {
>> +        pCatalog->Release();
>> +    }
>> +    if (pApps) {
>> +        pApps->Release();
>> +    }
>> +    if (pRoles) {
>> +        pRoles->Release();
>> +    }
>> +    if (pUsersInRole) {
>> +        pUsersInRole->Release();
>> +    }
>> +    if (pObj) {
>> +        pObj->Release();
>> +    }
>> +
>> +    if (FAILED(hr)) {
>> +        COMUnregister();

I've actually grown to accept this COMUnregister() call here... Undoing
all the crap from before staircase-wise would be a nightmare.


>> +    }
>> +    CoUninitialize();

Another question: can you nest CoInitialize() calls (and more
importantly, does CoUninitialize() respect that)? Consider the following
call chain:

COMRegister()
  CoInitialize()
  COMUnregister() -- on the "out:" path
    DllUnregisterServer()
      CoInitialize() -- init after init
      CoUninitialize()
    CoInitialize()
    CoUninitialize()
  CoUninitialize() -- right here, uninit after uninit

Will these work (especially the final two)?


>> +
>> +    return hr;
>> +}
>> +
>> +/* Unregister this module from COM+ Applications Catalog */
>> +STDAPI COMUnregister(void)
>> +{
>> +    HRESULT hr;
>> +    IUnknown *pUnknown = NULL;
>> +    ICOMAdminCatalog *pCatalog = NULL;
>> +    ICatalogCollection *pColl = NULL;
>> +    ICatalogObject *pObj = NULL;
>> +    _variant_t var;
>> +    long i, n;
>> +
>> +    if (VSSCheckOSVersion() == S_FALSE) {
>> +        printf("VSS provider is not supported in this OS version.\n");

I suggest stderr again.


>> +        return S_FALSE; /* VSS feature is disabled */
>> +    }
>> +
>> +    chk(DllUnregisterServer());

Apparently DllUnregisterServer() can't fail. (And it shouldn't, we
depend on it, since COMUnregister() is also used as destructor after
partial construction, so we must not bail here.) I propose to change the
return type of DllUnregisterServer() to void, just to drive the point
home.


>> +
>> +    chk(CoInitialize(NULL));

(same question as before -- can this fail? See unconditional uninit at
the bottom.)

I'll continue from here.

Thanks
Laszlo


>> +    chk(CoCreateInstance(CLSID_COMAdminCatalog, NULL, CLSCTX_INPROC_SERVER,
>> +                         IID_IUnknown, (void **)&pUnknown));
>> +    chk(pUnknown->QueryInterface(IID_ICOMAdminCatalog, (void **)&pCatalog));
>> +
>> +    chk(pCatalog->GetCollection(_bstr_t(L"Applications"),
>> +                                (IDispatch **)&pColl));
>> +    chk(pColl->Populate());
>> +
>> +    chk(pColl->get_Count(&n));
>> +    for (i = n - 1; i >= 0; i--) {
>> +        chk(pColl->get_Item(i, (IDispatch **)&pObj));
>> +        chk(pObj->get_Value(_bstr_t(L"Name"), &var));
>> +        if (var == _variant_t(QGA_PROVIDER_LNAME)) {
>> +            printf("Removing COM+ Application: %S\n", (wchar_t *)_bstr_t(var));
>> +            chk(pColl->Remove(i));
>> +        }
>> +    }
>> +    chk(pColl->SaveChanges(&n));
>> +
>> +out:
>> +    if (pUnknown) {
>> +        pUnknown->Release();
>> +    }
>> +    if (pCatalog) {
>> +        pCatalog->Release();
>> +    }
>> +    if (pColl) {
>> +        pColl->Release();
>> +    }
>> +    if (pObj) {
>> +        pObj->Release();
>> +    }
>> +    CoUninitialize();
>> +
>> +    return hr;
>> +}
>> +
>> +
>> +static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR data)
>> +{
>> +    HKEY  hKey;
>> +    LONG  ret;
>> +    DWORD size;
>> +
>> +    ret = RegCreateKeyEx(HKEY_CLASSES_ROOT, key, 0, NULL,
>> +        REG_OPTION_NON_VOLATILE, KEY_WRITE, NULL, &hKey, NULL);
>> +    if (ret != ERROR_SUCCESS) {
>> +        goto out;
>> +    }
>> +
>> +    if (data != NULL) {
>> +        size = (lstrlen(data) + 1) * sizeof(TCHAR);
>> +    } else {
>> +        size = 0;
>> +    }
>> +
>> +    ret = RegSetValueEx(hKey, value, 0, REG_SZ, (LPBYTE)data, size);
>> +    RegCloseKey(hKey);
>> +
>> +out:
>> +    if (ret != ERROR_SUCCESS) {
>> +        /* We cannot printf here, and need MessageBox to report an error. */
>> +        errmsg_dialog(ret, "Cannot add registry ", key);
>> +        return FALSE;
>> +    }
>> +    return TRUE;
>> +}
>> +
>> +/* Register this dll as a VSS provider */
>> +STDAPI DllRegisterServer(void)
>> +{
>> +    IVssAdmin *pVssAdmin = NULL;
>> +    HRESULT hr = E_FAIL;
>> +    char dllPath[MAX_PATH];
>> +    char key[256];
>> +
>> +    CoInitialize(NULL);
>> +
>> +    if (!g_hinstDll) {
>> +        errmsg_dialog(hr, "Module instance is not available");
>> +        goto out;
>> +    }
>> +
>> +    /* Add this module to registery */
>> +
>> +    sprintf(key, "CLSID\\%s", g_szClsid);
>> +    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
>> +        goto out;
>> +    }
>> +
>> +    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
>> +        errmsg_dialog(GetLastError(), "GetModuleFileName failed");
>> +        goto out;
>> +    }
>> +    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
>> +    if (!CreateRegistryKey(key, NULL, dllPath)) {
>> +        goto out;
>> +    }
>> +
>> +    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
>> +    if (!CreateRegistryKey(key, "ThreadingModel", "Apartment")) {
>> +        goto out;
>> +    }
>> +
>> +    sprintf(key, "CLSID\\%s\\ProgID", g_szClsid);
>> +    if (!CreateRegistryKey(key, NULL, g_szProgid)) {
>> +        goto out;
>> +    }
>> +
>> +    if (!CreateRegistryKey(g_szProgid, NULL, QGA_PROVIDER_NAME)) {
>> +        goto out;
>> +    }
>> +
>> +    sprintf(key, "%s\\CLSID", g_szProgid);
>> +    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
>> +        goto out;
>> +    }
>> +
>> +    hr = CoCreateInstance(CLSID_VSSCoordinator,
>> +        NULL, CLSCTX_ALL, IID_IVssAdmin, (void **)&pVssAdmin);
>> +    if (FAILED(hr)) {
>> +        errmsg_dialog(hr, "CoCreateInstance(VSSCoordinator) failed");
>> +        goto out;
>> +    }
>> +
>> +    hr = pVssAdmin->RegisterProvider(
>> +        g_gProviderId, CLSID_QGAVSSProvider,
>> +        const_cast<WCHAR*>(QGA_PROVIDER_LNAME), VSS_PROV_SOFTWARE,
>> +        const_cast<WCHAR*>(QGA_PROVIDER_VERSION), g_gProviderVersion);
>> +    if (FAILED(hr)) {
>> +        errmsg_dialog(hr, "RegisterProvider failed");
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    if (pVssAdmin) {
>> +        pVssAdmin->Release();
>> +    }
>> +    CoUninitialize();
>> +
>> +    if (FAILED(hr)) {
>> +        DllUnregisterServer();
>> +    }
>> +
>> +    return hr;
>> +}
>> +
>> +/* Unregister this VSS hardware provider from the system */
>> +STDAPI DllUnregisterServer(void)
>> +{
>> +    TCHAR key[256];
>> +    IVssAdmin *pVssAdmin = NULL;
>> +
>> +    CoInitialize(NULL);
>> +
>> +    HRESULT hr = CoCreateInstance(CLSID_VSSCoordinator,
>> +         NULL, CLSCTX_ALL, IID_IVssAdmin, (void **)&pVssAdmin);
>> +    if (SUCCEEDED(hr)) {
>> +        hr = pVssAdmin->UnregisterProvider(g_gProviderId);
>> +        pVssAdmin->Release();
>> +    } else {
>> +        errmsg_dialog(hr, "CoCreateInstance(VSSCoordinator) failed");
>> +    }
>> +
>> +    sprintf(key, "CLSID\\%s", g_szClsid);
>> +    SHDeleteKey(HKEY_CLASSES_ROOT, key);
>> +    SHDeleteKey(HKEY_CLASSES_ROOT, g_szProgid);
>> +
>> +    CoUninitialize();
>> +
>> +    return S_OK; /* Uninstall should never fail */
>> +}
>> +
>> +
>> +/* Support functions for _bstr_t in MinGW: Originally written by: Diaa Sami */
>> +
>> +void __stdcall _com_issue_error(HRESULT hr)
>> +{
>> +    printf("_com_issue_error() called with parameter HRESULT = %lu", hr);
>> +}
>> +
>> +namespace _com_util
>> +{
>> +    char * __stdcall ConvertBSTRToString(BSTR bstr)
>> +    {
>> +        const unsigned int stringLength = lstrlenW(bstr);
>> +        char *const ascii = new char [stringLength + 1];
>> +
>> +        wcstombs(ascii, bstr, stringLength + 1);
>> +
>> +        return ascii;
>> +    }
>> +
>> +    BSTR __stdcall ConvertStringToBSTR(const char *const ascii)
>> +    {
>> +        const unsigned int stringLength = lstrlenA(ascii);
>> +        BSTR bstr = SysAllocStringLen(NULL, stringLength);
>> +
>> +        mbstowcs(bstr, ascii, stringLength + 1);
>> +
>> +        return bstr;
>> +    }
>> +}
> 
>> diff --git a/qga/vss-win32-provider/provider.cpp b/qga/vss-win32-provider/provider.cpp
>> new file mode 100644
>> index 0000000..90a3d25
>> --- /dev/null
>> +++ b/qga/vss-win32-provider/provider.cpp
>> @@ -0,0 +1,479 @@
>> +/*
>> + * QEMU Guest Agent win32 VSS Provider implementations
>> + *
>> + * Copyright Hitachi Data Systems Corp. 2013
>> + *
>> + * Authors:
>> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include <stdio.h>
>> +#include "../vss-win32.h"
>> +#include "inc/win2003/vscoordint.h"
>> +#include "inc/win2003/vsprov.h"
>> +
>> +static long g_nComObjsInUse;
>> +HINSTANCE g_hinstDll;
>> +
>> +/* VSS common GUID's */
>> +
>> +const CLSID CLSID_VSSCoordinator = { 0xE579AB5F, 0x1CC4, 0x44b4,
>> +    {0xBE, 0xD9, 0xDE, 0x09, 0x91, 0xFF, 0x06, 0x23} };
>> +const IID IID_IVssAdmin = { 0x77ED5996, 0x2F63, 0x11d3,
>> +    {0x8A, 0x39, 0x00, 0xC0, 0x4F, 0x72, 0xD8, 0xE3} };
>> +
>> +const IID IID_IVssHardwareSnapshotProvider = { 0x9593A157, 0x44E9, 0x4344,
>> +    {0xBB, 0xEB, 0x44, 0xFB, 0xF9, 0xB0, 0x6B, 0x10} };
>> +const IID IID_IVssSoftwareSnapshotProvider = { 0x609e123e, 0x2c5a, 0x44d3,
>> +    {0x8f, 0x01, 0x0b, 0x1d, 0x9a, 0x47, 0xd1, 0xff} };
>> +const IID IID_IVssProviderCreateSnapshotSet = { 0x5F894E5B, 0x1E39, 0x4778,
>> +    {0x8E, 0x23, 0x9A, 0xBA, 0xD9, 0xF0, 0xE0, 0x8C} };
>> +const IID IID_IVssProviderNotifications = { 0xE561901F, 0x03A5, 0x4afe,
>> +    {0x86, 0xD0, 0x72, 0xBA, 0xEE, 0xCE, 0x70, 0x04} };
>> +
>> +const IID IID_IVssEnumObject = { 0xAE1C7110, 0x2F60, 0x11d3,
>> +    {0x8A, 0x39, 0x00, 0xC0, 0x4F, 0x72, 0xD8, 0xE3} };
>> +
>> +
>> +void LockModule(BOOL block)
>> +{
>> +    if (block) {
>> +        InterlockedIncrement(&g_nComObjsInUse);
>> +    } else {
>> +        InterlockedDecrement(&g_nComObjsInUse);
>> +    }
>> +}
>> +
>> +/* Empty enumerator for VssObject */
>> +
>> +class CQGAVSSEnumObject : public IVssEnumObject
>> +{
>> +public:
>> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppObj);
>> +    STDMETHODIMP_(ULONG) AddRef();
>> +    STDMETHODIMP_(ULONG) Release();
>> +
>> +    /* IVssEnumObject Methods */
>> +    STDMETHODIMP Next(
>> +        ULONG celt, VSS_OBJECT_PROP *rgelt, ULONG *pceltFetched);
>> +    STDMETHODIMP Skip(ULONG celt);
>> +    STDMETHODIMP Reset(void);
>> +    STDMETHODIMP Clone(IVssEnumObject **ppenum);
>> +
>> +    /* CQGAVSSEnumObject Methods */
>> +    CQGAVSSEnumObject();
>> +    ~CQGAVSSEnumObject();
>> +
>> +private:
>> +    long m_nRefCount;
>> +};
>> +
>> +CQGAVSSEnumObject::CQGAVSSEnumObject()
>> +{
>> +    m_nRefCount = 0;
>> +    LockModule(TRUE);
>> +}
>> +
>> +CQGAVSSEnumObject::~CQGAVSSEnumObject()
>> +{
>> +    LockModule(FALSE);
>> +}
>> +
>> +STDMETHODIMP CQGAVSSEnumObject::QueryInterface(REFIID riid, void **ppObj)
>> +{
>> +    if (riid == IID_IUnknown || riid == IID_IVssEnumObject) {
>> +        *ppObj = static_cast<void*>(static_cast<IVssEnumObject*>(this));
>> +        AddRef();
>> +        return S_OK;
>> +    }
>> +    ppObj = NULL;
>> +    return E_NOINTERFACE;
>> +}
>> +
>> +STDMETHODIMP_(ULONG) CQGAVSSEnumObject::AddRef()
>> +{
>> +    return InterlockedIncrement(&m_nRefCount);
>> +}
>> +
>> +STDMETHODIMP_(ULONG) CQGAVSSEnumObject::Release()
>> +{
>> +    long nRefCount = InterlockedDecrement(&m_nRefCount);
>> +    if (m_nRefCount == 0) {
>> +        delete this;
>> +    }
>> +    return nRefCount;
>> +}
>> +
>> +STDMETHODIMP CQGAVSSEnumObject::Next(
>> +    ULONG celt, VSS_OBJECT_PROP *rgelt, ULONG *pceltFetched)
>> +{
>> +    *pceltFetched = 0;
>> +    return S_FALSE;
>> +}
>> +
>> +STDMETHODIMP CQGAVSSEnumObject::Skip(ULONG celt)
>> +{
>> +    return S_FALSE;
>> +}
>> +
>> +STDMETHODIMP CQGAVSSEnumObject::Reset(void)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVSSEnumObject::Clone(IVssEnumObject **ppenum)
>> +{
>> +    return E_NOTIMPL;
>> +}
>> +
>> +
>> +/* QGAVssProvider */
>> +
>> +class CQGAVssProvider :
>> +    public IVssSoftwareSnapshotProvider,
>> +    public IVssProviderCreateSnapshotSet,
>> +    public IVssProviderNotifications
>> +{
>> +public:
>> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppObj);
>> +    STDMETHODIMP_(ULONG) AddRef();
>> +    STDMETHODIMP_(ULONG) Release();
>> +
>> +    /* IVssSoftwareSnapshotProvider Methods */
>> +    STDMETHODIMP SetContext(LONG lContext);
>> +    STDMETHODIMP GetSnapshotProperties(
>> +        VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp);
>> +    STDMETHODIMP Query(
>> +        VSS_ID QueriedObjectId, VSS_OBJECT_TYPE eQueriedObjectType,
>> +        VSS_OBJECT_TYPE eReturnedObjectsType, IVssEnumObject **ppEnum);
>> +    STDMETHODIMP DeleteSnapshots(
>> +        VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
>> +        BOOL bForceDelete, LONG *plDeletedSnapshots,
>> +        VSS_ID *pNondeletedSnapshotID);
>> +    STDMETHODIMP BeginPrepareSnapshot(
>> +        VSS_ID SnapshotSetId, VSS_ID SnapshotId,
>> +        VSS_PWSZ pwszVolumeName, LONG lNewContext);
>> +    STDMETHODIMP IsVolumeSupported(
>> +        VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider);
>> +    STDMETHODIMP IsVolumeSnapshotted(
>> +        VSS_PWSZ pwszVolumeName, BOOL *pbSnapshotsPresent,
>> +        LONG *plSnapshotCompatibility);
>> +    STDMETHODIMP SetSnapshotProperty(
>> +        VSS_ID SnapshotId, VSS_SNAPSHOT_PROPERTY_ID eSnapshotPropertyId,
>> +        VARIANT vProperty);
>> +    STDMETHODIMP RevertToSnapshot(VSS_ID SnapshotId);
>> +    STDMETHODIMP QueryRevertStatus(VSS_PWSZ pwszVolume, IVssAsync **ppAsync);
>> +
>> +    /* IVssProviderCreateSnapshotSet Methods */
>> +    STDMETHODIMP EndPrepareSnapshots(VSS_ID SnapshotSetId);
>> +    STDMETHODIMP PreCommitSnapshots(VSS_ID SnapshotSetId);
>> +    STDMETHODIMP CommitSnapshots(VSS_ID SnapshotSetId);
>> +    STDMETHODIMP PostCommitSnapshots(
>> +        VSS_ID SnapshotSetId, LONG lSnapshotsCount);
>> +    STDMETHODIMP PreFinalCommitSnapshots(VSS_ID SnapshotSetId);
>> +    STDMETHODIMP PostFinalCommitSnapshots(VSS_ID SnapshotSetId);
>> +    STDMETHODIMP AbortSnapshots(VSS_ID SnapshotSetId);
>> +
>> +    /* IVssProviderNotifications Methods */
>> +    STDMETHODIMP OnLoad(IUnknown *pCallback);
>> +    STDMETHODIMP OnUnload(BOOL bForceUnload);
>> +
>> +    /* CQGAVssProvider Methods */
>> +    CQGAVssProvider();
>> +    ~CQGAVssProvider();
>> +
>> +private:
>> +    long m_nRefCount;
>> +};
>> +
>> +CQGAVssProvider::CQGAVssProvider()
>> +{
>> +    m_nRefCount = 0;
>> +    LockModule(TRUE);
>> +}
>> +
>> +CQGAVssProvider::~CQGAVssProvider()
>> +{
>> +    LockModule(FALSE);
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::QueryInterface(REFIID riid, void **ppObj)
>> +{
>> +    if (riid == IID_IUnknown) {
>> +        *ppObj = static_cast<void*>(this);
>> +        AddRef();
>> +        return S_OK;
>> +    } else if (riid == IID_IVssSoftwareSnapshotProvider) {
>> +        *ppObj = static_cast<void*>(
>> +            static_cast<IVssSoftwareSnapshotProvider*>(this));
>> +        AddRef();
>> +        return S_OK;
>> +    } else if (riid == IID_IVssProviderCreateSnapshotSet) {
>> +        *ppObj = static_cast<void*>(
>> +            static_cast<IVssProviderCreateSnapshotSet*>(this));
>> +        AddRef();
>> +        return S_OK;
>> +    } else if (riid == IID_IVssProviderNotifications) {
>> +        *ppObj = static_cast<void*>(
>> +            static_cast<IVssProviderNotifications*>(this));
>> +        AddRef();
>> +        return S_OK;
>> +    }
>> +    *ppObj = NULL;
>> +    return E_NOINTERFACE;
>> +}
>> +
>> +STDMETHODIMP_(ULONG) CQGAVssProvider::AddRef()
>> +{
>> +    return InterlockedIncrement(&m_nRefCount);
>> +}
>> +
>> +STDMETHODIMP_(ULONG) CQGAVssProvider::Release()
>> +{
>> +    long nRefCount = InterlockedDecrement(&m_nRefCount);
>> +    if (m_nRefCount == 0) {
>> +        delete this;
>> +    }
>> +    return nRefCount;
>> +}
>> +
>> +
>> +/*
>> + * IVssSoftwareSnapshotProvider methods
>> + */
>> +
>> +STDMETHODIMP CQGAVssProvider::SetContext(LONG lContext)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::GetSnapshotProperties(
>> +    VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp)
>> +{
>> +    return VSS_E_OBJECT_NOT_FOUND;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::Query(
>> +    VSS_ID QueriedObjectId, VSS_OBJECT_TYPE eQueriedObjectType,
>> +    VSS_OBJECT_TYPE eReturnedObjectsType, IVssEnumObject **ppEnum)
>> +{
>> +    *ppEnum = new CQGAVSSEnumObject;
>> +    (*ppEnum)->AddRef();
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::DeleteSnapshots(
>> +    VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
>> +    BOOL bForceDelete, LONG *plDeletedSnapshots, VSS_ID *pNondeletedSnapshotID)
>> +{
>> +    return E_NOTIMPL;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot(
>> +    VSS_ID SnapshotSetId, VSS_ID SnapshotId,
>> +    VSS_PWSZ pwszVolumeName, LONG lNewContext)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::IsVolumeSupported(
>> +    VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider)
>> +{
>> +    *pbSupportedByThisProvider = TRUE;
>> +
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::IsVolumeSnapshotted(VSS_PWSZ pwszVolumeName,
>> +    BOOL *pbSnapshotsPresent, LONG *plSnapshotCompatibility)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::SetSnapshotProperty(VSS_ID SnapshotId,
>> +    VSS_SNAPSHOT_PROPERTY_ID eSnapshotPropertyId, VARIANT vProperty)
>> +{
>> +    return E_NOTIMPL;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::RevertToSnapshot(VSS_ID SnapshotId)
>> +{
>> +    return E_NOTIMPL;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::QueryRevertStatus(
>> +    VSS_PWSZ pwszVolume, IVssAsync **ppAsync)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +
>> +/*
>> + * IVssProviderCreateSnapshotSet methods
>> + */
>> +
>> +STDMETHODIMP CQGAVssProvider::EndPrepareSnapshots(VSS_ID SnapshotSetId)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::PreCommitSnapshots(VSS_ID SnapshotSetId)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId)
>> +{
>> +    HRESULT hr = S_OK;
>> +    HANDLE hEvent, hEvent2;
>> +
>> +    hEvent = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_FROZEN);
>> +    if (hEvent == INVALID_HANDLE_VALUE) {
>> +        hr = E_FAIL;
>> +        goto out;
>> +    }
>> +
>> +    hEvent2 = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_THAW);
>> +    if (hEvent == INVALID_HANDLE_VALUE) {
>> +        CloseHandle(hEvent);
>> +        hr = E_FAIL;
>> +        goto out;
>> +    }
>> +
>> +    SetEvent(hEvent);
>> +    if (WaitForSingleObject(hEvent2, 60*1000) != WAIT_OBJECT_0) {
>> +        hr = E_ABORT;
>> +    }
>> +
>> +    CloseHandle(hEvent2);
>> +    CloseHandle(hEvent);
>> +out:
>> +    return hr;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::PostCommitSnapshots(
>> +    VSS_ID SnapshotSetId, LONG lSnapshotsCount)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::PreFinalCommitSnapshots(VSS_ID SnapshotSetId)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::PostFinalCommitSnapshots(VSS_ID SnapshotSetId)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::AbortSnapshots(VSS_ID SnapshotSetId)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +/*
>> + * IVssProviderNotifications methods
>> + */
>> +
>> +STDMETHODIMP CQGAVssProvider::OnLoad(IUnknown *pCallback)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProvider::OnUnload(BOOL bForceUnload)
>> +{
>> +    return S_OK;
>> +}
>> +
>> +
>> +/*
>> + * CQGAVssProviderFactory class
>> + */
>> +
>> +class CQGAVssProviderFactory : public IClassFactory
>> +{
>> +public:
>> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppv);
>> +    STDMETHODIMP_(ULONG) AddRef();
>> +    STDMETHODIMP_(ULONG) Release();
>> +    STDMETHODIMP CreateInstance(
>> +        IUnknown *pUnknownOuter, REFIID iid, void **ppv);
>> +    STDMETHODIMP LockServer(BOOL bLock) { return E_NOTIMPL; }
>> +private:
>> +    long m_nRefCount;
>> +};
>> +
>> +STDMETHODIMP CQGAVssProviderFactory::QueryInterface(REFIID riid, void **ppv)
>> +{
>> +    if (riid == IID_IUnknown || riid == IID_IClassFactory) {
>> +        *ppv = static_cast<void*>(this);
>> +        AddRef();
>> +        return S_OK;
>> +    }
>> +    *ppv = NULL;
>> +    return E_NOINTERFACE;
>> +}
>> +
>> +STDMETHODIMP_(ULONG) CQGAVssProviderFactory::AddRef()
>> +{
>> +    LockModule(TRUE);
>> +    return 2;
>> +}
>> +
>> +STDMETHODIMP_(ULONG) CQGAVssProviderFactory::Release()
>> +{
>> +    LockModule(FALSE);
>> +    return 1;
>> +}
>> +
>> +STDMETHODIMP CQGAVssProviderFactory::CreateInstance(
>> +    IUnknown *pUnknownOuter, REFIID iid, void **ppv)
>> +{
>> +    if (pUnknownOuter) {
>> +        return CLASS_E_NOAGGREGATION;
>> +    }
>> +    CQGAVssProvider *pObj = new CQGAVssProvider;
>> +    if (!pObj) {
>> +        return E_OUTOFMEMORY;
>> +    }
>> +    return pObj->QueryInterface(iid, ppv);
>> +}
>> +
>> +
>> +/*
>> + * DLL functions
>> + */
>> +
>> +STDAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID *ppv)
>> +{
>> +    static CQGAVssProviderFactory factory;
>> +
>> +    *ppv = NULL;
>> +    if (IsEqualCLSID(rclsid, CLSID_QGAVSSProvider)) {
>> +        return factory.QueryInterface(riid, ppv);
>> +    }
>> +    return CLASS_E_CLASSNOTAVAILABLE;
>> +}
>> +
>> +STDAPI DllCanUnloadNow()
>> +{
>> +    return g_nComObjsInUse == 0 ? S_OK : S_FALSE;
>> +}
>> +
>> +EXTERN_C
>> +BOOL WINAPI DllMain(HINSTANCE hinstDll, DWORD dwReason, LPVOID lpReserved)
>> +{
>> +    switch (dwReason) {
>> +
>> +    case DLL_PROCESS_ATTACH:
>> +        g_hinstDll = hinstDll;
>> +        DisableThreadLibraryCalls(hinstDll);
>> +        break;
>> +    }
>> +
>> +    return TRUE;
>> +}
>
Paolo Bonzini - June 26, 2013, 9:27 p.m.
Il 26/06/2013 16:32, Laszlo Ersek ha scritto:
>>> >> +#define EVENT_NAME_FROZEN "Global\\QGAVSSEvent-frozen"
>>> >> +#define EVENT_NAME_THAW   "Global\\QGAVSSEvent-thaw"
> No idea what I'm talking about, but since we're qualifying the second
> component with the "QGAVSSEvent" prefix, shouldn't we just replace the
> first component ("Global") with it? Or would it effect "event routing"?

Yes:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms684305%28v=vs.85%29.aspx
says

Terminal Services:  The name can have a "Global\" or "Local\" prefix to
explicitly open an object in the global or session namespace. The
remainder of the name can contain any character except the backslash
character (\). For more information, see Kernel Object Namespaces.

Paolo
Tomoki Sekiyama - June 26, 2013, 10:09 p.m.
On 6/26/13 10:32 , "Laszlo Ersek" <lersek@redhat.com> wrote:

>On 06/25/13 18:03, Laszlo Ersek wrote:
>> On 06/06/13 17:06, Tomoki Sekiyama wrote:
>
>Comparing the .def file and the provider header file:
>...
>(a) what is the reason for not listing DllCanUnloadNow() and
>DllGetClassObject() in the header file?

Because these two are reserved functions for COM, and not
Referenced from qemu-ga.exe.

>(b) Does STDAPI imply a return type? Or are you just going with the
>implicit "int"? (Based on my encounters with EFIAPI, I think it's the
>latter.)

It is extracted as 'extern "C" HRESULT __stdcall'.
(__stdcall specifies calling convention.)

>>>+#define EVENT_NAME_FROZEN "Global\\QGAVSSEvent-frozen"
>>> +#define EVENT_NAME_THAW   "Global\\QGAVSSEvent-thaw"
>
>No idea what I'm talking about, but since we're qualifying the second
>component with the "QGAVSSEvent" prefix, shouldn't we just replace the
>first component ("Global") with it? Or would it effect "event routing"?
>(Tangential question, anyway.)

Latter. "Global\\" is a reserved namespace in Windows used for
communication between services and the other program, so we can't
omit this.

>>> +const GUID CLSID_COMAdminCatalog = { 0xF618C514, 0xDFB8, 0x11d1,
>>> +    {0xA2, 0xCF, 0x00, 0x80, 0x5F, 0xC7, 0x92, 0x35} };
>>> +const GUID IID_ICOMAdminCatalog = { 0xDD662187, 0xDFC2, 0x11d1,
>>> +    {0xA2, 0xCF, 0x00, 0x80, 0x5F, 0xC7, 0x92, 0x35} };
>>> +const GUID CLSID_WbemLocator = { 0x4590f811, 0x1d3a, 0x11d0,
>>> +    {0x89, 0x1f, 0x00, 0xaa, 0x00, 0x4b, 0x2e, 0x24} };
>>> +const GUID IID_IWbemLocator = { 0xdc12a687, 0x737f, 0x11cf,
>>> +    {0x88, 0x4d, 0x00, 0xaa, 0x00, 0x4b, 0x2e, 0x24} };
>
>Would it be possible to make these static? Not too important of course.

Some of these are declared as "extern" in headers in MinGW or VSS SDK,
so unfortunately we can't make them static.

>>>+
>>> +static void errmsg(DWORD err, const char *text)
>>> +{
>>> +    char *msg = NULL, *nul = strchr(text, '(');
>>> +    int len = nul ? nul - text : -1;
>>> +
>>> +    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
>>> +                  FORMAT_MESSAGE_FROM_SYSTEM |
>>>FORMAT_MESSAGE_IGNORE_INSERTS,
>>> +                  NULL, err, MAKELANGID(LANG_NEUTRAL,
>>>SUBLANG_DEFAULT),
>>> +                  (char *)&msg, 0, NULL);
>
>(The FormatMessage() specification in MSDN is simply incredible.
>"(char *)&msg", seriously? Your code is correct, but the interface is
>unbelievable.)

Yeah, really...

>>> +    printf("%.*s. (Error: %lx) %s\n", len, text, err, msg);
>
>(a) Since we're printing an error message here, I suspect it should go
>to stderr.

OK.

>(b) In case "text" doesn't contain an opening paren, "len" is set to -1.
>"A negative precision is taken as if the precision were omitted", IOW
>the entire string will be printed. Clever!

Maybe need some comments...

>(c) Does DWORD expand to "uint32_t"? In that case "%lx" is too big on
>64-bit (LP64). We should probably use "%x" or "%"PRIx32. (Or do we build
>qemu-ga.exe only for 32-bit? I seem to remember something like that.)

DWORD is typedef as unsigned long int. MinGW64 uses LLP64 model, so
it is 32bit both in 32-bit and 64-bit MinGW.
So if I use %x here, gcc outputs:

qga/vss-win32-provider/install.cpp:44:57: warning: format '%x' expects
argument of type 'unsigned int', but argument 4 has type 'DWORD {aka long
unsigned int}' [-Wformat]

>>>+    snprintf(buf, sizeof(buf), "%s%s. (Error: %lx) %s\n", text, opt,
>>>err, msg);
>>> +    MessageBox(NULL, buf, "Error from " QGA_PROVIDER_NAME,
>>>MB_OK|MB_ICONERROR);
>
>Do you need a trailing newline for a message box?

No. I will remove "\n". Thanks.

>>> +/* Lookup Administrators group name from winmgmt */
>>> +static HRESULT GetAdminName(_bstr_t &name)
>>> +{
>
>I can see that you want to call GetAdminName() in chk(), hence you must
>return a HRESULT and store the value through the parameter list.
>However, can we keep the C++ features to a minimum? I'd prefer if the
>non-const reference were replaced with a pointer-to-_bstr_t.
>(Tangential, change only if you respin anyway and don't mind it.)

OK, I will make this into 'static HRESULT GetAdminName(_bstr_t *name)'.

>>> +    if (!pEnum) {
>>> +        errmsg(E_FAIL, "Failed to query for Administrators");
>>> +        goto out;
>
>I think you forgot to set "hr" to something ugly -- the code below the
>"out" label will simply return it, and "hr" is currently something nice.
>(The last chk() succeeded.)

I will add "hr = E_FAIL;" here. Thanks.


>>> +    }
>
>This branch is probably for the case when ExecQuery() succeeds, but
>doesn't return any rows, right?

We must not come here (while the reference is accurate).
It is just in case.

>>> +    chk(pEnum->Next(WBEM_INFINITE, 1, &pWobj, &returned));
>>> +    if (returned == 0) {
>>> +        errmsg(E_FAIL, "Failed to query for Administrators");
>>> +        goto out;
>>> +    }
>>> +
>>> +    chk(pWobj->Get(_bstr_t(L"Name"), 0, &var, 0, 0));
>>> +    name = var;
>
>This assigns a _variant_t to a _bstr_t (... for which I proposed *name =
>var above, but it's irrelevant now).
>
>Can this conversion (or the assignment operator) fail? If so, does
>either throw an exception?

Hm, yes. I will add try ... catch here.

>>> +out:
>>> +    if (pLoc) {
>>> +        pLoc->Release();
>>> +    }
>>> +    if (pSvc) {
>>> +        pSvc->Release();
>>> +    }
>>> +    if (pEnum) {
>>> +        pEnum->Release();
>>> +    }
>>> +    if (pWobj) {
>>> +        pWobj->Release();
>>> +    }
>>> +    return hr;
>>> +}
>
>I'm sure this is idiomatic code, but do these pointers point to static /
>long-term instances? I'm missing the equivalent of "delete pLoc" and so
>on. Each Release() member function gets the implicit "this" parameter
>here, but do they free it too? (Honestly, I can't decide which would be
>worse, if they did or they didn't.)
>
>... Ah, it's probably reference counting. OK.

Right, it's decrementing reference counters.


>>>+    if (!g_hinstDll) {
>>>+        errmsg(E_FAIL, "Failed to initialize DLL");
>>>+        goto out;
>
>OK, "hr" is E_FAIL here.

Ah, this must be rewritten to "return", as described below.

>>>+    }
>>>+
>>>+    if (VSSCheckOSVersion() == S_FALSE) {
>>> +        printf("VSS provider is not supported in this OS version.\n");
>
>I suggest stderr.

OK.

>>> +        return S_FALSE; /* VSS feature is disabled */
>
>Somewhat inconsistent with the above, you issue "return" although you
>used "goto out" before. No resources are leaked so it's no problem in
>practice. (You could have written "return" above.)

This must be return, because we must not call CoUninitialize()
in the "out:" path. I will rewrite "goto out" above into "return".

>>> +    }
>>> +
>>> +    COMUnregister();
>
>What requires / justifies this? (I can see another call below, on the
>error path.) I think whatever COMUnregister() would release depends
>first on the success of COMRegister(), doesn't it?

It was intended to clean up something in the catalog with the same
name, just in case.

>You've told Paolo,
>
>On 06/26/13 00:31, Tomoki Sekiyama wrote:
>> COMRegister and COMUnregister are called by`qemu-ga -s install`,
>> to register/unregister the DLL into/from COM+ application catalogue.
>
>and indeed I can see a COMRegister() call in 09/10. Nonetheless that
>doesn't seem to justify this call to COMUnregister() -- if the DLL has
>been previously registered in the application catalog, a new
>registration attempt should fail.

Okey, then I will remove this COMUnregister().


>>> +
>>> +    chk(CoInitialize(NULL));
>
>Can this fail in practice? If so, we could have error handling trouble,
>because "out:" will call CoUninitialize() without any successful
>initialization.

Ah, it won't fail. I will remove chk() here.

>>>+    /* Install COM+ Component */
>>> +
>>> +    chk(pCatalog->GetCollection(_bstr_t(L"Applications"),
>>> +                                (IDispatch **)&pApps));
>>> +    chk(pApps->Populate());
>>> +    chk(pApps->Add((IDispatch **)&pObj));
>>> +    chk(put_Value(pObj, L"Name",        QGA_PROVIDER_LNAME));
>>> +    chk(put_Value(pObj, L"Description", QGA_PROVIDER_LNAME));
>>> +    chk(put_Value(pObj, L"ApplicationAccessChecksEnabled", true));
>>> +    chk(put_Value(pObj, L"Authentication",                 short(6)));
>>> +    chk(put_Value(pObj, L"AuthenticationCapability",       short(2)));
>>> +    chk(put_Value(pObj, L"ImpersonationLevel",             short(2)));
>>> +    chk(pApps->SaveChanges(&n));
>
>This looks like a globally visible change that is not rolled back if
>something fails later in this function. Is the COMUnregister() call on
>the error handling path supposed to undo these? Complicated :)

This code is creating a tree, whose root is an "Application"
with the name of QGA_PROVIDER_LNAME.
COMUnregister will erase whole tree by erasing the root,
and every modification below will be rolled back.

>Then, should we check "n", or does the retval check suffice?

If something fails, it returns error (E_***), so I think the
retval check is good enough.


>>> +
>>> +    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
>>> +        errmsg(GetLastError(), "GetModuleFileName failed");
>>> +        goto out;
>
>"hr" has not been set to a failure code.

Oops, I will add a code to set hr here,

>>> +    }
>>> +    n = strlen(dllPath);
>>> +    if (n < 3) {
>>> +        errmsg(E_FAIL, "Failed to lookup dll");
>
>Presumably you wanted to set "hr" and jump to "out" as well.

And here too, with "goto out;".

>>> +    }
>>> +    strcpy(tlbPath, dllPath);
>>> +    strcpy(tlbPath+n-3, "TLB");
>>> +    printf("Registering " QGA_PROVIDER_NAME ":\n");
>>> +    printf("  %s\n", dllPath);
>>> +    printf("  %s\n", tlbPath);
>
>These are arguably diagnostic messages (destined for stderr), but I
>won't press it :)

OK.

>>> +    if (!PathFileExists(tlbPath)) {
>>> +        errmsg(ERROR_FILE_NOT_FOUND, "Failed to lookup tlb");
>>> +        goto out;
>
>"hr" not set properly.

And here too.


...


>>> +    CoUninitialize();
>
>Another question: can you nest CoInitialize() calls (and more
>importantly, does CoUninitialize() respect that)? Consider the following
>call chain:
>
>COMRegister()
>  CoInitialize()
>  COMUnregister() -- on the "out:" path
>    DllUnregisterServer()
>      CoInitialize() -- init after init
>      CoUninitialize()
>    CoInitialize()
>    CoUninitialize()
>  CoUninitialize() -- right here, uninit after uninit
>
>Will these work (especially the final two)?

Yes, it can be nested, as far as CoInitialize and CoUninitialize
are balanced.


>>>+/* Unregister this module from COM+ Applications Catalog */
>>> +STDAPI COMUnregister(void)
>>> +{
>>> +    HRESULT hr;
>>> +    IUnknown *pUnknown = NULL;
>>> +    ICOMAdminCatalog *pCatalog = NULL;
>>> +    ICatalogCollection *pColl = NULL;
>>> +    ICatalogObject *pObj = NULL;
>>> +    _variant_t var;
>>> +    long i, n;
>>> +
>>> +    if (VSSCheckOSVersion() == S_FALSE) {
>>> +        printf("VSS provider is not supported in this OS version.\n");
>
>I suggest stderr again.

OK.

>>> +        return S_FALSE; /* VSS feature is disabled */
>>> +    }
>>> +
>>> +    chk(DllUnregisterServer());
>
>Apparently DllUnregisterServer() can't fail. (And it shouldn't, we
>depend on it, since COMUnregister() is also used as destructor after
>partial construction, so we must not bail here.) I propose to change the
>return type of DllUnregisterServer() to void, just to drive the point
>home.

OK. I can't make it void (because it is a reserved interface for COM),
but will remove chk here.


>>> +
>>> +    chk(CoInitialize(NULL));
>
>(same question as before -- can this fail? See unconditional uninit at
>the bottom.)

Will remove chk from here too.


>I'll continue from here.

Thanks a lot for reviewing this long complicated windows code!


Tomoki Sekiyama
Laszlo Ersek - June 27, 2013, 3:01 p.m.
On 06/26/13 16:32, Laszlo Ersek wrote:
> On 06/25/13 18:03, Laszlo Ersek wrote:
>> On 06/06/13 17:06, Tomoki Sekiyama wrote:

>>> +/* Unregister this module from COM+ Applications Catalog */
>>> +STDAPI COMUnregister(void)
>>> +{
>>> +    HRESULT hr;
>>> +    IUnknown *pUnknown = NULL;
>>> +    ICOMAdminCatalog *pCatalog = NULL;
>>> +    ICatalogCollection *pColl = NULL;
>>> +    ICatalogObject *pObj = NULL;
>>> +    _variant_t var;
>>> +    long i, n;
>>> +
>>> +    if (VSSCheckOSVersion() == S_FALSE) {
>>> +        printf("VSS provider is not supported in this OS version.\n");
>>> +        return S_FALSE; /* VSS feature is disabled */
>>> +    }
>>> +
>>> +    chk(DllUnregisterServer());
>>> +
>>> +    chk(CoInitialize(NULL));

picking up here

>>> +    chk(CoCreateInstance(CLSID_COMAdminCatalog, NULL, CLSCTX_INPROC_SERVER,
>>> +                         IID_IUnknown, (void **)&pUnknown));
>>> +    chk(pUnknown->QueryInterface(IID_ICOMAdminCatalog, (void **)&pCatalog));
>>> +
>>> +    chk(pCatalog->GetCollection(_bstr_t(L"Applications"),
>>> +                                (IDispatch **)&pColl));
>>> +    chk(pColl->Populate());
>>> +
>>> +    chk(pColl->get_Count(&n));
>>> +    for (i = n - 1; i >= 0; i--) {
>>> +        chk(pColl->get_Item(i, (IDispatch **)&pObj));
>>> +        chk(pObj->get_Value(_bstr_t(L"Name"), &var));
>>> +        if (var == _variant_t(QGA_PROVIDER_LNAME)) {
>>> +            printf("Removing COM+ Application: %S\n", (wchar_t *)_bstr_t(var));

(stderr)

>>> +            chk(pColl->Remove(i));
>>> +        }

I think you leak a pObj reference here, at the end of the iteration. The
next round will set pObj to something else; I think we should call
pObj->Release() here, and set pObj to NULL (for the case when this is
the last iteration).

I'm not sure if you're allowed to call pObj->Release() after the
pColl()->Remove(i) call. So maybe call pObj->Release() in an else
branch. (In this case however the out: logic should be modified as
well.)

>>> +    }
>>> +    chk(pColl->SaveChanges(&n));

Right, there's not much to do if deregistration fails...

>>> +
>>> +out:
>>> +    if (pUnknown) {
>>> +        pUnknown->Release();
>>> +    }
>>> +    if (pCatalog) {
>>> +        pCatalog->Release();
>>> +    }
>>> +    if (pColl) {
>>> +        pColl->Release();
>>> +    }
>>> +    if (pObj) {
>>> +        pObj->Release();
>>> +    }
>>> +    CoUninitialize();
>>> +
>>> +    return hr;
>>> +}
>>> +
>>> +
>>> +static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR data)
>>> +{
>>> +    HKEY  hKey;
>>> +    LONG  ret;
>>> +    DWORD size;
>>> +
>>> +    ret = RegCreateKeyEx(HKEY_CLASSES_ROOT, key, 0, NULL,
>>> +        REG_OPTION_NON_VOLATILE, KEY_WRITE, NULL, &hKey, NULL);
>>> +    if (ret != ERROR_SUCCESS) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (data != NULL) {
>>> +        size = (lstrlen(data) + 1) * sizeof(TCHAR);

I think we should drop the multiplication by sizeof(TCHAR), it's 1.
According to MSDN, TCHAR could be something wider than "char" (ie.
WCHAR) "for Unicode platforms".

However in all your CreateRegistryKey() calls, the argument passed as
"data" depends on sizeof(TCHAR)==1, directly or indirectly. I think it's
best to be honest about it. For example,

>>> +    } else {
>>> +        size = 0;
>>> +    }
>>> +
>>> +    ret = RegSetValueEx(hKey, value, 0, REG_SZ, (LPBYTE)data, size);
>>> +    RegCloseKey(hKey);
>>> +
>>> +out:
>>> +    if (ret != ERROR_SUCCESS) {
>>> +        /* We cannot printf here, and need MessageBox to report an error. */
>>> +        errmsg_dialog(ret, "Cannot add registry ", key);

right here we equate (const char *) with LPCTSTR (by virtue of the third
arg being "key").

You might also replace lstrlen() with strlen() for consistency.

(Tangential anyhow.)

>>> +        return FALSE;
>>> +    }
>>> +    return TRUE;
>>> +}
>>> +
>>> +/* Register this dll as a VSS provider */
>>> +STDAPI DllRegisterServer(void)
>>> +{
>>> +    IVssAdmin *pVssAdmin = NULL;
>>> +    HRESULT hr = E_FAIL;
>>> +    char dllPath[MAX_PATH];
>>> +    char key[256];
>>> +
>>> +    CoInitialize(NULL);
>>> +
>>> +    if (!g_hinstDll) {
>>> +        errmsg_dialog(hr, "Module instance is not available");
>>> +        goto out;
>>> +    }
>>> +
>>> +    /* Add this module to registery */
>>> +
>>> +    sprintf(key, "CLSID\\%s", g_szClsid);
>>> +    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
>>> +        errmsg_dialog(GetLastError(), "GetModuleFileName failed");
>>> +        goto out;
>>> +    }
>>> +    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
>>> +    if (!CreateRegistryKey(key, NULL, dllPath)) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);

(you could reuse "key" from the previous sprintf())

>>> +    if (!CreateRegistryKey(key, "ThreadingModel", "Apartment")) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    sprintf(key, "CLSID\\%s\\ProgID", g_szClsid);
>>> +    if (!CreateRegistryKey(key, NULL, g_szProgid)) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (!CreateRegistryKey(g_szProgid, NULL, QGA_PROVIDER_NAME)) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    sprintf(key, "%s\\CLSID", g_szProgid);
>>> +    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    hr = CoCreateInstance(CLSID_VSSCoordinator,
>>> +        NULL, CLSCTX_ALL, IID_IVssAdmin, (void **)&pVssAdmin);

(indentation)

>>> +    if (FAILED(hr)) {
>>> +        errmsg_dialog(hr, "CoCreateInstance(VSSCoordinator) failed");
>>> +        goto out;
>>> +    }
>>> +
>>> +    hr = pVssAdmin->RegisterProvider(
>>> +        g_gProviderId, CLSID_QGAVSSProvider,
>>> +        const_cast<WCHAR*>(QGA_PROVIDER_LNAME), VSS_PROV_SOFTWARE,
>>> +        const_cast<WCHAR*>(QGA_PROVIDER_VERSION), g_gProviderVersion);

(indentation)

>>> +    if (FAILED(hr)) {
>>> +        errmsg_dialog(hr, "RegisterProvider failed");
>>> +        goto out;

(goto unnecessary)

>>> +    }
>>> +
>>> +out:
>>> +    if (pVssAdmin) {
>>> +        pVssAdmin->Release();
>>> +    }
>>> +    CoUninitialize();
>>> +
>>> +    if (FAILED(hr)) {
>>> +        DllUnregisterServer();
>>> +    }
>>> +
>>> +    return hr;
>>> +}
>>> +
>>> +/* Unregister this VSS hardware provider from the system */
>>> +STDAPI DllUnregisterServer(void)
>>> +{
>>> +    TCHAR key[256];
>>> +    IVssAdmin *pVssAdmin = NULL;
>>> +
>>> +    CoInitialize(NULL);
>>> +
>>> +    HRESULT hr = CoCreateInstance(CLSID_VSSCoordinator,
>>> +         NULL, CLSCTX_ALL, IID_IVssAdmin, (void **)&pVssAdmin);

(indentation, maybe)

>>> +    if (SUCCEEDED(hr)) {
>>> +        hr = pVssAdmin->UnregisterProvider(g_gProviderId);
>>> +        pVssAdmin->Release();
>>> +    } else {
>>> +        errmsg_dialog(hr, "CoCreateInstance(VSSCoordinator) failed");
>>> +    }
>>> +
>>> +    sprintf(key, "CLSID\\%s", g_szClsid);
>>> +    SHDeleteKey(HKEY_CLASSES_ROOT, key);
>>> +    SHDeleteKey(HKEY_CLASSES_ROOT, g_szProgid);
>>> +
>>> +    CoUninitialize();
>>> +
>>> +    return S_OK; /* Uninstall should never fail */
>>> +}

Seems OK.

>>> +
>>> +
>>> +/* Support functions for _bstr_t in MinGW: Originally written by: Diaa Sami */
>>> +

Where does this code originate from? What is its license?

>>> +void __stdcall _com_issue_error(HRESULT hr)
>>> +{
>>> +    printf("_com_issue_error() called with parameter HRESULT = %lu", hr);
>>> +}

This wouldn't be hard to reimplement anyway, just print the message to
stderr. Plus it's missing \n.

I googled the function name, and some people put up a message box here.
Not sure under what circumstances this function is called.


>>> +
>>> +namespace _com_util
>>> +{
>>> +    char * __stdcall ConvertBSTRToString(BSTR bstr)
>>> +    {
>>> +        const unsigned int stringLength = lstrlenW(bstr);
>>> +        char *const ascii = new char [stringLength + 1];
>>> +
>>> +        wcstombs(ascii, bstr, stringLength + 1);
>>> +
>>> +        return ascii;
>>> +    }

The BSTR, _bstr_t, LPCTSTR etc mess is incredible. Is BSTR just
(wchar_t*)?

These COM interfaces seem broken by the way; how does one report a
conversion error? wcstombs() is locale-dependent and can fail. I'll just
pretend that whatever "bstr" contains in UTF-16 will always be
representable in pure ASCII.

>>> +
>>> +    BSTR __stdcall ConvertStringToBSTR(const char *const ascii)
>>> +    {
>>> +        const unsigned int stringLength = lstrlenA(ascii);
>>> +        BSTR bstr = SysAllocStringLen(NULL, stringLength);
>>> +
>>> +        mbstowcs(bstr, ascii, stringLength + 1);
>>> +
>>> +        return bstr;
>>> +    }
>>> +}


>>> diff --git a/qga/vss-win32-provider/provider.cpp b/qga/vss-win32-provider/provider.cpp
>>> new file mode 100644
>>> index 0000000..90a3d25
>>> --- /dev/null
>>> +++ b/qga/vss-win32-provider/provider.cpp
>>> @@ -0,0 +1,479 @@
>>> +/*
>>> + * QEMU Guest Agent win32 VSS Provider implementations
>>> + *
>>> + * Copyright Hitachi Data Systems Corp. 2013
>>> + *
>>> + * Authors:
>>> + *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#include <stdio.h>
>>> +#include "../vss-win32.h"
>>> +#include "inc/win2003/vscoordint.h"
>>> +#include "inc/win2003/vsprov.h"
>>> +
>>> +static long g_nComObjsInUse;
>>> +HINSTANCE g_hinstDll;
>>> +
>>> +/* VSS common GUID's */
>>> +
>>> +const CLSID CLSID_VSSCoordinator = { 0xE579AB5F, 0x1CC4, 0x44b4,
>>> +    {0xBE, 0xD9, 0xDE, 0x09, 0x91, 0xFF, 0x06, 0x23} };
>>> +const IID IID_IVssAdmin = { 0x77ED5996, 0x2F63, 0x11d3,
>>> +    {0x8A, 0x39, 0x00, 0xC0, 0x4F, 0x72, 0xD8, 0xE3} };
>>> +
>>> +const IID IID_IVssHardwareSnapshotProvider = { 0x9593A157, 0x44E9, 0x4344,
>>> +    {0xBB, 0xEB, 0x44, 0xFB, 0xF9, 0xB0, 0x6B, 0x10} };
>>> +const IID IID_IVssSoftwareSnapshotProvider = { 0x609e123e, 0x2c5a, 0x44d3,
>>> +    {0x8f, 0x01, 0x0b, 0x1d, 0x9a, 0x47, 0xd1, 0xff} };
>>> +const IID IID_IVssProviderCreateSnapshotSet = { 0x5F894E5B, 0x1E39, 0x4778,
>>> +    {0x8E, 0x23, 0x9A, 0xBA, 0xD9, 0xF0, 0xE0, 0x8C} };
>>> +const IID IID_IVssProviderNotifications = { 0xE561901F, 0x03A5, 0x4afe,
>>> +    {0x86, 0xD0, 0x72, 0xBA, 0xEE, 0xCE, 0x70, 0x04} };
>>> +
>>> +const IID IID_IVssEnumObject = { 0xAE1C7110, 0x2F60, 0x11d3,
>>> +    {0x8A, 0x39, 0x00, 0xC0, 0x4F, 0x72, 0xD8, 0xE3} };
>>> +
>>> +
>>> +void LockModule(BOOL block)

Did you mean "lock" instead of "block"?

>>> +{
>>> +    if (block) {
>>> +        InterlockedIncrement(&g_nComObjsInUse);
>>> +    } else {
>>> +        InterlockedDecrement(&g_nComObjsInUse);
>>> +    }
>>> +}
>>> +
>>> +/* Empty enumerator for VssObject */
>>> +
>>> +class CQGAVSSEnumObject : public IVssEnumObject
>>> +{
>>> +public:
>>> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppObj);
>>> +    STDMETHODIMP_(ULONG) AddRef();
>>> +    STDMETHODIMP_(ULONG) Release();
>>> +
>>> +    /* IVssEnumObject Methods */
>>> +    STDMETHODIMP Next(
>>> +        ULONG celt, VSS_OBJECT_PROP *rgelt, ULONG *pceltFetched);
>>> +    STDMETHODIMP Skip(ULONG celt);
>>> +    STDMETHODIMP Reset(void);
>>> +    STDMETHODIMP Clone(IVssEnumObject **ppenum);
>>> +
>>> +    /* CQGAVSSEnumObject Methods */
>>> +    CQGAVSSEnumObject();
>>> +    ~CQGAVSSEnumObject();
>>> +
>>> +private:
>>> +    long m_nRefCount;
>>> +};
>>> +
>>> +CQGAVSSEnumObject::CQGAVSSEnumObject()
>>> +{
>>> +    m_nRefCount = 0;

I think idiomatic C++ might want to put it on the member initializer
list, but this way is correct as well, and maybe more understandable
(and I asked for minimizing C++ features :)) so OK.

>>> +    LockModule(TRUE);
>>> +}
>>> +
>>> +CQGAVSSEnumObject::~CQGAVSSEnumObject()
>>> +{
>>> +    LockModule(FALSE);
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVSSEnumObject::QueryInterface(REFIID riid, void **ppObj)
>>> +{
>>> +    if (riid == IID_IUnknown || riid == IID_IVssEnumObject) {
>>> +        *ppObj = static_cast<void*>(static_cast<IVssEnumObject*>(this));

Storing the address of the base object, right?

Actually (based on what the other class does below), do you think it's
right to return the base object's address for IID_IUnknown too, rather
than static_cast<void*>(this)? ... We have a single base object here so
in practice these addresses should be the same.

>>> +        AddRef();
>>> +        return S_OK;
>>> +    }
>>> +    ppObj = NULL;

Indirection operator missing ("*ppObj = NULL")?

>>> +    return E_NOINTERFACE;
>>> +}
>>> +
>>> +STDMETHODIMP_(ULONG) CQGAVSSEnumObject::AddRef()
>>> +{
>>> +    return InterlockedIncrement(&m_nRefCount);
>>> +}
>>> +
>>> +STDMETHODIMP_(ULONG) CQGAVSSEnumObject::Release()
>>> +{
>>> +    long nRefCount = InterlockedDecrement(&m_nRefCount);
>>> +    if (m_nRefCount == 0) {
>>> +        delete this;

I guess we're dead sure the object was allocated with "new".

The destructor invoked here may also remove the last reference to the
module.

No further access to this->XXX is allowed.

http://www.parashift.com/c++-faq-lite/delete-this.html

OK.

>>> +    }
>>> +    return nRefCount;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVSSEnumObject::Next(
>>> +    ULONG celt, VSS_OBJECT_PROP *rgelt, ULONG *pceltFetched)
>>> +{
>>> +    *pceltFetched = 0;
>>> +    return S_FALSE;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVSSEnumObject::Skip(ULONG celt)
>>> +{
>>> +    return S_FALSE;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVSSEnumObject::Reset(void)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVSSEnumObject::Clone(IVssEnumObject **ppenum)
>>> +{
>>> +    return E_NOTIMPL;
>>> +}

(This class seems to track references to the module and do nothing
else.)

>>> +
>>> +
>>> +/* QGAVssProvider */
>>> +
>>> +class CQGAVssProvider :
>>> +    public IVssSoftwareSnapshotProvider,
>>> +    public IVssProviderCreateSnapshotSet,
>>> +    public IVssProviderNotifications
>>> +{
>>> +public:
>>> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppObj);
>>> +    STDMETHODIMP_(ULONG) AddRef();
>>> +    STDMETHODIMP_(ULONG) Release();
>>> +
>>> +    /* IVssSoftwareSnapshotProvider Methods */
>>> +    STDMETHODIMP SetContext(LONG lContext);
>>> +    STDMETHODIMP GetSnapshotProperties(
>>> +        VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp);
>>> +    STDMETHODIMP Query(
>>> +        VSS_ID QueriedObjectId, VSS_OBJECT_TYPE eQueriedObjectType,
>>> +        VSS_OBJECT_TYPE eReturnedObjectsType, IVssEnumObject **ppEnum);
>>> +    STDMETHODIMP DeleteSnapshots(
>>> +        VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
>>> +        BOOL bForceDelete, LONG *plDeletedSnapshots,
>>> +        VSS_ID *pNondeletedSnapshotID);
>>> +    STDMETHODIMP BeginPrepareSnapshot(
>>> +        VSS_ID SnapshotSetId, VSS_ID SnapshotId,
>>> +        VSS_PWSZ pwszVolumeName, LONG lNewContext);
>>> +    STDMETHODIMP IsVolumeSupported(
>>> +        VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider);
>>> +    STDMETHODIMP IsVolumeSnapshotted(
>>> +        VSS_PWSZ pwszVolumeName, BOOL *pbSnapshotsPresent,
>>> +        LONG *plSnapshotCompatibility);
>>> +    STDMETHODIMP SetSnapshotProperty(
>>> +        VSS_ID SnapshotId, VSS_SNAPSHOT_PROPERTY_ID eSnapshotPropertyId,
>>> +        VARIANT vProperty);
>>> +    STDMETHODIMP RevertToSnapshot(VSS_ID SnapshotId);
>>> +    STDMETHODIMP QueryRevertStatus(VSS_PWSZ pwszVolume, IVssAsync **ppAsync);
>>> +
>>> +    /* IVssProviderCreateSnapshotSet Methods */
>>> +    STDMETHODIMP EndPrepareSnapshots(VSS_ID SnapshotSetId);
>>> +    STDMETHODIMP PreCommitSnapshots(VSS_ID SnapshotSetId);
>>> +    STDMETHODIMP CommitSnapshots(VSS_ID SnapshotSetId);
>>> +    STDMETHODIMP PostCommitSnapshots(
>>> +        VSS_ID SnapshotSetId, LONG lSnapshotsCount);
>>> +    STDMETHODIMP PreFinalCommitSnapshots(VSS_ID SnapshotSetId);
>>> +    STDMETHODIMP PostFinalCommitSnapshots(VSS_ID SnapshotSetId);
>>> +    STDMETHODIMP AbortSnapshots(VSS_ID SnapshotSetId);
>>> +
>>> +    /* IVssProviderNotifications Methods */
>>> +    STDMETHODIMP OnLoad(IUnknown *pCallback);
>>> +    STDMETHODIMP OnUnload(BOOL bForceUnload);
>>> +
>>> +    /* CQGAVssProvider Methods */
>>> +    CQGAVssProvider();
>>> +    ~CQGAVssProvider();
>>> +
>>> +private:
>>> +    long m_nRefCount;
>>> +};
>>> +
>>> +CQGAVssProvider::CQGAVssProvider()
>>> +{
>>> +    m_nRefCount = 0;
>>> +    LockModule(TRUE);
>>> +}
>>> +
>>> +CQGAVssProvider::~CQGAVssProvider()
>>> +{
>>> +    LockModule(FALSE);
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::QueryInterface(REFIID riid, void **ppObj)
>>> +{
>>> +    if (riid == IID_IUnknown) {
>>> +        *ppObj = static_cast<void*>(this);
>>> +        AddRef();
>>> +        return S_OK;
>>> +    } else if (riid == IID_IVssSoftwareSnapshotProvider) {

(No "else" needed if "return" is the last statement in the "if"'s
block.)


>>> +        *ppObj = static_cast<void*>(
>>> +            static_cast<IVssSoftwareSnapshotProvider*>(this));
>>> +        AddRef();
>>> +        return S_OK;
>>> +    } else if (riid == IID_IVssProviderCreateSnapshotSet) {
>>> +        *ppObj = static_cast<void*>(
>>> +            static_cast<IVssProviderCreateSnapshotSet*>(this));
>>> +        AddRef();
>>> +        return S_OK;
>>> +    } else if (riid == IID_IVssProviderNotifications) {
>>> +        *ppObj = static_cast<void*>(
>>> +            static_cast<IVssProviderNotifications*>(this));
>>> +        AddRef();
>>> +        return S_OK;
>>> +    }
>>> +    *ppObj = NULL;
>>> +    return E_NOINTERFACE;
>>> +}

Seems OK. Could be reworked into a switch too to save some space, maybe,
but don't bother.


>>> +
>>> +STDMETHODIMP_(ULONG) CQGAVssProvider::AddRef()
>>> +{
>>> +    return InterlockedIncrement(&m_nRefCount);
>>> +}
>>> +
>>> +STDMETHODIMP_(ULONG) CQGAVssProvider::Release()
>>> +{
>>> +    long nRefCount = InterlockedDecrement(&m_nRefCount);
>>> +    if (m_nRefCount == 0) {
>>> +        delete this;
>>> +    }
>>> +    return nRefCount;
>>> +}
>>> +
>>> +
>>> +/*
>>> + * IVssSoftwareSnapshotProvider methods
>>> + */
>>> +
>>> +STDMETHODIMP CQGAVssProvider::SetContext(LONG lContext)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::GetSnapshotProperties(
>>> +    VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp)
>>> +{
>>> +    return VSS_E_OBJECT_NOT_FOUND;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::Query(
>>> +    VSS_ID QueriedObjectId, VSS_OBJECT_TYPE eQueriedObjectType,
>>> +    VSS_OBJECT_TYPE eReturnedObjectsType, IVssEnumObject **ppEnum)
>>> +{
>>> +    *ppEnum = new CQGAVSSEnumObject;
>>> +    (*ppEnum)->AddRef();
>>> +    return S_OK;
>>> +}

OK, so the pattern is, refcount is always incremented in QueryXXXX(), no
matter whether we return the address of one of our own base objects (in
which case we increment our own refcount), or we create a new object
(strictly with "new") and increment the refcount to 1 on that. The
caller of QueryXXXX() is responsible for calling Release() later.

>>> +
>>> +STDMETHODIMP CQGAVssProvider::DeleteSnapshots(
>>> +    VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
>>> +    BOOL bForceDelete, LONG *plDeletedSnapshots, VSS_ID *pNondeletedSnapshotID)
>>> +{
>>> +    return E_NOTIMPL;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot(
>>> +    VSS_ID SnapshotSetId, VSS_ID SnapshotId,
>>> +    VSS_PWSZ pwszVolumeName, LONG lNewContext)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::IsVolumeSupported(
>>> +    VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider)
>>> +{
>>> +    *pbSupportedByThisProvider = TRUE;
>>> +
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::IsVolumeSnapshotted(VSS_PWSZ pwszVolumeName,
>>> +    BOOL *pbSnapshotsPresent, LONG *plSnapshotCompatibility)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::SetSnapshotProperty(VSS_ID SnapshotId,
>>> +    VSS_SNAPSHOT_PROPERTY_ID eSnapshotPropertyId, VARIANT vProperty)
>>> +{
>>> +    return E_NOTIMPL;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::RevertToSnapshot(VSS_ID SnapshotId)
>>> +{
>>> +    return E_NOTIMPL;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::QueryRevertStatus(
>>> +    VSS_PWSZ pwszVolume, IVssAsync **ppAsync)
>>> +{
>>> +    return S_OK;
>>> +}

Shouldn't you set *ppAsync to something here? (No idea, just asking --
S_OK could imply something on output.) Same for IsVolumeSnapshotted()
above.

>>> +
>>> +
>>> +/*
>>> + * IVssProviderCreateSnapshotSet methods
>>> + */
>>> +
>>> +STDMETHODIMP CQGAVssProvider::EndPrepareSnapshots(VSS_ID SnapshotSetId)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::PreCommitSnapshots(VSS_ID SnapshotSetId)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId)
>>> +{
>>> +    HRESULT hr = S_OK;
>>> +    HANDLE hEvent, hEvent2;
>>> +
>>> +    hEvent = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_FROZEN);
>>> +    if (hEvent == INVALID_HANDLE_VALUE) {
>>> +        hr = E_FAIL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    hEvent2 = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_THAW);
>>> +    if (hEvent == INVALID_HANDLE_VALUE) {
>>> +        CloseHandle(hEvent);
>>> +        hr = E_FAIL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    SetEvent(hEvent);

I think we should add a comment here -- this is where qemu-ga.exe /
libvirt will create the snapshot.

>>> +    if (WaitForSingleObject(hEvent2, 60*1000) != WAIT_OBJECT_0) {
>>> +        hr = E_ABORT;
>>> +    }
>>> +
>>> +    CloseHandle(hEvent2);
>>> +    CloseHandle(hEvent);
>>> +out:
>>> +    return hr;
>>> +}

Seems correct in general.

However I think you could shave off a few lines from this function by
reorganizing the "out:" path, for example by moving CloseHandle(hEvent)
there, and replacing the first goto with a return -- currently the
goto's actually waste space; even plain returns would be more
compressed.

Second, maybe the magic constant 60*1000 (known from the VSS docs)
should be a macro.


>>> +
>>> +STDMETHODIMP CQGAVssProvider::PostCommitSnapshots(
>>> +    VSS_ID SnapshotSetId, LONG lSnapshotsCount)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::PreFinalCommitSnapshots(VSS_ID SnapshotSetId)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::PostFinalCommitSnapshots(VSS_ID SnapshotSetId)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::AbortSnapshots(VSS_ID SnapshotSetId)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +/*
>>> + * IVssProviderNotifications methods
>>> + */
>>> +
>>> +STDMETHODIMP CQGAVssProvider::OnLoad(IUnknown *pCallback)
>>> +{
>>> +    return S_OK;
>>> +}
>>> +
>>> +STDMETHODIMP CQGAVssProvider::OnUnload(BOOL bForceUnload)
>>> +{
>>> +    return S_OK;
>>> +}

(Side question: are these methods all abstract in the base class? Can we
save a few LOCs by simply inheriting some functions?)

>>> +
>>> +
>>> +/*
>>> + * CQGAVssProviderFactory class
>>> + */
>>> +
>>> +class CQGAVssProviderFactory : public IClassFactory
>>> +{
>>> +public:
>>> +    STDMETHODIMP QueryInterface(REFIID riid, void **ppv);
>>> +    STDMETHODIMP_(ULONG) AddRef();
>>> +    STDMETHODIMP_(ULONG) Release();
>>> +    STDMETHODIMP CreateInstance(
>>> +        IUnknown *pUnknownOuter, REFIID iid, void **ppv);
>>> +    STDMETHODIMP LockServer(BOOL bLock) { return E_NOTIMPL; }
>>> +private:
>>> +    long m_nRefCount;
>>> +};
>>> +
>>> +STDMETHODIMP CQGAVssProviderFactory::QueryInterface(REFIID riid, void **ppv)
>>> +{
>>> +    if (riid == IID_IUnknown || riid == IID_IClassFactory) {
>>> +        *ppv = static_cast<void*>(this);

Again, in practice the pointer value should be correct for both
interfaces (single inheritance, only one base object), but I'd feel
safer if we had two static casts. Does the C++ standard (or some C++
ABI) guarantee that the first base object is always at offset 0 in the
derived object? (In C, the standard requires that there be no padding at
the beginning of a structure.)


>>> +        AddRef();
>>> +        return S_OK;
>>> +    }
>>> +    *ppv = NULL;
>>> +    return E_NOINTERFACE;
>>> +}
>>> +
>>> +STDMETHODIMP_(ULONG) CQGAVssProviderFactory::AddRef()
>>> +{
>>> +    LockModule(TRUE);
>>> +    return 2;
>>> +}
>>> +
>>> +STDMETHODIMP_(ULONG) CQGAVssProviderFactory::Release()
>>> +{
>>> +    LockModule(FALSE);
>>> +    return 1;
>>> +}

Would it be preferable to change the prototype of LockModule() to return
the post-increment / post-decrement value of "g_nComObjsInUse" (ie.
whatever InterlockedIncrement() or InterlockedDecrement() returns in
LockModule()), and just forward that retval in these two functions? The
values "2" and "1" seem quite arbitrary.

Also, the private data member "m_nRefCount" is unused. This class has
reference counting but no constructor or destructor.


>>> +
>>> +STDMETHODIMP CQGAVssProviderFactory::CreateInstance(
>>> +    IUnknown *pUnknownOuter, REFIID iid, void **ppv)
>>> +{
>>> +    if (pUnknownOuter) {
>>> +        return CLASS_E_NOAGGREGATION;
>>> +    }
>>> +    CQGAVssProvider *pObj = new CQGAVssProvider;
>>> +    if (!pObj) {
>>> +        return E_OUTOFMEMORY;
>>> +    }

(We generally assume that memory allocation never fails.)

>>> +    return pObj->QueryInterface(iid, ppv);

This may leak.

If CQGAVssProvider::QueryInterface() doesn't recognize the requested
interface, it will set *ppv to NULL, and the caller will have no chance
to call Release() on it later. That last bit is actually correct (we
haven't bumped the refcount to 1), but accordingly we should delete pObj
here in that case.


>>> +}
>>> +
>>> +
>>> +/*
>>> + * DLL functions
>>> + */
>>> +
>>> +STDAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID *ppv)
>>> +{

(Right, this is the factory factory function. Awesome :/)


>>> +    static CQGAVssProviderFactory factory;
>>> +
>>> +    *ppv = NULL;
>>> +    if (IsEqualCLSID(rclsid, CLSID_QGAVSSProvider)) {
>>> +        return factory.QueryInterface(riid, ppv);
>>> +    }
>>> +    return CLASS_E_CLASSNOTAVAILABLE;
>>> +}

Not sure what to suggest here. I just don't like the factory object
being static *and* having reference counting.

... Basically you translate references to the factory object to
references to the module. I guess I could see the logic in that if you
deleted the "m_nRefCount" member. However the externally visible
AddRef() and Release() return values are broken in any case. Somehow the
existence of AddRef() and Release() seems fundamentally broken for a
static object -- you simply can't go to refcount==0, which would be the
only situation when the DLL could be removed.

What about this:
- factories would be objects allocated with "new",
- real reference counting for them (with constructor and destructor
  too),
- the ctor/dtor would massage LockModule().

In this aspect CQGAVssProviderFactory would work exactly like the
CQGAVssProvider class, and DllGetClassObject() -- the factory factory
method -- would work like CQGAVssProviderFactory::CreateInstance() --
the factory method.

Of course I have no clue how a factory object must be released
officially, I assume though with the usual ->Release() call, which can
be optionally followed by DLL removal. These windows interfaces are
utterly over-engineered.


>>> +
>>> +STDAPI DllCanUnloadNow()
>>> +{
>>> +    return g_nComObjsInUse == 0 ? S_OK : S_FALSE;
>>> +}

Don't you need some kind of atomic or locked read here? We could read a
stale value here. Granted, most stale values would err on the safe side
(ie. read >0 instead of ==0), but in theory the other mistake is
possible, no?

>>> +
>>> +EXTERN_C
>>> +BOOL WINAPI DllMain(HINSTANCE hinstDll, DWORD dwReason, LPVOID lpReserved)
>>> +{
>>> +    switch (dwReason) {
>>> +
>>> +    case DLL_PROCESS_ATTACH:
>>> +        g_hinstDll = hinstDll;
>>> +        DisableThreadLibraryCalls(hinstDll);
>>> +        break;
>>> +    }
>>> +
>>> +    return TRUE;
>>> +}
>>

Seems fine I guess, though an "if" would be more idiomatic.

No more comments for this patch from me. As usual I don't insist on
fixing anything, I only raise points that you might want to address.

Laszlo
Tomoki Sekiyama - June 27, 2013, 10:25 p.m.
On 6/27/13 11:01 , "Laszlo Ersek" <lersek@redhat.com> wrote:

>On 06/26/13 16:32, Laszlo Ersek wrote:
>> On 06/25/13 18:03, Laszlo Ersek wrote:
>>> On 06/06/13 17:06, Tomoki Sekiyama wrote:
>>>>+    chk(pColl->get_Count(&n));
>>>> +    for (i = n - 1; i >= 0; i--) {
>>>> +        chk(pColl->get_Item(i, (IDispatch **)&pObj));
>>>> +        chk(pObj->get_Value(_bstr_t(L"Name"), &var));
>>>> +        if (var == _variant_t(QGA_PROVIDER_LNAME)) {
>>>> +            printf("Removing COM+ Application: %S\n", (wchar_t
>>>>*)_bstr_t(var));
>
>(stderr)

Sure.

>>>> +            chk(pColl->Remove(i));
>>>> +        }
>
>I think you leak a pObj reference here, at the end of the iteration. The
>next round will set pObj to something else; I think we should call
>pObj->Release() here, and set pObj to NULL (for the case when this is
>the last iteration).

I will add "pObj->Release(); pObj = NULL;" here.

>I'm not sure if you're allowed to call pObj->Release() after the
>pColl()->Remove(i) call. So maybe call pObj->Release() in an else
>branch. (In this case however the out: logic should be modified as
>well.)

It's safe to call Release() after Remove(), by reference counter.

...

>>>> +static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR
>>>>data)
>>>> +{
>>>> +    HKEY  hKey;
>>>> +    LONG  ret;
>>>> +    DWORD size;
>>>> +
>>>> +    ret = RegCreateKeyEx(HKEY_CLASSES_ROOT, key, 0, NULL,
>>>> +        REG_OPTION_NON_VOLATILE, KEY_WRITE, NULL, &hKey, NULL);
>>>> +    if (ret != ERROR_SUCCESS) {
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    if (data != NULL) {
>>>> +        size = (lstrlen(data) + 1) * sizeof(TCHAR);
>
>I think we should drop the multiplication by sizeof(TCHAR), it's 1.
>According to MSDN, TCHAR could be something wider than "char" (ie.
>WCHAR) "for Unicode platforms".
>
>However in all your CreateRegistryKey() calls, the argument passed as
>"data" depends on sizeof(TCHAR)==1, directly or indirectly. I think it's
>best to be honest about it. For example,
>
>>>> +    } else {
>>>> +        size = 0;
>>>> +    }
>>>> +
>>>> +    ret = RegSetValueEx(hKey, value, 0, REG_SZ, (LPBYTE)data, size);
>>>> +    RegCloseKey(hKey);
>>>> +
>>>> +out:
>>>> +    if (ret != ERROR_SUCCESS) {
>>>> +        /* We cannot printf here, and need MessageBox to report an
>>>>error. */
>>>> +        errmsg_dialog(ret, "Cannot add registry ", key);
>
>right here we equate (const char *) with LPCTSTR (by virtue of the third
>arg being "key").
>
>You might also replace lstrlen() with strlen() for consistency.
>
>(Tangential anyhow.)

Right, I assume sizeof(TCHAR) == 1. I will take the simpler way.


>>>>+/* Register this dll as a VSS provider */
>>>> +STDAPI DllRegisterServer(void)
>>>> +{
...
>>>> +    sprintf(key, "CLSID\\%s", g_szClsid);
>>>> +    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
>>>> +        errmsg_dialog(GetLastError(), "GetModuleFileName failed");
>>>> +        goto out;
>>>> +    }
>>>> +    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
>>>> +    if (!CreateRegistryKey(key, NULL, dllPath)) {
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
>
>(you could reuse "key" from the previous sprintf())

OK.

...
> (indentation)
I will fix them.
...

>>>>+/* Support functions for _bstr_t in MinGW: Originally written by:
>>>>Diaa Sami */
>>>> +
>
>Where does this code originate from? What is its license?

This code is from MinGW User ML, and says "use this code however you like,
but don't remove the comment at the beginning".
But I will reimplement these by myself,

>>>> +void __stdcall _com_issue_error(HRESULT hr)
>>>> +{
>>>> +    printf("_com_issue_error() called with parameter HRESULT = %lu",
>>>>hr);
>>>> +}
>
>This wouldn't be hard to reimplement anyway, just print the message to
>stderr. Plus it's missing \n.

Maybe I can use errmsg() here.

>I googled the function name, and some people put up a message box here.
>Not sure under what circumstances this function is called.

It is used inside MinGW headers. I regard this as a user-customized error
notifier function.

>>>> +
>>>> +namespace _com_util
>>>> +{
>>>> +    char * __stdcall ConvertBSTRToString(BSTR bstr)
>>>> +    {
>>>> +        const unsigned int stringLength = lstrlenW(bstr);
>>>> +        char *const ascii = new char [stringLength + 1];
>>>> +
>>>> +        wcstombs(ascii, bstr, stringLength + 1);
>>>> +
>>>> +        return ascii;
>>>> +    }
>
>The BSTR, _bstr_t, LPCTSTR etc mess is incredible. Is BSTR just
>(wchar_t*)?

BSTR is usable like wchar_t*, but needs special allocator which prepend
size information at the memory area pointed.

>These COM interfaces seem broken by the way; how does one report a
>conversion error? wcstombs() is locale-dependent and can fail. I'll just
>pretend that whatever "bstr" contains in UTF-16 will always be
>representable in pure ASCII.

Hmm, althogh I believe only ASCII strings (which appear in this source) is
passed to this function, I will add some conversion error outputs.

Also, qemu-ga.exe actually use only ConvertStringToBSTR, so I will
omit ConvertBSTRToString.



>>>>+void LockModule(BOOL block)
>
>Did you mean "lock" instead of "block"?

Ah, it was bLock ... But will use lock here.

>>>>+STDMETHODIMP CQGAVSSEnumObject::QueryInterface(REFIID riid, void
>>>>**ppObj)
>>>> +{
>>>> +    if (riid == IID_IUnknown || riid == IID_IVssEnumObject) {
>>>> +        *ppObj =
>>>>static_cast<void*>(static_cast<IVssEnumObject*>(this));
>
>Storing the address of the base object, right?

Yes.

>Actually (based on what the other class does below), do you think it's
>right to return the base object's address for IID_IUnknown too, rather
>than static_cast<void*>(this)? ... We have a single base object here so
>in practice these addresses should be the same.

Both is OK I think, if IUnknown pointer value is valid to call
QueryInterface, AddRef and Release virtual methods, and is always the same
value for the same object to distinguish two objects.

>>>> +        AddRef();
>>>> +        return S_OK;
>>>> +    }
>>>> +    ppObj = NULL;
>
>Indirection operator missing ("*ppObj = NULL")?

Oops, I will fix this.

>>>>+STDMETHODIMP_(ULONG) CQGAVSSEnumObject::Release()
>>>> +{
>>>> +    long nRefCount = InterlockedDecrement(&m_nRefCount);
>>>> +    if (m_nRefCount == 0) {
>>>> +        delete this;
>
>I guess we're dead sure the object was allocated with "new".

Yes, it is only created by CQGAVssProvider::Query();

>>>>+STDMETHODIMP CQGAVssProvider::QueryInterface(REFIID riid, void
>>>>**ppObj)
>>>> +{
>>>> +    if (riid == IID_IUnknown) {
>>>> +        *ppObj = static_cast<void*>(this);
>>>> +        AddRef();
>>>> +        return S_OK;
>>>> +    } else if (riid == IID_IVssSoftwareSnapshotProvider) {
>
>(No "else" needed if "return" is the last statement in the "if"'s
>block.)

OK.

...

>>>> +STDMETHODIMP CQGAVssProvider::QueryRevertStatus(
>>>> +    VSS_PWSZ pwszVolume, IVssAsync **ppAsync)
>>>> +{
>>>> +    return S_OK;
>>>> +}
>
>Shouldn't you set *ppAsync to something here? (No idea, just asking --
>S_OK could imply something on output.)

This should be E_NOTIMPL, because we don't support revert via VSS.

>Same for IsVolumeSnapshotted() above.

Right, I will add return values.


>>>> +STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId)
>>>> +{
...
>>>>+    SetEvent(hEvent);
>
>I think we should add a comment here -- this is where qemu-ga.exe /
>libvirt will create the snapshot.

OK.

>>>> +    if (WaitForSingleObject(hEvent2, 60*1000) != WAIT_OBJECT_0) {
>>>> +        hr = E_ABORT;
>>>> +    }
>>>> +
>>>> +    CloseHandle(hEvent2);
>>>> +    CloseHandle(hEvent);
>>>> +out:
>>>> +    return hr;
>>>> +}
>
>Seems correct in general.
>
>However I think you could shave off a few lines from this function by
>reorganizing the "out:" path, for example by moving CloseHandle(hEvent)
>there, and replacing the first goto with a return -- currently the
>goto's actually waste space; even plain returns would be more
>compressed.

I will replace goto with return.

>Second, maybe the magic constant 60*1000 (known from the VSS docs)
>should be a macro.

OK.


>>>> +/*
>>>> + * IVssProviderNotifications methods
>>>> + */
>>>> +
>>>> +STDMETHODIMP CQGAVssProvider::OnLoad(IUnknown *pCallback)
>>>> +{
>>>> +    return S_OK;
>>>> +}
>>>> +
>>>> +STDMETHODIMP CQGAVssProvider::OnUnload(BOOL bForceUnload)
>>>> +{
>>>> +    return S_OK;
>>>> +}
>
>(Side question: are these methods all abstract in the base class? Can we
>save a few LOCs by simply inheriting some functions?)

These are all abstract. :/


>>>>+STDMETHODIMP CQGAVssProviderFactory::QueryInterface(REFIID riid, void
>>>>**ppv)
>>>> +{
>>>> +    if (riid == IID_IUnknown || riid == IID_IClassFactory) {
>>>> +        *ppv = static_cast<void*>(this);
>
>Again, in practice the pointer value should be correct for both
>interfaces (single inheritance, only one base object), but I'd feel
>safer if we had two static casts. Does the C++ standard (or some C++
>ABI) guarantee that the first base object is always at offset 0 in the
>derived object? (In C, the standard requires that there be no padding at
>the beginning of a structure.)

OK, I will add double static_cast. (Actually these class methods are
 virtual, this should OK as vtable can be resolved....)


>>>> +        AddRef();
>>>> +        return S_OK;
>>>> +    }
>>>> +    *ppv = NULL;
>>>> +    return E_NOINTERFACE;
>>>> +}
>>>> +
>>>> +STDMETHODIMP_(ULONG) CQGAVssProviderFactory::AddRef()
>>>> +{
>>>> +    LockModule(TRUE);
>>>> +    return 2;
>>>> +}
>>>> +
>>>> +STDMETHODIMP_(ULONG) CQGAVssProviderFactory::Release()
>>>> +{
>>>> +    LockModule(FALSE);
>>>> +    return 1;
>>>> +}
>
>Would it be preferable to change the prototype of LockModule() to return
>the post-increment / post-decrement value of "g_nComObjsInUse" (ie.
>whatever InterlockedIncrement() or InterlockedDecrement() returns in
>LockModule()), and just forward that retval in these two functions? The
>values "2" and "1" seem quite arbitrary.
>
>Also, the private data member "m_nRefCount" is unused. This class has
>reference counting but no constructor or destructor.

Um, I will rework the reference counting for the factory class,
instead of omitting it.

>>>> +
>>>> +STDMETHODIMP CQGAVssProviderFactory::CreateInstance(
>>>> +    IUnknown *pUnknownOuter, REFIID iid, void **ppv)
>>>> +{
>>>> +    if (pUnknownOuter) {
>>>> +        return CLASS_E_NOAGGREGATION;
>>>> +    }
>>>> +    CQGAVssProvider *pObj = new CQGAVssProvider;
>>>> +    if (!pObj) {
>>>> +        return E_OUTOFMEMORY;
>>>> +    }
>
>(We generally assume that memory allocation never fails.)

Ah, OK...

>>>> +    return pObj->QueryInterface(iid, ppv);
>
>This may leak.
>
>If CQGAVssProvider::QueryInterface() doesn't recognize the requested
>interface, it will set *ppv to NULL, and the caller will have no chance
>to call Release() on it later. That last bit is actually correct (we
>haven't bumped the refcount to 1), but accordingly we should delete pObj
>here in that case.

True. I will fix this.

>>>> +STDAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID *ppv)
>>>> +{
>
>(Right, this is the factory factory function. Awesome :/)
>
>>>> +    static CQGAVssProviderFactory factory;
>>>> +
>>>> +    *ppv = NULL;
>>>> +    if (IsEqualCLSID(rclsid, CLSID_QGAVSSProvider)) {
>>>> +        return factory.QueryInterface(riid, ppv);
>>>> +    }
>>>> +    return CLASS_E_CLASSNOTAVAILABLE;
>>>> +}
>
>Not sure what to suggest here. I just don't like the factory object
>being static *and* having reference counting.
>
>... Basically you translate references to the factory object to
>references to the module. I guess I could see the logic in that if you
>deleted the "m_nRefCount" member. However the externally visible
>AddRef() and Release() return values are broken in any case. Somehow the
>existence of AddRef() and Release() seems fundamentally broken for a
>static object -- you simply can't go to refcount==0, which would be the
>only situation when the DLL could be removed.
>
>What about this:
>- factories would be objects allocated with "new",
>- real reference counting for them (with constructor and destructor
>  too),
>- the ctor/dtor would massage LockModule().
>
>In this aspect CQGAVssProviderFactory would work exactly like the
>CQGAVssProvider class, and DllGetClassObject() -- the factory factory
>method -- would work like CQGAVssProviderFactory::CreateInstance() --
>the factory method.

This sounds better than the current implementation.
I will rework like above.

>Of course I have no clue how a factory object must be released
>officially, I assume though with the usual ->Release() call, which can
>be optionally followed by DLL removal. These windows interfaces are
>utterly over-engineered.

Yeah, usually these stuff will be automatically generated by IDE
using C++ templates, but MinGW doesn't support such mechanism.

>>>> +STDAPI DllCanUnloadNow()
>>>> +{
>>>> +    return g_nComObjsInUse == 0 ? S_OK : S_FALSE;
>>>> +}
>
>Don't you need some kind of atomic or locked read here? We could read a
>stale value here. Granted, most stale values would err on the safe side
>(ie. read >0 instead of ==0), but in theory the other mistake is
>possible, no?

MSDN says "Simple reads and writes to properly-aligned 32bit variables
are atomic", and I couldn't find a function to atomic read provided.
(Maybe InterlockedCompareExchange(&g_nComObjsInUse, 0, 0)...?)

>>>> +EXTERN_C
>>>> +BOOL WINAPI DllMain(HINSTANCE hinstDll, DWORD dwReason, LPVOID
>>>>lpReserved)
>>>> +{
>>>> +    switch (dwReason) {
>>>> +
>>>> +    case DLL_PROCESS_ATTACH:
>>>> +        g_hinstDll = hinstDll;
>>>> +        DisableThreadLibraryCalls(hinstDll);
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return TRUE;
>>>> +}
>>>
>
>Seems fine I guess, though an "if" would be more idiomatic.

OK, make this in to if.


I really appreciate your review.

Thnaks,
Tomoki Sekiyama
Paolo Bonzini - June 28, 2013, 7:05 a.m.
Il 28/06/2013 00:25, Tomoki Sekiyama ha scritto:
>>>>> >>>> +STDMETHODIMP CQGAVssProviderFactory::CreateInstance(
>>>>> >>>> +    IUnknown *pUnknownOuter, REFIID iid, void **ppv)
>>>>> >>>> +{
>>>>> >>>> +    if (pUnknownOuter) {
>>>>> >>>> +        return CLASS_E_NOAGGREGATION;
>>>>> >>>> +    }
>>>>> >>>> +    CQGAVssProvider *pObj = new CQGAVssProvider;
>>>>> >>>> +    if (!pObj) {
>>>>> >>>> +        return E_OUTOFMEMORY;
>>>>> >>>> +    }
>> >
>> >(We generally assume that memory allocation never fails.)
> Ah, OK...

Actually, we do because we use g_malloc/g_free.  The functions exit on
memory allocation failure.  I'm not sure the same is true of the new
operator... doesn't it throw an exception on allocation failure (that's
what I vaguely remember)?

Also, this is not running in the context of qemu-ga, so I think it is
better to be more conservative and trap memory allocation failure.


Paolo
Paolo Bonzini - June 28, 2013, 10:40 a.m.
Il 28/06/2013 12:40, Laszlo Ersek ha scritto:
>> > Also, this is not running in the context of qemu-ga, so I think it is
>> > better to be more conservative and trap memory allocation failure.
> In that case other "new" calls must assume the nothrow form too, plus
> other allocation functions should be checked as well (eg.
> SysAllocStringLen(), although its only use might be in the function that
> Tomoki plans to remove anyway).

Agreed (nothrow or try...catch, depending on what's better for the C++
runtime; by the way, Tomoki-san, I suspect this will require libstdc++
when compiled with GCC---can you confirm?).

Thanks,

Paolo
Laszlo Ersek - June 28, 2013, 10:40 a.m.
On 06/28/13 09:05, Paolo Bonzini wrote:
> Il 28/06/2013 00:25, Tomoki Sekiyama ha scritto:
>>>>>>>>>> +STDMETHODIMP CQGAVssProviderFactory::CreateInstance(
>>>>>>>>>> +    IUnknown *pUnknownOuter, REFIID iid, void **ppv)
>>>>>>>>>> +{
>>>>>>>>>> +    if (pUnknownOuter) {
>>>>>>>>>> +        return CLASS_E_NOAGGREGATION;
>>>>>>>>>> +    }
>>>>>>>>>> +    CQGAVssProvider *pObj = new CQGAVssProvider;
>>>>>>>>>> +    if (!pObj) {
>>>>>>>>>> +        return E_OUTOFMEMORY;
>>>>>>>>>> +    }
>>>>
>>>> (We generally assume that memory allocation never fails.)
>> Ah, OK...
> 
> Actually, we do because we use g_malloc/g_free.  The functions exit on
> memory allocation failure.  I'm not sure the same is true of the new
> operator... doesn't it throw an exception on allocation failure (that's
> what I vaguely remember)?

It throws std::bad_alloc on failure. There's another new operator (the
nothrow form) thar returns 0 on failure.

  18.4.1.1 Single-object forms [lib.new.delete.single]; p9:

    [Example:
      T* p1 = new T;          // throws bad_alloc if it fails
      T* p2 = new(nothrow) T; // returns 0 if it fails
    —end example]

(
"nothrow" in the above is std::nothrow, an object with static storage
duration, of type "nothrow_t" -- it's a dummy argument so that operator
new() can have to prototypes. It is passed by const reference.

  18.4 Dynamic memory management [lib.support.dynamic]; p1:

  namespace std {
    class bad_alloc;
    struct nothrow_t {};
    extern const nothrow_t nothrow;
    /* ... */
  }

  void* operator new(std::size_t size) throw(std::bad_alloc);
  void* operator new(std::size_t size, const std::nothrow_t&) throw();
)

As far as I can remember, older C++ implementations had problems with
bad_alloc. I believe though that any gcc release frome after the stone
age should handle this correctly; see also -fcheck-new.

Of course I have no idea what happens when a C++ exception tries to
propagate past a "DLL boundary".


> Also, this is not running in the context of qemu-ga, so I think it is
> better to be more conservative and trap memory allocation failure.

In that case other "new" calls must assume the nothrow form too, plus
other allocation functions should be checked as well (eg.
SysAllocStringLen(), although its only use might be in the function that
Tomoki plans to remove anyway).

Laszlo
Laszlo Ersek - June 28, 2013, 10:44 a.m.
>>>>> +STDAPI DllCanUnloadNow()
>>>>> +{
>>>>> +    return g_nComObjsInUse == 0 ? S_OK : S_FALSE;
>>>>> +}
>>
>> Don't you need some kind of atomic or locked read here? We could read a
>> stale value here. Granted, most stale values would err on the safe side
>> (ie. read >0 instead of ==0), but in theory the other mistake is
>> possible, no?
> 
> MSDN says "Simple reads and writes to properly-aligned 32bit variables
> are atomic", and I couldn't find a function to atomic read provided.
> (Maybe InterlockedCompareExchange(&g_nComObjsInUse, 0, 0)...?)

Alright then. Thanks!
Laszlo
Tomoki Sekiyama - June 28, 2013, 5:18 p.m.
On 6/28/13 6:40 , "Paolo Bonzini" <pbonzini@redhat.com> wrote:

>Il 28/06/2013 12:40, Laszlo Ersek ha scritto:
>>> > Also, this is not running in the context of qemu-ga, so I think it is
>>> > better to be more conservative and trap memory allocation failure.
>> In that case other "new" calls must assume the nothrow form too, plus
>> other allocation functions should be checked as well (eg.
>> SysAllocStringLen(), although its only use might be in the function that
>> Tomoki plans to remove anyway).
>
>Agreed (nothrow or try...catch, depending on what's better for the C++
>runtime;

I tried something like 'main(){ for(;;) new int[999]; }' and it caused:

Terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc

abnormal program termination


I should catch and handle this.

> by the way, Tomoki-san, I suspect this will require libstdc++
>when compiled with GCC---can you confirm?).

Yes, we need "libstdc++-6.dll" from MinGW to run qemu-ga.exe.


>Thanks,
>
>Paolo

Thanks,
Tomoki Sekiyama
Gal Hammer - June 30, 2013, 1:16 p.m.
On 26/06/2013 01:31, Tomoki Sekiyama wrote:

>> Also, is it needed to call VSSCheckOSVersion from the requestor?  I
>> would think that checking VSSAPI.DLL is stronger than checking the
>> version, and indeed you do that check too.
>
> In Windows XP, VSSAPI.DLL exists but it has different functionality
> and interfaces from newer Windows.
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa384627(v=vs.85).aspx
>
> It is checking the OS version because this patchset only supports
> Windows 2003 or later.

So why don't you query for the interface you supports rather than 
checking the OS version?

Thanks,

     Gal.

> Thanks,
> Tomoki Sekiyama
>
Tomoki Sekiyama - July 1, 2013, 6:57 p.m.
On 6/30/13 9:16 , "Gal Hammer" <ghammer@redhat.com> wrote:

>On 26/06/2013 01:31, Tomoki Sekiyama wrote:
>
>>> Also, is it needed to call VSSCheckOSVersion from the requestor?  I
>>> would think that checking VSSAPI.DLL is stronger than checking the
>>> version, and indeed you do that check too.
>>
>> In Windows XP, VSSAPI.DLL exists but it has different functionality
>> and interfaces from newer Windows.
>> 
>>http://msdn.microsoft.com/en-us/library/windows/desktop/aa384627(v=vs.85)
>>.aspx
>>
>> It is checking the OS version because this patchset only supports
>> Windows 2003 or later.
>
>So why don't you query for the interface you supports rather than
>checking the OS version?

Hmm, I don't know what I should query for. Microsoft's document above only
provides a table of compatibility between VSS SDK version and OS version.

Of course I can install the component and test whether it works or not,
but I want to know that before installing something.

>Thanks,
>
>     Gal.

Thanks,
Tomoki Sekiyama
Gal Hammer - July 2, 2013, 12:36 p.m.
On 01/07/2013 21:57, Tomoki Sekiyama wrote:

>> So why don't you query for the interface you supports rather than
>> checking the OS version?
>
> Hmm, I don't know what I should query for. Microsoft's document above only
> provides a table of compatibility between VSS SDK version and OS version.
>
> Of course I can install the component and test whether it works or not,
> but I want to know that before installing something.

I'll check it myself and try to suggest a patch.

Sorry for missing the face that it is done during installation and not 
on a "freeze" request.

     Gal.

Patch

diff --git a/Makefile b/Makefile
index 4851ba0..636358d 100644
--- a/Makefile
+++ b/Makefile
@@ -235,7 +235,7 @@  clean:
 	rm -f qemu-options.def
 	find . -name '*.[oda]' -type f -exec rm -f {} +
 	find . -name '*.l[oa]' -type f -exec rm -f {} +
-	rm -f $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
+	rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
 	rm -Rf .libs
 	rm -f qemu-img-cmds.h
 	@# May not be present in GENERATED_HEADERS
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)
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
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
+qga-obj-y += vss-win32-provider/
+qga-prv-obj-y += vss-win32-provider/
+endif
diff --git a/qga/vss-win32-provider.h b/qga/vss-win32-provider.h
new file mode 100644
index 0000000..a437e71
--- /dev/null
+++ b/qga/vss-win32-provider.h
@@ -0,0 +1,26 @@ 
+/*
+ * QEMU Guest Agent win32 VSS provider declarations
+ *
+ * Copyright Hitachi Data Systems Corp. 2013
+ *
+ * Authors:
+ *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
+ *
+ * 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_PROVIDER_H
+#define VSS_WIN32_PROVIDER_H
+
+#include <windows.h>
+
+STDAPI VSSCheckOSVersion(void);
+
+STDAPI COMRegister(void);
+STDAPI COMUnregister(void);
+
+STDAPI DllRegisterServer(void);
+STDAPI DllUnregisterServer(void);
+
+#endif
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)$@")
+
+
+# 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=$(WIN_SDK)/Bin/midl
+
+$(obj)/qga-provider.tlb: $(SRC_PATH)/$(obj)/qga-provider.idl
+ifeq ($(wildcard $(SRC_PATH)/$(obj)/qga-provider.tlb),)
+	$(call quiet-command,$(MIDL) -tlb $@ -I $(WIN_SDK)/Include $<,"  MIDL  $(TARGET_DIR)$@")
+else
+	$(call quiet-command,cp $(dir $<)qga-provider.tlb $@, "  COPY  $(TARGET_DIR)$@")
+endif
diff --git a/qga/vss-win32-provider/install.cpp b/qga/vss-win32-provider/install.cpp
new file mode 100644
index 0000000..d6f1d55
--- /dev/null
+++ b/qga/vss-win32-provider/install.cpp
@@ -0,0 +1,493 @@ 
+/*
+ * QEMU Guest Agent win32 VSS Provider installer
+ *
+ * Copyright Hitachi Data Systems Corp. 2013
+ *
+ * Authors:
+ *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <stdio.h>
+#include <string.h>
+
+#include "../vss-win32.h"
+#include "inc/win2003/vscoordint.h"
+#include "../vss-win32-provider.h"
+
+#include <comadmin.h>
+#include <wbemidl.h>
+#include <comutil.h>
+
+extern HINSTANCE g_hinstDll;
+
+const GUID CLSID_COMAdminCatalog = { 0xF618C514, 0xDFB8, 0x11d1,
+    {0xA2, 0xCF, 0x00, 0x80, 0x5F, 0xC7, 0x92, 0x35} };
+const GUID IID_ICOMAdminCatalog = { 0xDD662187, 0xDFC2, 0x11d1,
+    {0xA2, 0xCF, 0x00, 0x80, 0x5F, 0xC7, 0x92, 0x35} };
+const GUID CLSID_WbemLocator = { 0x4590f811, 0x1d3a, 0x11d0,
+    {0x89, 0x1f, 0x00, 0xaa, 0x00, 0x4b, 0x2e, 0x24} };
+const GUID IID_IWbemLocator = { 0xdc12a687, 0x737f, 0x11cf,
+    {0x88, 0x4d, 0x00, 0xaa, 0x00, 0x4b, 0x2e, 0x24} };
+
+static void errmsg(DWORD err, const char *text)
+{
+    char *msg = NULL, *nul = strchr(text, '(');
+    int len = nul ? nul - text : -1;
+
+    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
+                  FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
+                  NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                  (char *)&msg, 0, NULL);
+    printf("%.*s. (Error: %lx) %s\n", len, text, err, msg);
+    LocalFree(msg);
+}
+
+static void errmsg_dialog(DWORD err, const char *text, const char *opt = "")
+{
+    char *msg, buf[512];
+
+    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
+                  FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
+                  NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                  (char *)&msg, 0, NULL);
+    snprintf(buf, sizeof(buf), "%s%s. (Error: %lx) %s\n", text, opt, err, msg);
+    MessageBox(NULL, buf, "Error from " QGA_PROVIDER_NAME, MB_OK|MB_ICONERROR);
+    LocalFree(msg);
+}
+
+#define _chk(hr, status, msg, err_label)        \
+    do {                                        \
+        hr = (status);                          \
+        if (FAILED(hr)) {                       \
+            errmsg(hr, msg);                    \
+            goto err_label;                     \
+        }                                       \
+    } while (0)
+
+#define chk(status) _chk(hr, status, "Failed to " #status, out)
+
+template<class T>
+HRESULT put_Value(ICatalogObject *pObj, LPCWSTR name, T val)
+{
+    return pObj->put_Value(_bstr_t(name), _variant_t(val));
+}
+
+/* Check whether this OS version supports VSS providers */
+STDAPI VSSCheckOSVersion(void)
+{
+    OSVERSIONINFO OSver;
+
+    OSver.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+    GetVersionEx(&OSver);
+    if ((OSver.dwMajorVersion == 5 && OSver.dwMinorVersion >= 2) ||
+       OSver.dwMajorVersion > 5) {
+        return S_OK;
+    }
+    return S_FALSE;
+}
+
+/* Lookup Administrators group name from winmgmt */
+static HRESULT GetAdminName(_bstr_t &name)
+{
+    HRESULT hr;
+    IWbemLocator *pLoc = NULL;
+    IWbemServices *pSvc = NULL;
+    IEnumWbemClassObject *pEnum = NULL;
+    IWbemClassObject *pWobj = NULL;
+    ULONG returned;
+    _variant_t var;
+
+    chk(CoCreateInstance(CLSID_WbemLocator, NULL, CLSCTX_INPROC_SERVER,
+                         IID_IWbemLocator, (LPVOID *)&pLoc));
+    chk(pLoc->ConnectServer(_bstr_t(L"ROOT\\CIMV2"), NULL, NULL, NULL,
+                            0, 0, 0, &pSvc));
+    chk(CoSetProxyBlanket(pSvc, RPC_C_AUTHN_WINNT, RPC_C_AUTHZ_NONE,
+                          NULL, RPC_C_AUTHN_LEVEL_CALL,
+                          RPC_C_IMP_LEVEL_IMPERSONATE, NULL, EOAC_NONE));
+    chk(pSvc->ExecQuery(_bstr_t(L"WQL"),
+                        _bstr_t(L"select * from Win32_Account where "
+                                "SID='S-1-5-32-544' and localAccount=TRUE"),
+                        WBEM_FLAG_RETURN_IMMEDIATELY | WBEM_FLAG_FORWARD_ONLY,
+                        NULL, &pEnum));
+    if (!pEnum) {
+        errmsg(E_FAIL, "Failed to query for Administrators");
+        goto out;
+    }
+    chk(pEnum->Next(WBEM_INFINITE, 1, &pWobj, &returned));
+    if (returned == 0) {
+        errmsg(E_FAIL, "Failed to query for Administrators");
+        goto out;
+    }
+
+    chk(pWobj->Get(_bstr_t(L"Name"), 0, &var, 0, 0));
+    name = var;
+out:
+    if (pLoc) {
+        pLoc->Release();
+    }
+    if (pSvc) {
+        pSvc->Release();
+    }
+    if (pEnum) {
+        pEnum->Release();
+    }
+    if (pWobj) {
+        pWobj->Release();
+    }
+    return hr;
+}
+
+/* Register this module to COM+ Applications Catalog */
+STDAPI COMRegister(void)
+{
+    HRESULT hr = E_FAIL;
+    IUnknown *pUnknown = NULL;
+    ICOMAdminCatalog *pCatalog = NULL;
+    ICatalogCollection *pApps = NULL, *pRoles = NULL, *pUsersInRole = NULL;
+    ICatalogObject *pObj = NULL;
+    long n;
+    _bstr_t name;
+    _variant_t key;
+    CHAR dllPath[MAX_PATH], tlbPath[MAX_PATH];
+
+    if (!g_hinstDll) {
+        errmsg(E_FAIL, "Failed to initialize DLL");
+        goto out;
+    }
+
+    if (VSSCheckOSVersion() == S_FALSE) {
+        printf("VSS provider is not supported in this OS version.\n");
+        return S_FALSE; /* VSS feature is disabled */
+    }
+
+    COMUnregister();
+
+    chk(CoInitialize(NULL));
+    chk(CoCreateInstance(CLSID_COMAdminCatalog, NULL, CLSCTX_INPROC_SERVER,
+                         IID_IUnknown, (void **)&pUnknown));
+    chk(pUnknown->QueryInterface(IID_ICOMAdminCatalog, (void **)&pCatalog));
+
+    /* Install COM+ Component */
+
+    chk(pCatalog->GetCollection(_bstr_t(L"Applications"),
+                                (IDispatch **)&pApps));
+    chk(pApps->Populate());
+    chk(pApps->Add((IDispatch **)&pObj));
+    chk(put_Value(pObj, L"Name",        QGA_PROVIDER_LNAME));
+    chk(put_Value(pObj, L"Description", QGA_PROVIDER_LNAME));
+    chk(put_Value(pObj, L"ApplicationAccessChecksEnabled", true));
+    chk(put_Value(pObj, L"Authentication",                 short(6)));
+    chk(put_Value(pObj, L"AuthenticationCapability",       short(2)));
+    chk(put_Value(pObj, L"ImpersonationLevel",             short(2)));
+    chk(pApps->SaveChanges(&n));
+    chk(pObj->get_Key(&key));
+
+    pObj->Release();
+    pObj = NULL;
+
+    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
+        errmsg(GetLastError(), "GetModuleFileName failed");
+        goto out;
+    }
+    n = strlen(dllPath);
+    if (n < 3) {
+        errmsg(E_FAIL, "Failed to lookup dll");
+    }
+    strcpy(tlbPath, dllPath);
+    strcpy(tlbPath+n-3, "TLB");
+    printf("Registering " QGA_PROVIDER_NAME ":\n");
+    printf("  %s\n", dllPath);
+    printf("  %s\n", tlbPath);
+    if (!PathFileExists(tlbPath)) {
+        errmsg(ERROR_FILE_NOT_FOUND, "Failed to lookup tlb");
+        goto out;
+    }
+
+    chk(pCatalog->InstallComponent(_bstr_t(QGA_PROVIDER_LNAME),
+                                   _bstr_t(dllPath), _bstr_t(tlbPath),
+                                   _bstr_t("")));
+
+    /* Setup roles of the applicaion */
+
+    chk(pApps->GetCollection(_bstr_t(L"Roles"), key,
+                             (IDispatch **)&pRoles));
+    chk(pRoles->Populate());
+    chk(pRoles->Add((IDispatch **)&pObj));
+    chk(put_Value(pObj, L"Name",        L"Administrators"));
+    chk(put_Value(pObj, L"Description", L"Administrators group"));
+    chk(pRoles->SaveChanges(&n));
+    chk(pObj->get_Key(&key));
+
+    pObj->Release();
+    pObj = NULL;
+
+    /* Setup users in the role */
+
+    chk(pRoles->GetCollection(_bstr_t(L"UsersInRole"), key,
+                              (IDispatch **)&pUsersInRole));
+    chk(pUsersInRole->Populate());
+
+    chk(pUsersInRole->Add((IDispatch **)&pObj));
+    chk(GetAdminName(name));
+    chk(put_Value(pObj, L"User", _bstr_t(".\\") + name));
+
+    pObj->Release();
+    pObj = NULL;
+
+    chk(pUsersInRole->Add((IDispatch **)&pObj));
+    chk(put_Value(pObj, L"User", L"SYSTEM"));
+    chk(pUsersInRole->SaveChanges(&n));
+
+out:
+    if (pUnknown) {
+        pUnknown->Release();
+    }
+    if (pCatalog) {
+        pCatalog->Release();
+    }
+    if (pApps) {
+        pApps->Release();
+    }
+    if (pRoles) {
+        pRoles->Release();
+    }
+    if (pUsersInRole) {
+        pUsersInRole->Release();
+    }
+    if (pObj) {
+        pObj->Release();
+    }
+
+    if (FAILED(hr)) {
+        COMUnregister();
+    }
+    CoUninitialize();
+
+    return hr;
+}
+
+/* Unregister this module from COM+ Applications Catalog */
+STDAPI COMUnregister(void)
+{
+    HRESULT hr;
+    IUnknown *pUnknown = NULL;
+    ICOMAdminCatalog *pCatalog = NULL;
+    ICatalogCollection *pColl = NULL;
+    ICatalogObject *pObj = NULL;
+    _variant_t var;
+    long i, n;
+
+    if (VSSCheckOSVersion() == S_FALSE) {
+        printf("VSS provider is not supported in this OS version.\n");
+        return S_FALSE; /* VSS feature is disabled */
+    }
+
+    chk(DllUnregisterServer());
+
+    chk(CoInitialize(NULL));
+    chk(CoCreateInstance(CLSID_COMAdminCatalog, NULL, CLSCTX_INPROC_SERVER,
+                         IID_IUnknown, (void **)&pUnknown));
+    chk(pUnknown->QueryInterface(IID_ICOMAdminCatalog, (void **)&pCatalog));
+
+    chk(pCatalog->GetCollection(_bstr_t(L"Applications"),
+                                (IDispatch **)&pColl));
+    chk(pColl->Populate());
+
+    chk(pColl->get_Count(&n));
+    for (i = n - 1; i >= 0; i--) {
+        chk(pColl->get_Item(i, (IDispatch **)&pObj));
+        chk(pObj->get_Value(_bstr_t(L"Name"), &var));
+        if (var == _variant_t(QGA_PROVIDER_LNAME)) {
+            printf("Removing COM+ Application: %S\n", (wchar_t *)_bstr_t(var));
+            chk(pColl->Remove(i));
+        }
+    }
+    chk(pColl->SaveChanges(&n));
+
+out:
+    if (pUnknown) {
+        pUnknown->Release();
+    }
+    if (pCatalog) {
+        pCatalog->Release();
+    }
+    if (pColl) {
+        pColl->Release();
+    }
+    if (pObj) {
+        pObj->Release();
+    }
+    CoUninitialize();
+
+    return hr;
+}
+
+
+static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR data)
+{
+    HKEY  hKey;
+    LONG  ret;
+    DWORD size;
+
+    ret = RegCreateKeyEx(HKEY_CLASSES_ROOT, key, 0, NULL,
+        REG_OPTION_NON_VOLATILE, KEY_WRITE, NULL, &hKey, NULL);
+    if (ret != ERROR_SUCCESS) {
+        goto out;
+    }
+
+    if (data != NULL) {
+        size = (lstrlen(data) + 1) * sizeof(TCHAR);
+    } else {
+        size = 0;
+    }
+
+    ret = RegSetValueEx(hKey, value, 0, REG_SZ, (LPBYTE)data, size);
+    RegCloseKey(hKey);
+
+out:
+    if (ret != ERROR_SUCCESS) {
+        /* We cannot printf here, and need MessageBox to report an error. */
+        errmsg_dialog(ret, "Cannot add registry ", key);
+        return FALSE;
+    }
+    return TRUE;
+}
+
+/* Register this dll as a VSS provider */
+STDAPI DllRegisterServer(void)
+{
+    IVssAdmin *pVssAdmin = NULL;
+    HRESULT hr = E_FAIL;
+    char dllPath[MAX_PATH];
+    char key[256];
+
+    CoInitialize(NULL);
+
+    if (!g_hinstDll) {
+        errmsg_dialog(hr, "Module instance is not available");
+        goto out;
+    }
+
+    /* Add this module to registery */
+
+    sprintf(key, "CLSID\\%s", g_szClsid);
+    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
+        goto out;
+    }
+
+    if (!GetModuleFileName(g_hinstDll, dllPath, sizeof(dllPath))) {
+        errmsg_dialog(GetLastError(), "GetModuleFileName failed");
+        goto out;
+    }
+    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
+    if (!CreateRegistryKey(key, NULL, dllPath)) {
+        goto out;
+    }
+
+    sprintf(key, "CLSID\\%s\\InprocServer32", g_szClsid);
+    if (!CreateRegistryKey(key, "ThreadingModel", "Apartment")) {
+        goto out;
+    }
+
+    sprintf(key, "CLSID\\%s\\ProgID", g_szClsid);
+    if (!CreateRegistryKey(key, NULL, g_szProgid)) {
+        goto out;
+    }
+
+    if (!CreateRegistryKey(g_szProgid, NULL, QGA_PROVIDER_NAME)) {
+        goto out;
+    }
+
+    sprintf(key, "%s\\CLSID", g_szProgid);
+    if (!CreateRegistryKey(key, NULL, g_szClsid)) {
+        goto out;
+    }
+
+    hr = CoCreateInstance(CLSID_VSSCoordinator,
+        NULL, CLSCTX_ALL, IID_IVssAdmin, (void **)&pVssAdmin);
+    if (FAILED(hr)) {
+        errmsg_dialog(hr, "CoCreateInstance(VSSCoordinator) failed");
+        goto out;
+    }
+
+    hr = pVssAdmin->RegisterProvider(
+        g_gProviderId, CLSID_QGAVSSProvider,
+        const_cast<WCHAR*>(QGA_PROVIDER_LNAME), VSS_PROV_SOFTWARE,
+        const_cast<WCHAR*>(QGA_PROVIDER_VERSION), g_gProviderVersion);
+    if (FAILED(hr)) {
+        errmsg_dialog(hr, "RegisterProvider failed");
+        goto out;
+    }
+
+out:
+    if (pVssAdmin) {
+        pVssAdmin->Release();
+    }
+    CoUninitialize();
+
+    if (FAILED(hr)) {
+        DllUnregisterServer();
+    }
+
+    return hr;
+}
+
+/* Unregister this VSS hardware provider from the system */
+STDAPI DllUnregisterServer(void)
+{
+    TCHAR key[256];
+    IVssAdmin *pVssAdmin = NULL;
+
+    CoInitialize(NULL);
+
+    HRESULT hr = CoCreateInstance(CLSID_VSSCoordinator,
+         NULL, CLSCTX_ALL, IID_IVssAdmin, (void **)&pVssAdmin);
+    if (SUCCEEDED(hr)) {
+        hr = pVssAdmin->UnregisterProvider(g_gProviderId);
+        pVssAdmin->Release();
+    } else {
+        errmsg_dialog(hr, "CoCreateInstance(VSSCoordinator) failed");
+    }
+
+    sprintf(key, "CLSID\\%s", g_szClsid);
+    SHDeleteKey(HKEY_CLASSES_ROOT, key);
+    SHDeleteKey(HKEY_CLASSES_ROOT, g_szProgid);
+
+    CoUninitialize();
+
+    return S_OK; /* Uninstall should never fail */
+}
+
+
+/* Support functions for _bstr_t in MinGW: Originally written by: Diaa Sami */
+
+void __stdcall _com_issue_error(HRESULT hr)
+{
+    printf("_com_issue_error() called with parameter HRESULT = %lu", hr);
+}
+
+namespace _com_util
+{
+    char * __stdcall ConvertBSTRToString(BSTR bstr)
+    {
+        const unsigned int stringLength = lstrlenW(bstr);
+        char *const ascii = new char [stringLength + 1];
+
+        wcstombs(ascii, bstr, stringLength + 1);
+
+        return ascii;
+    }
+
+    BSTR __stdcall ConvertStringToBSTR(const char *const ascii)
+    {
+        const unsigned int stringLength = lstrlenA(ascii);
+        BSTR bstr = SysAllocStringLen(NULL, stringLength);
+
+        mbstowcs(bstr, ascii, stringLength + 1);
+
+        return bstr;
+    }
+}
diff --git a/qga/vss-win32-provider/provider.cpp b/qga/vss-win32-provider/provider.cpp
new file mode 100644
index 0000000..90a3d25
--- /dev/null
+++ b/qga/vss-win32-provider/provider.cpp
@@ -0,0 +1,479 @@ 
+/*
+ * QEMU Guest Agent win32 VSS Provider implementations
+ *
+ * Copyright Hitachi Data Systems Corp. 2013
+ *
+ * Authors:
+ *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <stdio.h>
+#include "../vss-win32.h"
+#include "inc/win2003/vscoordint.h"
+#include "inc/win2003/vsprov.h"
+
+static long g_nComObjsInUse;
+HINSTANCE g_hinstDll;
+
+/* VSS common GUID's */
+
+const CLSID CLSID_VSSCoordinator = { 0xE579AB5F, 0x1CC4, 0x44b4,
+    {0xBE, 0xD9, 0xDE, 0x09, 0x91, 0xFF, 0x06, 0x23} };
+const IID IID_IVssAdmin = { 0x77ED5996, 0x2F63, 0x11d3,
+    {0x8A, 0x39, 0x00, 0xC0, 0x4F, 0x72, 0xD8, 0xE3} };
+
+const IID IID_IVssHardwareSnapshotProvider = { 0x9593A157, 0x44E9, 0x4344,
+    {0xBB, 0xEB, 0x44, 0xFB, 0xF9, 0xB0, 0x6B, 0x10} };
+const IID IID_IVssSoftwareSnapshotProvider = { 0x609e123e, 0x2c5a, 0x44d3,
+    {0x8f, 0x01, 0x0b, 0x1d, 0x9a, 0x47, 0xd1, 0xff} };
+const IID IID_IVssProviderCreateSnapshotSet = { 0x5F894E5B, 0x1E39, 0x4778,
+    {0x8E, 0x23, 0x9A, 0xBA, 0xD9, 0xF0, 0xE0, 0x8C} };
+const IID IID_IVssProviderNotifications = { 0xE561901F, 0x03A5, 0x4afe,
+    {0x86, 0xD0, 0x72, 0xBA, 0xEE, 0xCE, 0x70, 0x04} };
+
+const IID IID_IVssEnumObject = { 0xAE1C7110, 0x2F60, 0x11d3,
+    {0x8A, 0x39, 0x00, 0xC0, 0x4F, 0x72, 0xD8, 0xE3} };
+
+
+void LockModule(BOOL block)
+{
+    if (block) {
+        InterlockedIncrement(&g_nComObjsInUse);
+    } else {
+        InterlockedDecrement(&g_nComObjsInUse);
+    }
+}
+
+/* Empty enumerator for VssObject */
+
+class CQGAVSSEnumObject : public IVssEnumObject
+{
+public:
+    STDMETHODIMP QueryInterface(REFIID riid, void **ppObj);
+    STDMETHODIMP_(ULONG) AddRef();
+    STDMETHODIMP_(ULONG) Release();
+
+    /* IVssEnumObject Methods */
+    STDMETHODIMP Next(
+        ULONG celt, VSS_OBJECT_PROP *rgelt, ULONG *pceltFetched);
+    STDMETHODIMP Skip(ULONG celt);
+    STDMETHODIMP Reset(void);
+    STDMETHODIMP Clone(IVssEnumObject **ppenum);
+
+    /* CQGAVSSEnumObject Methods */
+    CQGAVSSEnumObject();
+    ~CQGAVSSEnumObject();
+
+private:
+    long m_nRefCount;
+};
+
+CQGAVSSEnumObject::CQGAVSSEnumObject()
+{
+    m_nRefCount = 0;
+    LockModule(TRUE);
+}
+
+CQGAVSSEnumObject::~CQGAVSSEnumObject()
+{
+    LockModule(FALSE);
+}
+
+STDMETHODIMP CQGAVSSEnumObject::QueryInterface(REFIID riid, void **ppObj)
+{
+    if (riid == IID_IUnknown || riid == IID_IVssEnumObject) {
+        *ppObj = static_cast<void*>(static_cast<IVssEnumObject*>(this));
+        AddRef();
+        return S_OK;
+    }
+    ppObj = NULL;
+    return E_NOINTERFACE;
+}
+
+STDMETHODIMP_(ULONG) CQGAVSSEnumObject::AddRef()
+{
+    return InterlockedIncrement(&m_nRefCount);
+}
+
+STDMETHODIMP_(ULONG) CQGAVSSEnumObject::Release()
+{
+    long nRefCount = InterlockedDecrement(&m_nRefCount);
+    if (m_nRefCount == 0) {
+        delete this;
+    }
+    return nRefCount;
+}
+
+STDMETHODIMP CQGAVSSEnumObject::Next(
+    ULONG celt, VSS_OBJECT_PROP *rgelt, ULONG *pceltFetched)
+{
+    *pceltFetched = 0;
+    return S_FALSE;
+}
+
+STDMETHODIMP CQGAVSSEnumObject::Skip(ULONG celt)
+{
+    return S_FALSE;
+}
+
+STDMETHODIMP CQGAVSSEnumObject::Reset(void)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVSSEnumObject::Clone(IVssEnumObject **ppenum)
+{
+    return E_NOTIMPL;
+}
+
+
+/* QGAVssProvider */
+
+class CQGAVssProvider :
+    public IVssSoftwareSnapshotProvider,
+    public IVssProviderCreateSnapshotSet,
+    public IVssProviderNotifications
+{
+public:
+    STDMETHODIMP QueryInterface(REFIID riid, void **ppObj);
+    STDMETHODIMP_(ULONG) AddRef();
+    STDMETHODIMP_(ULONG) Release();
+
+    /* IVssSoftwareSnapshotProvider Methods */
+    STDMETHODIMP SetContext(LONG lContext);
+    STDMETHODIMP GetSnapshotProperties(
+        VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp);
+    STDMETHODIMP Query(
+        VSS_ID QueriedObjectId, VSS_OBJECT_TYPE eQueriedObjectType,
+        VSS_OBJECT_TYPE eReturnedObjectsType, IVssEnumObject **ppEnum);
+    STDMETHODIMP DeleteSnapshots(
+        VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
+        BOOL bForceDelete, LONG *plDeletedSnapshots,
+        VSS_ID *pNondeletedSnapshotID);
+    STDMETHODIMP BeginPrepareSnapshot(
+        VSS_ID SnapshotSetId, VSS_ID SnapshotId,
+        VSS_PWSZ pwszVolumeName, LONG lNewContext);
+    STDMETHODIMP IsVolumeSupported(
+        VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider);
+    STDMETHODIMP IsVolumeSnapshotted(
+        VSS_PWSZ pwszVolumeName, BOOL *pbSnapshotsPresent,
+        LONG *plSnapshotCompatibility);
+    STDMETHODIMP SetSnapshotProperty(
+        VSS_ID SnapshotId, VSS_SNAPSHOT_PROPERTY_ID eSnapshotPropertyId,
+        VARIANT vProperty);
+    STDMETHODIMP RevertToSnapshot(VSS_ID SnapshotId);
+    STDMETHODIMP QueryRevertStatus(VSS_PWSZ pwszVolume, IVssAsync **ppAsync);
+
+    /* IVssProviderCreateSnapshotSet Methods */
+    STDMETHODIMP EndPrepareSnapshots(VSS_ID SnapshotSetId);
+    STDMETHODIMP PreCommitSnapshots(VSS_ID SnapshotSetId);
+    STDMETHODIMP CommitSnapshots(VSS_ID SnapshotSetId);
+    STDMETHODIMP PostCommitSnapshots(
+        VSS_ID SnapshotSetId, LONG lSnapshotsCount);
+    STDMETHODIMP PreFinalCommitSnapshots(VSS_ID SnapshotSetId);
+    STDMETHODIMP PostFinalCommitSnapshots(VSS_ID SnapshotSetId);
+    STDMETHODIMP AbortSnapshots(VSS_ID SnapshotSetId);
+
+    /* IVssProviderNotifications Methods */
+    STDMETHODIMP OnLoad(IUnknown *pCallback);
+    STDMETHODIMP OnUnload(BOOL bForceUnload);
+
+    /* CQGAVssProvider Methods */
+    CQGAVssProvider();
+    ~CQGAVssProvider();
+
+private:
+    long m_nRefCount;
+};
+
+CQGAVssProvider::CQGAVssProvider()
+{
+    m_nRefCount = 0;
+    LockModule(TRUE);
+}
+
+CQGAVssProvider::~CQGAVssProvider()
+{
+    LockModule(FALSE);
+}
+
+STDMETHODIMP CQGAVssProvider::QueryInterface(REFIID riid, void **ppObj)
+{
+    if (riid == IID_IUnknown) {
+        *ppObj = static_cast<void*>(this);
+        AddRef();
+        return S_OK;
+    } else if (riid == IID_IVssSoftwareSnapshotProvider) {
+        *ppObj = static_cast<void*>(
+            static_cast<IVssSoftwareSnapshotProvider*>(this));
+        AddRef();
+        return S_OK;
+    } else if (riid == IID_IVssProviderCreateSnapshotSet) {
+        *ppObj = static_cast<void*>(
+            static_cast<IVssProviderCreateSnapshotSet*>(this));
+        AddRef();
+        return S_OK;
+    } else if (riid == IID_IVssProviderNotifications) {
+        *ppObj = static_cast<void*>(
+            static_cast<IVssProviderNotifications*>(this));
+        AddRef();
+        return S_OK;
+    }
+    *ppObj = NULL;
+    return E_NOINTERFACE;
+}
+
+STDMETHODIMP_(ULONG) CQGAVssProvider::AddRef()
+{
+    return InterlockedIncrement(&m_nRefCount);
+}
+
+STDMETHODIMP_(ULONG) CQGAVssProvider::Release()
+{
+    long nRefCount = InterlockedDecrement(&m_nRefCount);
+    if (m_nRefCount == 0) {
+        delete this;
+    }
+    return nRefCount;
+}
+
+
+/*
+ * IVssSoftwareSnapshotProvider methods
+ */
+
+STDMETHODIMP CQGAVssProvider::SetContext(LONG lContext)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::GetSnapshotProperties(
+    VSS_ID SnapshotId, VSS_SNAPSHOT_PROP *pProp)
+{
+    return VSS_E_OBJECT_NOT_FOUND;
+}
+
+STDMETHODIMP CQGAVssProvider::Query(
+    VSS_ID QueriedObjectId, VSS_OBJECT_TYPE eQueriedObjectType,
+    VSS_OBJECT_TYPE eReturnedObjectsType, IVssEnumObject **ppEnum)
+{
+    *ppEnum = new CQGAVSSEnumObject;
+    (*ppEnum)->AddRef();
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::DeleteSnapshots(
+    VSS_ID SourceObjectId, VSS_OBJECT_TYPE eSourceObjectType,
+    BOOL bForceDelete, LONG *plDeletedSnapshots, VSS_ID *pNondeletedSnapshotID)
+{
+    return E_NOTIMPL;
+}
+
+STDMETHODIMP CQGAVssProvider::BeginPrepareSnapshot(
+    VSS_ID SnapshotSetId, VSS_ID SnapshotId,
+    VSS_PWSZ pwszVolumeName, LONG lNewContext)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::IsVolumeSupported(
+    VSS_PWSZ pwszVolumeName, BOOL *pbSupportedByThisProvider)
+{
+    *pbSupportedByThisProvider = TRUE;
+
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::IsVolumeSnapshotted(VSS_PWSZ pwszVolumeName,
+    BOOL *pbSnapshotsPresent, LONG *plSnapshotCompatibility)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::SetSnapshotProperty(VSS_ID SnapshotId,
+    VSS_SNAPSHOT_PROPERTY_ID eSnapshotPropertyId, VARIANT vProperty)
+{
+    return E_NOTIMPL;
+}
+
+STDMETHODIMP CQGAVssProvider::RevertToSnapshot(VSS_ID SnapshotId)
+{
+    return E_NOTIMPL;
+}
+
+STDMETHODIMP CQGAVssProvider::QueryRevertStatus(
+    VSS_PWSZ pwszVolume, IVssAsync **ppAsync)
+{
+    return S_OK;
+}
+
+
+/*
+ * IVssProviderCreateSnapshotSet methods
+ */
+
+STDMETHODIMP CQGAVssProvider::EndPrepareSnapshots(VSS_ID SnapshotSetId)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::PreCommitSnapshots(VSS_ID SnapshotSetId)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::CommitSnapshots(VSS_ID SnapshotSetId)
+{
+    HRESULT hr = S_OK;
+    HANDLE hEvent, hEvent2;
+
+    hEvent = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_FROZEN);
+    if (hEvent == INVALID_HANDLE_VALUE) {
+        hr = E_FAIL;
+        goto out;
+    }
+
+    hEvent2 = OpenEvent(EVENT_ALL_ACCESS, FALSE, EVENT_NAME_THAW);
+    if (hEvent == INVALID_HANDLE_VALUE) {
+        CloseHandle(hEvent);
+        hr = E_FAIL;
+        goto out;
+    }
+
+    SetEvent(hEvent);
+    if (WaitForSingleObject(hEvent2, 60*1000) != WAIT_OBJECT_0) {
+        hr = E_ABORT;
+    }
+
+    CloseHandle(hEvent2);
+    CloseHandle(hEvent);
+out:
+    return hr;
+}
+
+STDMETHODIMP CQGAVssProvider::PostCommitSnapshots(
+    VSS_ID SnapshotSetId, LONG lSnapshotsCount)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::PreFinalCommitSnapshots(VSS_ID SnapshotSetId)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::PostFinalCommitSnapshots(VSS_ID SnapshotSetId)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::AbortSnapshots(VSS_ID SnapshotSetId)
+{
+    return S_OK;
+}
+
+/*
+ * IVssProviderNotifications methods
+ */
+
+STDMETHODIMP CQGAVssProvider::OnLoad(IUnknown *pCallback)
+{
+    return S_OK;
+}
+
+STDMETHODIMP CQGAVssProvider::OnUnload(BOOL bForceUnload)
+{
+    return S_OK;
+}
+
+
+/*
+ * CQGAVssProviderFactory class
+ */
+
+class CQGAVssProviderFactory : public IClassFactory
+{
+public:
+    STDMETHODIMP QueryInterface(REFIID riid, void **ppv);
+    STDMETHODIMP_(ULONG) AddRef();
+    STDMETHODIMP_(ULONG) Release();
+    STDMETHODIMP CreateInstance(
+        IUnknown *pUnknownOuter, REFIID iid, void **ppv);
+    STDMETHODIMP LockServer(BOOL bLock) { return E_NOTIMPL; }
+private:
+    long m_nRefCount;
+};
+
+STDMETHODIMP CQGAVssProviderFactory::QueryInterface(REFIID riid, void **ppv)
+{
+    if (riid == IID_IUnknown || riid == IID_IClassFactory) {
+        *ppv = static_cast<void*>(this);
+        AddRef();
+        return S_OK;
+    }
+    *ppv = NULL;
+    return E_NOINTERFACE;
+}
+
+STDMETHODIMP_(ULONG) CQGAVssProviderFactory::AddRef()
+{
+    LockModule(TRUE);
+    return 2;
+}
+
+STDMETHODIMP_(ULONG) CQGAVssProviderFactory::Release()
+{
+    LockModule(FALSE);
+    return 1;
+}
+
+STDMETHODIMP CQGAVssProviderFactory::CreateInstance(
+    IUnknown *pUnknownOuter, REFIID iid, void **ppv)
+{
+    if (pUnknownOuter) {
+        return CLASS_E_NOAGGREGATION;
+    }
+    CQGAVssProvider *pObj = new CQGAVssProvider;
+    if (!pObj) {
+        return E_OUTOFMEMORY;
+    }
+    return pObj->QueryInterface(iid, ppv);
+}
+
+
+/*
+ * DLL functions
+ */
+
+STDAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID *ppv)
+{
+    static CQGAVssProviderFactory factory;
+
+    *ppv = NULL;
+    if (IsEqualCLSID(rclsid, CLSID_QGAVSSProvider)) {
+        return factory.QueryInterface(riid, ppv);
+    }
+    return CLASS_E_CLASSNOTAVAILABLE;
+}
+
+STDAPI DllCanUnloadNow()
+{
+    return g_nComObjsInUse == 0 ? S_OK : S_FALSE;
+}
+
+EXTERN_C
+BOOL WINAPI DllMain(HINSTANCE hinstDll, DWORD dwReason, LPVOID lpReserved)
+{
+    switch (dwReason) {
+
+    case DLL_PROCESS_ATTACH:
+        g_hinstDll = hinstDll;
+        DisableThreadLibraryCalls(hinstDll);
+        break;
+    }
+
+    return TRUE;
+}
diff --git a/qga/vss-win32-provider/qga-provider.def b/qga/vss-win32-provider/qga-provider.def
new file mode 100644
index 0000000..9f3afc8
--- /dev/null
+++ b/qga/vss-win32-provider/qga-provider.def
@@ -0,0 +1,10 @@ 
+LIBRARY      "QGA-PROVIDER.DLL"
+
+EXPORTS
+	VSSCheckOSVersion	PRIVATE
+	COMRegister		PRIVATE
+	COMUnregister		PRIVATE
+	DllCanUnloadNow		PRIVATE
+	DllGetClassObject	PRIVATE
+	DllRegisterServer	PRIVATE
+	DllUnregisterServer	PRIVATE
diff --git a/qga/vss-win32-provider/qga-provider.idl b/qga/vss-win32-provider/qga-provider.idl
new file mode 100644
index 0000000..17abca0
--- /dev/null
+++ b/qga/vss-win32-provider/qga-provider.idl
@@ -0,0 +1,20 @@ 
+import "oaidl.idl";
+import "ocidl.idl";
+
+[
+    uuid(103B8142-6CE5-48A7-BDE1-794D3192FCF1),
+    version(1.0),
+    helpstring("QGAVSSProvider Type Library")
+]
+library QGAVSSHWProviderLib
+{
+    importlib("stdole2.tlb");
+    [
+        uuid(6E6A3492-8D4D-440C-9619-5E5D0CC31CA8),
+        helpstring("QGAVSSProvider Class")
+    ]
+    coclass QGAVSSHWProvider
+    {
+        [default] interface IUnknown;
+    };
+};
diff --git a/qga/vss-win32-provider/qga-provider.tlb b/qga/vss-win32-provider/qga-provider.tlb
new file mode 100644
index 0000000000000000000000000000000000000000..226452a1861371ffe0cad1019cf90fdfdcd5ef49
GIT binary patch
literal 1528
zcmeYbb_-!*U}OLRP8Kl5;0UB3A_y8H!@$4<WGF*9|A9aP$W{R21|SCUVfs9Pj1;IC
zKahR`)X0Ox{{ZC6An~sN`2tA%H9-9hNPHcj{0byK4>OPh6a(1_GM|T)fx!kz-UG<D
zK;nbc0l9GX===tt`Vb`fD?q*q5+7YXI$u?Zf#C;G4-9~uhYKVCC4f!`hZ{(Z0*HVD
zkhwswGqAt}ki;hd*$F@lQbP%-z+r|5P#hGW*vvKniaRx03p~wP?y>h_rLW<nKOg@=
z6{hYgzgH7@QE<^MU>KC!yoBjb#vz`9Lwu4+R-SJ!kIOX4xLBUUGN9-NyTyP76j}@n
z2f!qQ8-xepfXD+7rW+{SKz4&@7#qX~^1#sn3O|tFK>%ciE<<riN`6kNkzPqoQh0bc
zNbM*XJ|Un0jALSb15^rEE6h+Y8tCpA798vm9#E8DmYI@T<dc~c4pSpwQKEn@FU<fE
zfvHyrsVqoU0O~4AEUE;iEfI8i=bXgi;_z?|20Ng!&PAz-C8;S2NtFt|o-RHLWvNBQ
znfZAN=6VJOdIqMZrV5EA3T{Q23NES13Py$shQ?OLW>&_Q3PuKoMqI)S5zj9Ngog_=
gXfrXehlhjmFbIJB4$8MKU>*YlD1Z9^F{m5{03Vre%>V!Z

literal 0
HcmV?d00001

diff --git a/qga/vss-win32.h b/qga/vss-win32.h
new file mode 100644
index 0000000..21655cf
--- /dev/null
+++ b/qga/vss-win32.h
@@ -0,0 +1,86 @@ 
+/*
+ * QEMU Guest Agent win32 VSS common declarations
+ *
+ * Copyright Hitachi Data Systems Corp. 2013
+ *
+ * Authors:
+ *  Tomoki Sekiyama   <tomoki.sekiyama@hds.com>
+ *
+ * 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 */
+
+/* Ignore annotations for MS IDE */
+#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)
+
+/* Undef some duplicated error codes redefined in vss.h */
+#undef VSS_E_BAD_STATE
+#undef VSS_E_PROVIDER_NOT_REGISTERED
+#undef VSS_E_PROVIDER_VETO
+#undef VSS_E_OBJECT_NOT_FOUND
+#undef VSS_E_VOLUME_NOT_SUPPORTED
+#undef VSS_E_VOLUME_NOT_SUPPORTED_BY_PROVIDER
+#undef VSS_E_OBJECT_ALREADY_EXISTS
+#undef VSS_E_UNEXPECTED_PROVIDER_ERROR
+#undef VSS_E_INVALID_XML_DOCUMENT
+#undef VSS_E_MAXIMUM_NUMBER_OF_VOLUMES_REACHED
+#undef VSS_E_MAXIMUM_NUMBER_OF_SNAPSHOTS_REACHED
+
+/*
+ * VSS headers must be installed from Microsoft VSS SDK 7.2 available at:
+ * http://www.microsoft.com/en-us/download/details.aspx?id=23490
+ */
+#include "inc/win2003/vss.h"
+
+/* Macros to convert char definitions to wchar */
+#define _L(a) L##a
+#define L(a) _L(a)
+
+/* Constants for QGA VSS Provider */
+
+#define QGA_PROVIDER_NAME "QEMU Guest Agent VSS Provider"
+#define QGA_PROVIDER_LNAME L(QGA_PROVIDER_NAME)
+#define QGA_PROVIDER_VERSION L(QEMU_VERSION)
+
+#define EVENT_NAME_FROZEN "Global\\QGAVSSEvent-frozen"
+#define EVENT_NAME_THAW   "Global\\QGAVSSEvent-thaw"
+
+const GUID g_gProviderId = { 0x3629d4ed, 0xee09, 0x4e0e,
+    {0x9a, 0x5c, 0x6d, 0x8b, 0xa2, 0x87, 0x2a, 0xef} };
+const GUID g_gProviderVersion = { 0x11ef8b15, 0xcac6, 0x40d6,
+    {0x8d, 0x5c, 0x8f, 0xfc, 0x16, 0x3f, 0x24, 0xca} };
+
+const CLSID CLSID_QGAVSSProvider = { 0x6e6a3492, 0x8d4d, 0x440c,
+    {0x96, 0x19, 0x5e, 0x5d, 0x0c, 0xc3, 0x1c, 0xa8} };
+
+const TCHAR g_szClsid[] = TEXT("{6E6A3492-8D4D-440C-9619-5E5D0CC31CA8}");
+const TCHAR g_szProgid[] = TEXT("QGAVSSProvider");
+
+/* Enums undefined in VSS SDK 7.2 but defined in newer Windows SDK */
+enum __VSS_VOLUME_SNAPSHOT_ATTRIBUTES {
+    VSS_VOLSNAP_ATTR_NO_AUTORECOVERY       = 0x00000002,
+    VSS_VOLSNAP_ATTR_TXF_RECOVERY          = 0x02000000
+};
+
+#endif