Re: [PATCH] Re: [PATCHv2] cld: use XDR for all messages

On Tue, Feb 2, 2010 at 10:35 PM, Jeff Garzik <[email protected]> wrote:
> On 01/29/2010 08:01 PM, Colin McCabe wrote:
>>
>> That seems reasonable. All of those functions could be looked at as
>> "common."
>>
>> The intention was to group together a bunch of functions that were
>> useful for packet formatting. Although the bulk of the formatting is
>> done by XDR, there are some things that XDR doesn't do, like
>> generating and checking SHA sums. cld_dump_buf and cld_pkt_hdr_to_str
>> are purely for debugging purposes, but they seemed like something that
>> would be generally useful for developers making packet format changes.
>> I know that they were useful to me.
>
> I finally pushed out several changes split off from your main XDR patch, to
> the upstream cld git repo.  It took longer than expected because I would
> make changes of my own along the way, which, each time, necessitated a
> rediff+rebuild between "current cld" working tree and "cld + xdr" working
> tree.
>
> The attached patch is what remains to be split up and committed.  I have
> definitely whittled it down, and along the way, moved and renamed some
> things on my own.  With regards to cld_fmt.*, my main objection was to
> creating too many to-be-installed headers.  One more header can be a pain
> for us and for users, while one more source file in cld/lib/ is no big deal.
>
> Thus, the intention is to eliminate cld_fmt.h (newly renamed to cld_pkt.h)
> altogether, while keeping the arrangement you created in cld_fmt.c (newly
> renamed to lib/pkt.c).

Yeah, after I looked at the v2 patch I realized it wasn't separated
out well enough.
So I was working on a v3 patch series that had the following changes:

1. kill CLD_MAX_PKT_MSG and add CLD_MAX_PAYLOAD_SZ
2. reformat Makefile.am files (to one file per line for nicer diffs)
3. move cld_msg macros into cld_common
4. some style issues
5. add cld_fmt.c (now called cld_pkt.c)
6. create cldc_call_opts_get_data and switch to using it in test/ and tool/
7. the big XDR changes.

It looks like 2, 3, 4, 5 have already come to pass (thanks Jeff!)

I just sent out #1 and another patch that is related to CLD_INODE_NAME_MAX
I think I'll send out a patch for #6 soon.

> I will continue whittling down the patch until it just contains the XDR
> changes themselves.

In tools/Makefile.am, I don't think you need $(top_srcdir)/lib any
more, since cld_msg_rpc.h is now an installed header.

Colin