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

[Sean Hefty]

>>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.

I need to look this up again, but I thought this was the standard behavior for
QueryInterface.  (It forces an application crash if it assumes that the call
worked and tries to invoke the interface.)

>>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).

I need to change this everywhere then.


>>+       ~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.

I couldn't figure out how to get any of the template classes to work, so I gave
up for the sake of making _some_ sort of progress.

>> 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.

This sets ppDevice to NULL on failure below.

>
>>+               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.

I was just trying to follow the random documentation that is COM...  I'll update
this when initializing the ref count to 1.

>>+       hr = dev->Open(Guid);
>>+       if (hr != WV_SUCCESS)
>
>You can use the SUCCEEDED() and FAILED() macros alternatively.

I had actually taken all of these out and just used 'if (hr)'.  But if
'if(SUCCEEDED(hr))' is standard, I can switch to that.  The samples were
inconsistent on this.

>
>>+               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.

gotos in error handling cases are your friend.  :)


I have several changes since the last patch where I've started implementing the
IOCTL calls.  I'm still working on that piece.  Once I get at least the device
calls written, I'll post that portion.  I haven't been able to test yet, since
the kernel driver doesn't even load at this point...

- Sean