[ofw] NDv2 provider patch

[Hefty, Sean]

Responses are separated based on classes as I review them.

> +template<class T>
> +class auto_ref
>  {
> +       T* m_pObj;
> +
>  public:
> +       auto_ref( T* pObj ) :
> +               m_pObj( pObj )
> +       {
> +               if( m_pObj != NULL ){
> +                       m_pObj->AddRef();
> +               }
> +       }
> +       auto_ref( const auto_ref& r ) :
> +               m_pObj( r.m_pObj )
> +       {
> +               if( m_pObj != NULL ){

The syntax should be:

if (no space here) {
for(no space here) {
while(no space here) {

"( " and " )" are not used in normal writing, so should not be done in code.  It leads to inconsistencies as visible in the code below.  There should be a space before the '{'. 

> +                       m_pObj->AddRef();

Didn't you mean AddRef(  )?  ;)

> +               }
> +       }
> +       ~auto_ref()
> +       {
> +               if( m_pObj != NULL ){
> +                       m_pObj->Release();
> +               }
> +       }
> +
> +       T* operator ->() const { return m_pObj; }
> +       T* detach(){ T* tmp = m_pObj; m_pObj = NULL; return tmp; }
> +       T* get() const { return m_pObj; }
> +       T** ref(){ return &m_pObj; }
> +       VOID** ppvObj(){ return reinterpret_cast<void**>(ref()); }
> +
> +       auto_ref& operator =( T* pObj )
> +       {
> +               if( m_pObj != NULL )
> +               {
> +                       m_pObj->Release();
> +               }
> +               m_pObj = pObj;
> +               return *this;
> +       }
> +
> +       auto_ref& operator =( const auto_ref& r )
> +       {
> +               if( m_pObj != NULL )
> +               {
> +                       m_pObj->Release();
> +               }
> +               m_pObj = r.get();
> +               if( m_pObj != NULL )
> +               {
> +                       m_pObj->AddRef();
> +               }
> +               return *this;
> +       }
> +
> +};

auto-reference obscurity...

> +
> +
> +class CNDBase
> +{
> +       static volatile LONG m_nObj;
> +       volatile LONG m_nRef;
> +
> +protected:
>         CNDBase();
> -       ~CNDBase() {};
> +       virtual ~CNDBase();
> +
> +private:
> +       // Nobody should use these.

The use of 'private' kind of indicates that.

> +       CNDBase( const CNDBase& copyFromMe );
> +       CNDBase& operator =(const CNDBase& copyFromMe );
> +
> +public:
>         STDMETHODIMP_(ULONG) AddRef();
>         STDMETHODIMP_(ULONG) Release();
> 
> -       virtual void Delete() {};
> +       static LONG ObjCount(){ return m_nObj; }

What is this for?

> +};
> 
> -       volatile LONG                   m_nRef;
> 
> -protected:
> -};
> +extern HANDLE g_hHeap;
> +
> +HRESULT NDConvertWVStatus(HRESULT hr);
> +
> +} // namespace NetworkDirect
> +} // namespace WinVerbs
> 
>  __inline void* __cdecl operator new(size_t size)
>  {
> -       return HeapAlloc(GetProcessHeap(), 0, size);
> +       return ::HeapAlloc(WinVerbs::NetworkDirect::g_hHeap, 0, size);
>  }
> 
> -__inline void __cdecl operator delete(void *pObj)
> +__inline void* __cdecl operator new(size_t, void* p)
>  {
> -       HeapFree(GetProcessHeap(), 0, pObj);
> +       return p;
>  }

What's the purpose of this call?  The user passes in a pointer, and the function returns it.

> 
> -HRESULT NDConvertWVStatus(HRESULT hr);
> +__inline void __cdecl operator delete(void *pObj)
> +{
> +       ::HeapFree(WinVerbs::NetworkDirect::g_hHeap, 0, pObj);
> +}
> 
>  #endif // _ND_BASE_H_


> Index: user/nd_base.cpp
> ===================================================================
> --- user/nd_base.cpp    (revision 2848)
> +++ user/nd_base.cpp    (working copy)
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (c) 2009-2010 Intel Corporation. All rights reserved.
> + * Copyright (c) 2010 Microsoft Corporation.  All rights reserved.
>   *
>   * This software is available to you under the OpenIB.org BSD license
>   * below:
> @@ -27,18 +28,45 @@
>   * SOFTWARE.
>   */
> 
> +#include "precomp.h"
>  #include "nd_base.h"
> -#include <ndstatus.h>
> 
> -CNDBase::CNDBase()
> +
> +namespace WinVerbs {
> +namespace NetworkDirect {
> +
> +volatile LONG CNDBase::m_nObj = 0;
> +
> +CNDBase::CNDBase() :
> +       m_nRef( 1 )
>  {
> -       m_nRef = 1;
> +       ::InterlockedIncrement( &m_nObj );

The object is being created here.  Just assign the value 1 - no need for an interlocked operation.

> +}
> +
> +CNDBase::CNDBase(const CNDBase&) :
> +       m_nRef( 1 )
> +{
> +       // New object, so we increment m_nObj, but the ref count is 1.
> +       ::InterlockedIncrement( &m_nObj );
> +}
> +
> +CNDBase& CNDBase::operator =(const CNDBase&)
> +{
> +       // Both objects already exist, don't touch m_nObj.
> +       // Don't copy the ref count, either, as that could result
> +       // in improper behavior.
> +       return *this;
> +}
> +
> +CNDBase::~CNDBase()
> +{
> +       ::InterlockedDecrement( &m_nObj );

We're in the destructor here.  The reference count should be 0 before the destructor is called, not go to 0 in it.

>  }
> 
>  STDMETHODIMP_(ULONG) CNDBase::
>  AddRef(void)
>  {
> -       return InterlockedIncrement(&m_nRef);
> +       return ::InterlockedIncrement(&m_nRef);
>  }
> 
>  STDMETHODIMP_(ULONG) CNDBase::
> @@ -46,19 +74,23 @@ Release(void)
>  {
>         ULONG ref;
> 
> -       ref = (ULONG) InterlockedDecrement(&m_nRef);
> -       if (ref == 0) {
> -               Delete();
> +       ref = (ULONG)::InterlockedDecrement(&m_nRef);
> +       if (ref == 0){

Put a space before the brace

> +               delete this;
>         }
>         return ref;
>  }
> 
>  HRESULT NDConvertWVStatus(HRESULT hr)
>  {
> -       switch (hr) {
> +       switch (hr){

Put a space before the brace

>         case WV_IO_PENDING:
>                 return ND_PENDING;

Is this mapping still needed for ND 2?

>         default:
>                 return hr;
>         }
>  }
> +
> +
> +} // namespace NetworkDirect
> +} // namespace WinVerbs