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

[Fabian Tillier]

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.

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