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

[Tomoki Sekiyama]

On 6/27/13 11:01 , "Laszlo Ersek" <address@hidden> 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