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

[Sasha Khapyorsky]

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.

> @@ -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. Why do we need such flow?

> +	} 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.

> +			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