[Openib-windows] [PTACH] ib_dereg_svc func when registration failed

[Yossi Leybovich]

> -----Original Message-----
> From: ftillier.sst at gmail.com [mailto:ftillier.sst at gmail.com] 
> On Behalf Of Fabian Tillier
> Sent: Sunday, September 10, 2006 9:10 PM
> To: Yossi Leybovich
> Cc: openib-windows at openib.org
> Subject: Re: [Openib-windows] [PTACH] ib_dereg_svc func when 
> registration failed
> 
> Hi Yossi,
> 
> On 9/10/06, Yossi Leybovich <sleybo at mellanox.co.il> wrote:
> > Fab
> >
> > In case that a user call ib_reg_svc but the registration failed 
> > (invalid port), call to ib_dereg_svc will cause ibal to 
> stuck in endless loop.
> > That because ib_dereg_svc check that the handle is not NULL 
> and try to 
> > destroy the obj by deref the al obj.
> > In the case that the ib_reg_svc did not succeed ibal will 
> destroy the 
> > reg_svc obj before ib_reg_svc return.
> 
> Is the user calling ib_dereg_svc with a garbage handle?  If 
> so, this is an error in the client.  There's not much we can 
> do if the user passes in bogus pointers.  Any check on the 
> memory can cause a crash due to an access violation.
> 

ib_reg_svc call finish with status == IB_SUCCESS the error is discovered
only the reg_cb.
The user try to dereg the svc after the call to ib_reg_svc.
My problem was that user cause our ibal.dll to enter endless loop (due
to ref counting problems)
and the patch address this problem.

> We have two options here, one is to always set the output 
> handles in calls so that uninitialized variables get set to 
> NULL in case of failure.  The other is to expect the clients 
> to properly handle errors.  A client that relies on the 
> handle being non-NULL must initialize it before making a call.
> 
> Personally, I prefer the latter.  A call failure should not 
> modify the output handle.  It's up to the user to initialize 
> the handle if they expect to check its value.

In this case the answer is only in the reg_cb and not in the verb return
code.
The call to ib_reg_svc invalidate the handle ,maybe we should return
error code so that the user will know the registration failed.(the
registration use the sync flag  so we can wait to the answer)
What do you think ?

> 
> In any case, the check for invalid handle as you have it is 
> fine, so go ahead and check in.  Note some nits below about the patch.
> 
> - Fab
> 
> > Singed-off-by: Yossi Leybovich (sleybo at mellanoc.co.il)
> >  Index: al_reg_svc.c
> > ===================================================================
> > --- al_reg_svc.c (revision 1658)
> > +++ al_reg_svc.c (working copy)
> > @@ -344,12 +344,13 @@
> >  {
> >  AL_ENTER( AL_DBG_SA_REQ );
> >
> > - if( !h_reg_svc )
> > +
> Why the extra blank line?
> 
> > + if( AL_OBJ_INVALID_HANDLE( h_reg_svc, AL_OBJ_TYPE_H_SA_REG) )
> >  {
> > -  AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
> > ("IB_INVALID_HANDLE\n") );
> > +  AL_PRINT_EXIT( TRACE_LEVEL_ERROR, AL_DBG_ERROR,
> > ("AL_OBJ_TYPE_H_SA_REG\n") );
> 
> I think the original error message was better.  The handle 
> was invalid.  An error print of "AL_OBJ_TYPE_H_SA_REG" 
> doesn't really tell what the error was.
> 
> >   return IB_INVALID_HANDLE;
> >  }
> > -
> > +
> 
> You replaced a blank line with a line that has a trailing tab.
> 
> >  ref_al_obj( &h_reg_svc->obj );
> >  h_reg_svc->obj.pfn_destroy( &h_reg_svc->obj, pfn_destroy_cb );
>