Re: [Fwd: Re: [PATCH] libibmad: Fixes for failures when not all ports of HCA are connected]

[brendan doyle <brendan.doyle-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]

On 20/03/2013 23:19, Jason Gunthorpe wrote:
> On Wed, Mar 20, 2013 at 10:44:47PM +0000, brendan doyle wrote:
>
>> Where is the inconsistency?
> There is no other case in other IB libraries or libibmad itself where
> errno is expected to be set by an int returning function. int
> returning functions should return +ERRNO.

And that is what this patch does, currently we have:

# grep errno *
register.c:#include <errno.h>
rpc.c:#include <errno.h>
rpc.c:          errno = EINVAL;
rpc.c:          errno = ENODEV;
rpc.c:          errno = ENOMEM;
rpc.c:          if (!errno)
rpc.c:                  errno = EIO;
rpc.c:                  if (!errno)
rpc.c:                          errno = EINVAL;

The patch adds:

resolve.c:              if (!errno)
resolve.c:                      errno = EIO;
resolve.c:              if (!errno)
resolve.c:                      errno = EIO;
resolve.c:              if (!errno)
resolve.c:                      errno = EIO;

So where is the inconsistency? what is resolve.c doing that is different 
to what
rpc.c is doing?

>
>> errno is still undefined, it is set in some parts of libibmad and
>> not others, so nothing has changed from a consumer perspective?
> Consistency would be returning +ERRNO from int functions and properly
> propogating return codes from lower layers:
>
>         if (smp_query_via(portinfo, &self, IB_ATTR_PORT_INFO, 0, 0,
>               srcport))
>              return errno;
>
> Would be a better modification.
The patch only sets errno, if it is not set, so if lower layers set it then
it is propagated up.


>
>> +               if (!errno)
>> +                       errno = EIO;
> This is funny, it is not portable to expect that errno will ever be !=
> 0.

Well we have several examples of this exact code segment already in 
libibmad, and
I hazard to say elsewhere in the OFED pkgs. Why is it not portable to 
expect that
errno, *may* not be set on a failure?


>> @@ -178,6 +178,7 @@ int _do_madrpc(int port_id, void *sndbuf, void *rcvbuf, int
>>                  status = umad_status(rcvbuf);
>> +               errno = status;
> _do_madrpc already returns int, so this is not great either.

OK I could concede on this, the Solaris kernel uses ib_user_mad_hdr.status
to return an errno to indicate the nature of failure, where as linux does
not.

>> @@ -56,6 +57,7 @@ uint8_t *sa_rpc_call(const struct ibmad_port *ibmad_port,
>> void *rcvbuf,
>>
>>          if (portid->lid <= 0) {
>>                  IBWARN("only lid routes are supported");
>> +               errno = EIO;
>>                  return NULL;
> This one is OK.
>
> Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html