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

[Tomoki Sekiyama]

On 6/26/13 10:32 , "Laszlo Ersek" <address@hidden> 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