[ofa-general] Re: [PATCH] libibmad/rpc.c: Handle redirection status

[Hal Rosenstock]

Sasha,

On Tue, Mar 10, 2009 at 9:50 AM, Sasha Khapyorsky <sashak at voltaire.com> wrote:
> Hi Hal,
>
> On 08:54 Mon 09 Mar     , Hal Rosenstock wrote:
>>
>> Also, in mad_rpc, status should be based on management class
>
> Please try to not mix two things in one patch.

I can break this into two if that is your preference.

>> @@ -185,10 +186,28 @@ void *mad_rpc(const struct ibmad_port *port, ib_rpc_t * rpc, ib_portid_t * dport
>>
>>       mad = umad_get_mad(rcvbuf);
>>
>> -     if ((status = mad_get_field(mad, 0, IB_DRSMP_STATUS_F)) != 0) {
>> -             ERRS("MAD completed with error status 0x%x; dport (%s)",
>> -                  status, portid2str(dport));
>> -             return 0;
>> +     mgmtclass = mad_get_field(mad, 0, IB_MAD_MGMTCLASS_F);
>> +     if (mgmtclass == 1 || mgmtclass == 0x81) {
>> +             if (mgmtclass == 1)
>> +                     status = mad_get_field(mad, 0, IB_MAD_STATUS_F);
>> +             else
>> +                     status = mad_get_field(mad, 0, IB_DRSMP_STATUS_F);
>> +             if (status != 0) {
>> +                     ERRS("MAD completed with error status 0x%x; dport (%s)",
>> +                          status, portid2str(dport));
>> +                     return 0;
>> +             }
>
> Basically it is about the same field.

One bit difference.

> Why do we need such flow?

Strict spec compliance.

>> +     } else {
>> +             if ((status = mad_get_field(mad, 0, IB_MAD_STATUS_F)) != 0) {
>
>        } else if (...) {
>
>> +                     if (status & 2) { /* redirection */
>> +                             ERRS("MAD redirection not supported; dport (%s)",
>> +                                  portid2str(dport));
>> +                     } else {
>> +                             ERRS("MAD completed with error status 0x%x; dport (%s)",
>> +                                  status, portid2str(dport));
>> +                     }
>
> You are sending patches which cleans {} around single expressions...
> Just wrap ERRS() macro with 'do {..} while (0)' and code will be
> cleaner.

I'm not following what you mean by this.

-- Hal

>> +                     return 0;
>> +             }
>>       }
>>
>>       if (ibdebug) {
>> @@ -225,8 +244,13 @@ void *mad_rpc_rmpp(const struct ibmad_port *port, ib_rpc_t * rpc, ib_portid_t *
>>       mad = umad_get_mad(rcvbuf);
>>
>>       if ((status = mad_get_field(mad, 0, IB_MAD_STATUS_F)) != 0) {
>> -             ERRS("MAD completed with error status 0x%x; dport (%s)",
>> -                  status, portid2str(dport));
>> +             if (status & 2) { /* redirection */
>> +                     ERRS("MAD redirection not supported; dport (%s)",
>> +                          portid2str(dport));
>> +             } else {
>> +                     ERRS("MAD completed with error status 0x%x; dport (%s)",
>> +                          status, portid2str(dport));
>> +             }
>
> Ditto.
>
> Sasha
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>