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 <z.figura12(a)gmail.com>
> ---
>  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.

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

[Zebediah Figura]

On 02/08/2017 03:54 AM, Huw Davies wrote:
> 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 <z.figura12(a)gmail.com>
> > ---
> >  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.

Thanks for the comments. Most of the code was copied from the 32-bit
version, so my variable naming (and code structure) was not as good and
consistent as it should have been.