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

[Michael S. Tsirkin]

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.

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?
Thanks,
MST

-- 
MST