[ofw] [PATCH] winverbs: add framework for allocating device object

[Fab Tillier]

>Index: winverbs/core/winverbs/user/wv_device.cpp
>===================================================================
>--- winverbs/core/winverbs/user/wv_device.cpp   (revision 974)
>+++ winverbs/core/winverbs/user/wv_device.cpp   (working copy)
>@@ -33,22 +33,32 @@
> STDMETHODIMP CWVDevice::
> QueryInterface(REFIID riid, LPVOID FAR* ppvObj)
> {
>-       UNREFERENCED_PARAMETER(riid);
>-       UNREFERENCED_PARAMETER(ppvObj);
>+       if (riid != IID_IUnknown && riid != IID_IWVDevice)
>+       {
>+               *ppvObj = NULL;

Don't touch output params in case of error.

>+               return E_NOINTERFACE;
>+       }
>
>-       return E_NOTIMPL;
>+       *ppvObj = this;
>+       InterlockedIncrement(&m_nRef);

Calling AddRef here would probably make things easier to follow.  The member m_nRef should not be modified anywhere but in the constructor, in AddRef, and in Release.

>Index: winverbs/core/winverbs/user/wv_device.h
>===================================================================
>--- winverbs/core/winverbs/user/wv_device.h     (revision 974)
>+++ winverbs/core/winverbs/user/wv_device.h     (working copy)
>@@ -55,6 +55,23 @@
>        STDMETHODIMP CreateCompletionQueue(SIZE_T *pEntries, IWVCompletionQueue** ppCq);
>        STDMETHODIMP AllocateProtectionDomain(IWVProtectionDomain** ppPd);
>        STDMETHODIMP Notify(OVERLAPPED* pOverlapped, WV_EVENT* pEvent);
>+
>+       CWVDevice(IWVProvider *pProvider)
>+       {
>+               pProvider->AddRef();
>+               m_pProvider = pProvider;
>+               m_nRef = 0;

Object creation should result in an object with a ref count of 1.  You should never have a valid object with a ref count of zero.  Ref count should be 1 here, and the destructor can be private to force that you never call it directly (destruction should always be through the Release method).

>+       }
>+       ~CWVDevice()
>+       {
>+               m_pProvider->Release();
>+       }

You should think about creating an auto-ref template class that calls Release automatically when it goes out of scope.  You'll find it quite useful in the code when handling error flow, and can use it as the m_pProvider member, and never have to explicitly call Release when you destroy the object.

>Index: winverbs/core/winverbs/user/wv_provider.cpp
>===================================================================
>--- winverbs/core/winverbs/user/wv_provider.cpp (revision 980)
>+++ winverbs/core/winverbs/user/wv_provider.cpp (working copy)
>@@ -29,6 +29,7 @@
>
> #include "wv_base.h"
> #include "wv_provider.h"
>+#include "wv_device.h"
>
> STDMETHODIMP CWVProvider::
> QueryInterface(REFIID riid, LPVOID FAR* ppvObj)
>@@ -73,10 +74,17 @@
> STDMETHODIMP CWVProvider::
> QueryDevice(UINT64 Guid, WV_DEVICE_ATTRIBUTES* pAttributes)
> {
>-       UNREFERENCED_PARAMETER(Guid);
>-       UNREFERENCED_PARAMETER(pAttributes);
>+       IWVDevice *dev;
>+       HRESULT hr;
>
>-       return E_NOTIMPL;
>+       hr = OpenDevice(Guid, &dev);
>+       if (hr != WV_SUCCESS)
>+               goto out;
>+
>+       hr = dev->Query(pAttributes);
>+       dev->Release();

Auto-release class would eliminate this explicit Release.

>+out:
>+       return hr;
> }
>
> STDMETHODIMP CWVProvider::
>@@ -91,10 +99,29 @@
> STDMETHODIMP CWVProvider::
> OpenDevice(UINT64 Guid, IWVDevice** ppDevice)
> {
>-       UNREFERENCED_PARAMETER(Guid);
>-       UNREFERENCED_PARAMETER(ppDevice);
>+       HRESULT hr;
>+       CWVDevice *dev;
>
>-       return E_NOTIMPL;
>+       dev = new CWVDevice(this);
>+       if (!dev)
>+       {
>+               hr = WV_NO_MEMORY;

Just return.

>+               goto err1;
>+       }
>+
>+       dev->QueryInterface(IID_IWVDevice, (LPVOID*) ppDevice);

You shouldn't need this call.  In fact, you're setting yourself up for trouble: object creation should give you an object with a ref count of 1, calling QueryInterface will bump the ref count to 2, and then you return the object (still with a ref count of 2!)

Note also that QueryInterface sets ppDevice = this - the CWVDevice object implements the IWVDevice interface, so there's no reason to call QueryInterface.  Set ppDevice = this, and then when you return you get the proper ref count of 1.

>+       hr = dev->Open(Guid);
>+       if (hr != WV_SUCCESS)

You can use the SUCCEEDED() and FAILED() macros alternatively.

>+               goto err2;
>+
>+       return WV_SUCCESS;
>+
>+err2:
>+       dev->Release();

An auto-release class here would simplify your error flow - you would just return when you detect the error above.  Even without, move the Release call into the body of the if( hr != WV_SUCCESS ) statement above, and then return.  Basically, eliminate the gotos if you can.

>+err1:
>+       *ppDevice = NULL;

Don't touch the output parameter in case of error.

>+       return hr;