[ofw] [PATCH] ND Provider: fix service ID byte ordering

[Fab Tillier]

Hefty, Sean wrote on Fri, 11 Feb 2011 at 15:50:03

>> Index: core/al/kernel/al_ndi_cm.c
>> 
>> ===================================================================
>> --- core/al/kernel/al_ndi_cm.c	(revision 3095)
>> +++ core/al/kernel/al_ndi_cm.c	(working copy)
>> @@ -1072,7 +1072,9 @@
>> __ndi_fill_cm_req(
>> 
>>  	memset( p_cm_req, 0, sizeof(*p_cm_req) );
>> -	p_cm_req->service_id = IB_REQ_CM_RDMA_SID_PREFIX | (p_req-
>> >prot <<  16) | p_req->dst_port;
>> +	p_cm_req->service_id = IB_REQ_CM_RDMA_SID_PREFIX |
>> +		((UINT64)p_req->prot) << 40 |
>> +		((UINT64)p_req->dst_port) << 48;
> 
> IMO, it's clearer and more maintainable to define values and perform
> operations in host order, with swapping done at the end..

Doing it in host order becomes this ugly combination of nested swap calls:

    p_cm_req->service_id = RtlUlonglongByteSwap(
        RtlUlonglongByteSwap( IB_REQ_CM_RDMA_SID_PREFIX ) +
        ((UINT64)p_req->prot) << 16 +
        RtlUshortByteSwap( p_req->dst_port )
        );

Or

    p_cm_req->service_id = IB_REQ_CM_RDMA_SID_PREFIX +
        RtlUlonglongByteSwap( ((UINT64)p_req->prot) << 16 +  RtlUshortByteSwap( p_req->dst_port ) );

Is that any easier to understand/maintain?

Clearest is probably:

    strut _rdma_cm_sid
    {
        uint32_t zero;
        uint8_t is_rdma_cm;
        uint8_t prot;
        net16_t port;
    } * p_sid = &p_cm_req->service_id;

    p_sid->port = p_req->dst_port;
    p_sid->prot = p_req->prot;
    p_sid->is_rdma_cm = 1;
    p_sid->zero = 0;

>> +/*
>> + * The prefix and mask are defined in network order.
>> + */
>> +#define IB_REQ_CM_RDMA_SID_PREFIX 	0x0000000100000000I64 
>> +#define IB_REQ_CM_RDMA_SID_PREFIX_MASK 	0x000000FFFFFFFFFFI64
> 
> ..but if you want to define a value in network order, use something like:
> 
> #define VAL htonX(1234)
> 
> which is self documenting.  The compiler should be able to remove the
> actual computation.

I'll use CL_HTONx to make it clearer, as the compiler isn't smart enough to remove the computation.

Thanks for the feedback!
-Fab