Re: [PATCH] compobj.dll16: Implement CoGetClassObject().

[Huw Davies]

On Sat, Feb 04, 2017 at 07:47:32PM -0600, Zebediah Figura wrote:
> Fixes bug https://bugs.winehq.org/show_bug.cgi?id=41209
> Signed-off-by: Zebediah Figura
>
> dlls/compobj.dll16/compobj.c          | 242 +++++++++++++++++++++++++++++++---
>  dlls/compobj.dll16/compobj.dll16.spec |   2 +-
>  2 files changed, 222 insertions(+), 22 deletions(-)

Hi Zebediah,

This patch could be split into a series of smaller patches.
For example, implement CoGetClassObject16() first, then
CoCreateInstance16() as a seperate patch, and then CoFreeAllLibraries().

I've inlined some comments below.
> diff --git a/dlls/compobj.dll16/compobj.c b/dlls/compobj.dll16/compobj.c
> index b934a06..1b100c3 100644
> --- a/dlls/compobj.dll16/compobj.c
> +++ b/dlls/compobj.dll16/compobj.c
> @@ -30,6 +30,8 @@
>  #include <string.h>
>  #include <assert.h>
>
> +#define COBJMACROS
> -
>
> #include "windef.h"
>  #include "winbase.h"
>  #include "winuser.h"
> @@ -42,6 +44,7 @@
>  #include "wtypes.h"
>  #include "wine/unicode.h"
>  #include "wine/winbase16.h"
> +#include "wine/list.h"
>
> #include "wine/debug.h"
>
> @@ -98,6 +101,115 @@ static inline IMalloc16Impl *impl_from_IMalloc16(IMalloc16 *iface)
>          return CONTAINING_RECORD(iface, IMalloc16Impl, IMalloc16_iface);
>  }
>
> +typedef HRESULT (CALLBACK *DllGetClassObjectFunc)(REFCLSID clsid, REFIID iid, LPVOID *ppv);
> +typedef HRESULT (WINAPI *DllCanUnloadNowFunc)(void);
> -
>
> +typedef struct tagOpenDll
> +{
> - LONG refs;
> - LPSTR library_name;
> - HMODULE16 library;
> - DllGetClassObjectFunc DllGetClassObject;
> - DllCanUnloadNowFunc DllCanUnloadNow;
> - struct list entry;
>
> +} OpenDll;

Why not call the typedef open_dll and drop the tag name - it's
not used.  Or better yet, ditch the typedef and just use struct open_dll
throughout.
> +static struct list openDllList = LIST_INIT(openDllList);

Similarly rename to open_dll_list.
> -
>
> +/*****************************************************************************
> -
> - This section contains OpenDllList implementation
> - */
> -
>
> +static OpenDll *COMPOBJ16_DllList_Get(LPCSTR library_name)
> +{
> - OpenDll *ptr;
> - LIST_FOR_EACH_ENTRY(ptr, &openDllList, OpenDll, entry)
> - {
> -
>    if (!strcasecmp(library_name, ptr->library_name) &&
> -
>        ++(ptr->refs) != 1) /* entry is being destroy if == 1 */

This reference handling looks odd - at the very least you're leaving
a previously zero ref count at one.  Also, how about naming this function
(and others similarly below) dll_list_get()?
> +/* pass FALSE for free_entry to release a reference without destroying the
> -
> - entry if it reaches zero or TRUE otherwise */
>
> +static void COMPOBJ16_DllList_ReleaseRef(OpenDll *entry)
> +{
> - if (--(entry->refs) == 0)
> - {
> -
>    list_remove(&entry->entry);
> -
> -
>    TRACE("freeing %x\n", entry->library);
> -
>    FreeLibrary16(entry->library);
> -
> -
>    HeapFree(GetProcessHeap(), 0, entry->library_name);
> -
>    HeapFree(GetProcessHeap(), 0, entry);
> - }
>
> +}
> -
>
> /******************************************************************************
> -
> IMalloc16_QueryInterface	[COMPOBJ.500]
>
> */
> @@ -279,13 +391,34 @@ HRESULT WINAPI CoCreateStandardMalloc16(DWORD dwMemContext,
>      return S_OK;
>  }
>
> +/***********************************************************************
> -
> -
>       CoFreeAllLibraries [COMPOBJ.12]
> - */
>
> +void WINAPI CoFreeAllLibraries16(void)
> +{
> - OpenDll *entry, *next;
> -
> - TRACE("()\n");
> - LIST_FOR_EACH_ENTRY_SAFE(entry, next, &openDllList, OpenDll, entry)
> - {
> -
>    list_remove(&entry->entry);
> -
> -
>    TRACE("freeing %x\n", entry->library);
> -
>    FreeLibrary16(entry->library);
> -
> -
>    HeapFree(GetProcessHeap(), 0, entry->library_name);
> -
>    HeapFree(GetProcessHeap(), 0, entry);
> - }
>
> +}
> -

This code and the code in COMPOBJ16_DllList_ReleaseRef are essentially the same,
it should be possible to avoid this duplication.

Huw.