[PATCH v3 3/5] comsvcs: Implement ISharedPropertyGroupManager_CreatePropertyGroup().

[Nikolay Sivov]

On 8/30/20 6:03 PM, Jactry Zeng wrote:
>  static HRESULT WINAPI group_manager_CreatePropertyGroup(ISharedPropertyGroupManager *iface, BSTR name, LONG *isolation,
>          LONG *release, VARIANT_BOOL *exists, ISharedPropertyGroup **group)
>  {
> -    FIXME("iface %p, name %s, isolation %p, release %p, exists %p, group %p: stub.\n",
> +    struct group_manager *manager = impl_from_ISharedPropertyGroupManager(iface);
> +    struct property_group *property_group;
> +
> +    TRACE("iface %p, name %s, isolation %p, release %p, exists %p, group %p.\n",
>              iface, debugstr_w(name), isolation, release, exists, group);
> -    return E_NOTIMPL;
> +
> +    if (!name)
> +        return E_POINTER;
> +
> +    if (*isolation > 1 || *release > 1)
> +        return E_INVALIDARG;
> +
> +    if (*isolation || *release)
> +        FIXME("Unsopported mode: isolation %d, release %d.\n", *isolation, *release);
They have named constants for modes. Also typo in fixme message.
> +
> +    EnterCriticalSection(&manager->cs);
> +
> +    property_group = find_propery_group(manager, name);
> +    if (!property_group)
> +    {
> +        property_group = heap_alloc(sizeof(*property_group));
> +        if (!property_group)
> +        {
> +            LeaveCriticalSection(&manager->cs);
> +            return E_OUTOFMEMORY;
> +        }
> +        property_group->ISharedPropertyGroup_iface.lpVtbl = &property_group_vtbl;
> +        property_group->parent = manager;
> +        property_group->isolation = *isolation;
> +        property_group->release = *release;
> +        property_group->refcount = 1;
> +        property_group->name = SysAllocString(name);
> +        if (!property_group->name)
> +        {
> +            LeaveCriticalSection(&manager->cs);
> +            heap_free(property_group);
> +            return E_OUTOFMEMORY;
> +        }
> +
> +        list_add_tail(&manager->property_groups, &property_group->entry);
> +
> +        *exists = FALSE;
> +        ISharedPropertyGroupManager_AddRef(&property_group->parent->ISharedPropertyGroupManager_iface);
> +    }
> +    else
> +    {
> +        *exists = TRUE;
> +        *isolation = property_group->isolation;
> +        *release = property_group->release;
> +        ISharedPropertyGroup_AddRef(&property_group->ISharedPropertyGroup_iface);
> +    }
> +    *group = &property_group->ISharedPropertyGroup_iface;
> +
> +    LeaveCriticalSection(&manager->cs);
> +
> +    return S_OK;
I think it would be more readable if lock/unlock was around a smaller
block, for example using a helper that creates a new group instance, to
avoid multiple error handling branches.