From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:58031)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1UsDfR-0006kY-LZ
	for qemu-devel@nongnu.org; Thu, 27 Jun 2013 10:59:30 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1UsDfM-0000Nf-Cv
	for qemu-devel@nongnu.org; Thu, 27 Jun 2013 10:59:25 -0400
Received: from mx1.redhat.com ([209.132.183.28]:6567)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1UsDfM-0000NK-2W
	for qemu-devel@nongnu.org; Thu, 27 Jun 2013 10:59:20 -0400
Message-ID: <51CC53AD.4000005@redhat.com>
Date: Thu, 27 Jun 2013 17:01:01 +0200
From: Laszlo Ersek <lersek@redhat.com>
MIME-Version: 1.0
References: <20130606150618.10486.60669.stgit@hds.com>
	<20130606150645.10486.23215.stgit@hds.com>
	<51C9BF54.6000008@redhat.com> <51CAFB95.5080609@redhat.com>
In-Reply-To: <51CAFB95.5080609@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v4 06/10] qemu-ga: Add Windows VSS provider
 to quiesce applications on fsfreeze
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
Cc: libaiqing@huawei.com, mdroth@linux.vnet.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, vrozenfe@redhat.com, pbonzini@redhat.com, seiji.aguchi@hds.com, areis@redhat.com

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