[openib-general] Re: [PATCH] RMPP: Fix length in first segment of multipacket sends

[Hal Rosenstock]

Hi Michael,

On Mon, 2005-08-29 at 10:47, Michael S. Tsirkin wrote:
> Hal, first, sorry about nitpicking.
> 
> Quoting Hal Rosenstock <halr at voltaire.com>:
> > > >                 rmpp_mad->rmpp_hdr.paylen_newwin =
> > > >                         cpu_to_be32(mad_send_wr->total_seg *
> > > >                                     (sizeof(struct ib_rmpp_mad) -
> > > > -                                      offsetof(struct ib_rmpp_mad, data)));
> > > > +                                      offsetof(struct ib_rmpp_mad, data)) -
> > > > +                                   mad_send_wr->pad);
> > > 
> > > BTW, I just noticed that whitespace was (and remains) broken in these lines:
> > > indentation is done by spaces.
> > 
> > The whitespace is preceeded by tabs and is to make the parameters line
> > up. I thought that was allowable coding style. It has been used in many
> > places in OpenIB code.
> 
> Sure, whitespace preceeded by tabs is OK.
> But, pls take a look at the original file, or the patch, I think
> you'll see that its not preceeded by tabs in this instance.

I think this is in the original file.

> Some more nitpicks:
> 
> In this specific instance, its probably best to just add a temp variable and
> write 
> 
> 			w =  mad_send_wr->total_seg *
> 				(sizeof(struct ib_rmpp_mad) -
> 				offsetof(struct ib_rmpp_mad, data)) -
> 				mad_send_wr->pad;
> 
> 			rmpp_mad->rmpp_hdr.paylen_newwin = cpu_to_be32(w)
> 
> to avoid placing the descendant cpu_to_be32 left of the "=" sign.
> 
> Documentation/CodingStyle says about this:
> > Descendants are always substantially shorter than the parent and are
> > placed substantially to the right.
> 
> 
> And by the way, wouldnt it be a good idea to replace hard-coded
> 220 and such in ib_mad.h by a symbolic constant, and then we'd just
> have
> 
> 			w =  mad_send_wr->total_seg * IB_RMMP_DATA_SIZE -
> 				mad_send_wr->pad;
> 
> 			rmpp_mad->rmpp_hdr.paylen_newwin = cpu_to_be32(w)
> 
> What do you think?

All seem reasonable to me. Sean should comment and has the final say on
this.

-- Hal