[ofw] [RFC] [PATCH 1/12] winverbs: add CWVBuffer class

[Sean Hefty]

>Probably better to follow the style of the UMDF samples: they are user-mode,
>C++, and implement COM interfaces.  Much closer to what you're trying to do,
>though IMO you should ignore statements like if( NULL == something), they just
>make it hard to read.

I followed the usermode toaster exe samples.

>I don't really see the value of this class.  Sure, the complexity is hidden by
>the class, but you're still always allocating at least 128 bytes on the stack.
>Are all IOCTLs pretty close to 128 bytes?  Is 128 bytes towards the large or
>small size?  Plus, stack allocation of arrays of char or wchar (and maybe other
>types) cause the compiler to add stack corruption checks (the /GS compiler
>flag, see http://msdn2.microsoft.com/en-us/library/aa290051.aspx for more
>details) to make sure the buffer wasn't overwritten.

Stack corruption checks should be debug only.  Most IOCTLs are smaller than 128
bytes.  Only the QP calls are likely to use a larger buffer because of the size
of the QP attribute structures.

At least with my Linux testing, I was able to consistently see a performance
difference.  It was small, yes, but enough small differences eventually become
noticeable.

>>+class CWVBuffer
>>+{
>>+public:
>>+       void* Get(size_t size)
>>+       {
>>+               if (size <= WvDefaultBufferSize) {
>>+                       pBuf = m_Buffer;
>>+               } else {
>>+                       pBuf = new UINT8[size];
>>+               }
>>+               return pBuf;
>>+       }
>>+
>>+       void Put()
>>+       {
>>+               if (pBuf != m_Buffer) {
>>+                       delete pBuf;
>
>I think technically this should use the delete[] operator not the delete
>operator, since you used the new[] operator.

yep - changed

>>+               }
>>+       }
>>+protected:
>>+       UINT8 m_Buffer[WvDefaultBufferSize];
>>+       void *pBuf;
>>+};
>>+
>
>Not sure what the following has to do with the CWVBuffer class...

nothing... this is just a general header file and not specific to CWVBuffer.

>
>>+HRESULT WvConvertIbStatus(ib_api_status_t status);
>>+
>>+HRESULT WvGetUserVerbs(HMODULE hLib, uvp_interface_t *pVerbs);
>>+
>> #endif // _WV_BASE_H_
>>\ No newline at end of file
>>Index: core/winverbs/user/wv_main.cpp
>>===================================================================
>>--- core/winverbs/user/wv_main.cpp      (revision 986)
>>+++ core/winverbs/user/wv_main.cpp      (working copy)
>>@@ -52,29 +52,33 @@
>>        CWVProvider *wv;
>
>If you look at the UMDF samples, you'll see that object creation is done in a
>static member function of the object being created.  This function would simply
>return CWVProvider::Create( rrid, ppvObj ) if you did this, and then you the
>implementation of CWVProvider::Create can be optimized since it has knowledge
>of the object (e.g. remove the QueryInterface call below).

I'll look at this.

>>
>>        wv = new CWVProvider;
>>-       if (!wv)
>>-       {
>>+       if (wv == NULL) {
>>                hr = WV_NO_MEMORY;
>>                goto err1;
>>        }
>>
>>        hr = wv->QueryInterface(riid, ppvObj);
>
>If you look at the UMDF samples you'll see that these just set the interface to
>the object, something along the lines of *ppvObj =
>static_cast<IWVProvider*>(this) would do.
>
>>-       if (hr != WV_SUCCESS)
>>+       if (FAILED(hr)) {
>>                goto err2;
>>+       }
>>+       wv->Release();
>>
>>+       hr = wv->Open();
>>+       if (FAILED(hr)) {
>>+               goto err2;
>>+       }
>>        return WV_SUCCESS;
>>
>> err2:
>>-       delete wv;
>>+       wv->Release();
>> err1:
>>        *ppvObj = NULL;
>
>If you look at the UMDF samples you will see that the output object pointer is
>not touched in case of error.

The documentation for QueryInterface states that it should be set to NULL.

- Sean