Re: [RFC] portabledeviceapi: Add dll

[Jacek Caban]

Hi Fabian,

On 09/11/2018 18:54, Fabian Maurer wrote:
> This is a first try to generalize COM implementations.
> I find it useful to not repeat all that code for every
> new implementation.
>
> Feedback appreciated, can we go that route?

Sure, some thoughts are bellow. Since you sent widl version later, let 
me concentrate on class factories, as it might be useful in widl 
conversation as well.
> diff --git a/include/wine/com.h b/include/wine/com.h
> new file mode 100644
> index 0000000000..2f73bfcbd0
> --- /dev/null
> +++ b/include/wine/com.h

I'm skeptical about this being general COM helper. A header just for 
class factory could be better.
> @@ -0,0 +1,256 @@
> +/*
> -
> - COM helper functions
> -
> -
> -
> - Copyright 2018 Fabian Maurer
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> - */
> -
>
> +#ifndef __WINE_WINE_COM_H
> +#define __WINE_WINE_COM_H
> -
>
> +#define COBJMACROS
> +#include "unknwn.h"
> -
>
> +#include <stdarg.h>
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winuser.h"
> +#include "winreg.h"
> +#include "rpcproxy.h"
> -
>
> +#include "wine/debug.h"
> -
>
> +#ifdef __WINE_WINE_TEST_H
> +#error This file should not be used in Wine tests
> +#endif
> -
>
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> -
>
> +static HINSTANCE instance_dll;
> -
>
> +#ifdef WINE_COM_MAIN
> -
>
> +typedef struct
> +{
> - const CLSID *clsid;
> - HRESULT (*pfnCreateInstance)(IUnknown *unk_outer, void **ppobj);
>
> +} com_object_creation_info;
> -
>
> +typedef struct
> +{
> - IClassFactory IClassFactory_iface;
> -
> - LONG ref;

Class factories don't really need to be heap-based. If they are static, 
they don't even need ref count.
> - HRESULT (*pfnCreateInstance)(IUnknown *unk_outer, void **ppobj);

This could be expressed directly in IClassFactoryVtbl by having 
different vtbls for each implemented class factory.
> +} IClassFactoryImpl;

With both above combined, you don't really need IClassFactoryImpl 
struct, which simplifies implementation a bit.
> -
>
> +static inline BOOL WINAPI com_main(HINSTANCE instance, DWORD reason, LPVOID reserved)
> +{
> - TRACE("(0x%p, %d, %p)\n", instance, reason, reserved);
> -
> - switch (reason)
> - {
> -
>    case DLL_WINE_PREATTACH:
> -
>        return FALSE;    /* prefer native version */
> -
>    case DLL_PROCESS_ATTACH:
> -
>        DisableThreadLibraryCalls(instance);
> -
>        instance_dll = instance;
> -
>        break;
> - }
> -
> - return TRUE;
>
> +}

I'm not convinced we need that. Even if we do, it has nothing to do with 
COM. This could be done by exposing module instance from winecrt0.
> +static HRESULT WINAPI classfactory_LockServer(IClassFactory *iface, BOOL dolock)
> +{
> - IClassFactoryImpl *impl = impl_from_IClassFactory(iface);
> - FIXME("(%p)->(%d), stub!\n", impl, dolock);

Having FIXME in generic implementation is not nice. Most Wine DLLs just 
returns S_FALSE from DllCanUnloadNow anyway and for them doing nothing 
in LockServer is perfectly fine.
> - return S_OK;
>
> +}
> -
>
> +static const IClassFactoryVtbl classfactory_Vtbl =
> +{
> - classfactory_QueryInterface,
> - classfactory_AddRef,
> - classfactory_Release,
> - classfactory_CreateInstance,
> - classfactory_LockServer
>
> +};
> -
>
> +HRESULT WINAPI com_get_class_object(const com_object_creation_info *object_creation, int object_creation_length,
> -
>    REFCLSID rclsid, REFIID riid, void **ppv)
>
> +{
> - unsigned int i;
> - IClassFactoryImpl *factory;
> -
> - TRACE("(%s,%s,%p)\n", debugstr_guid(rclsid), debugstr_guid(riid), ppv);
> -
> - if (!IsEqualGUID(&IID_IClassFactory, riid)
> -
>     && !IsEqualGUID( &IID_IUnknown, riid))
> -
>    return E_NOINTERFACE;
> -
> - for (i = 0; i < object_creation_length; i++)
> - {
> -
>    if (IsEqualGUID(object_creation[i].clsid, rclsid))
> -
>        break;
> - }
> -
> - if (i == object_creation_length)
> - {
> -
>    FIXME("%s: no class found.\n", debugstr_guid(rclsid));
> -
>    return CLASS_E_CLASSNOTAVAILABLE;
> - }
> -
> - factory = HeapAlloc(GetProcessHeap(), 0, sizeof(*factory));
> - if (factory == NULL)
> -
>    return E_OUTOFMEMORY;
> -
> - factory->IClassFactory_iface.lpVtbl = &classfactory_Vtbl;
> - factory->ref = 1;
> -
> - factory->pfnCreateInstance = object_creation[i].pfnCreateInstance;
> -
> - *ppv = &factory->IClassFactory_iface;
> - return S_OK;
>
> +}

This is much more complicated than it needs to be. Even after removing 
dynamic allocation, I'm not convinced that CLSID lookup is worth the 
complication.

Here is an idea about how implementing class factory could look like:

#include "wine/factory."

extern WINAPI object_constructor(IClassFactory*, REFIID, void**);

CLASS_FACTORY_IMPL(example_cf, object_constructor);

HRESULT WINAPI DllGetClassObject(REFCLSID clsid, REFIID riid, void **ppv)
{
     If (IsEqualGUID(GUID_Example, clsid))
         return IClassFactory_QueryInterface(&example_cf, riid, ppv);
     return CLASS_E_CLASSNOTAVAILABLE;
}

The actual code would be static functions inside the header and 
CLASS_FACTORY_IMPL macro would just fill the vtbl using supplied 
constructor for CreateInstance. How does that look to you?

Thanks,
Jacek