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

[Fab Tillier]

>
>Code format change to match that used by MS toaster driver sample.

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.

>
>Add WVBuffer class that can be used for IOCTL buffers.  The class
>defines a static byte array that can be used for small buffers, but
>allocates larger buffers.
>
>Signed-off-by: Sean Hefty <sean.hefty at intel.com>
>---
>This is based off of Tzachi's recommendation, with one minor change
>to the interface.  I'm not thrilled with the class name or function
>names, so I'm open to better ideas.  I wanted them to convey that
>each instance of the class is a single data buffer.  The class should
>not be shared among multiple threads.  The term 'allocator', to me
>anyway, implies that multiple threads could call Allocator::Alloc()
>and expect to get different data buffers.

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.

>
>Index: core/winverbs/user/wv_base.h
>===================================================================
>--- core/winverbs/user/wv_base.h        (revision 979)
>+++ core/winverbs/user/wv_base.h        (working copy)
>@@ -35,6 +35,7 @@
> #include <windows.h>
>
> #include <iba\ib_types.h>
>+#include <iba\ib_uvp.h>
>
> __inline void* __cdecl operator new(size_t size)
> {
>@@ -46,6 +47,34 @@
>        HeapFree(GetProcessHeap(), 0, pObj);
> }
>
>-HRESULT ConvertIbalStatus(ib_api_status_t status);
>+const int WvDefaultBufferSize = 128;
>
>+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.

>+               }
>+       }
>+protected:
>+       UINT8 m_Buffer[WvDefaultBufferSize];
>+       void *pBuf;
>+};
>+

Not sure what the following has to do with the CWVBuffer class...

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

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

>        return hr;
> }
>
>-HRESULT ConvertIbalStatus(ib_api_status_t status)
>+HRESULT WvConvertIbStatus(ib_api_status_t status)
> {
>-       switch (status)
>-       {
>+       switch (status) {
>        case IB_SUCCESS:                                return WV_SUCCESS;
>        case IB_INSUFFICIENT_RESOURCES: return WV_INSUFFICIENT_RESOURCES;
>        case IB_INSUFFICIENT_MEMORY:    return WV_NO_MEMORY;
>
>
>_______________________________________________
>ofw mailing list
>ofw at lists.openfabrics.org
>http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
>