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

[Jeff Garzik <jeff@garzik.org>]

On 01/10/2010 10:00 AM, Colin McCabe wrote:
> This patch moves CLD from using manual data serialization to using XDR (via
> rpcgen). Both the packet header and the message body are now serialized and
> deserialized using XDR. This makes it easy to have a variable-length packet
> header, as well as a variable-length message body.
>
> Message definitions are now in cld_msg.x. Helper functions for using messages
> are in cld_fmt.c. The new client and server-side functions for sending
> messages don't require the caller to precompute the size of the message.
> Instead, you simply tell it what type of message you have, and give it the
> data. It handles determining message size and encoding.
>
> The only fields that I did not choose to deal with using XDR are the packet
> sequence ID and the packet SHA checksum. The seqid would have been
> inconvenient and inefficient to store in the XDR header, because we have to
> check a lot of packets' seqids against the ACKs we receive very frequently.
> The SHA checksum can't be computed until the rest of the packet has been
> serialized, which is counter to the way XDR works (it is a single-pass
> serialization system.) So these fields are in the packet footer, which comes
> at the end of every packet.
>
> This patch introduces a minor libcldc API change in struct cldc_call_opts.
>
> Signed-off-by: Colin McCabe<cmccabe@alumni.cmu.edu>
> ---
>   .gitignore             |    4 +
>   include/Makefile.am    |    2 +-
>   include/cld_common.h   |   10 +
>   include/cld_fmt.h      |   78 ++++
>   include/cld_msg.h      |  252 -------------
>   include/cldc.h         |   37 +-
>   lib/Makefile.am        |   14 +-
>   lib/cld_fmt.c          |  189 ++++++++++
>   lib/cld_msg_rpc.x      |  212 +++++++++++
>   lib/cldc.c             |  960 +++++++++++++++++++++---------------------------
>   lib/common.c           |    6 +-
>   server/Makefile.am     |    9 +-
>   server/cld.h           |   97 ++++--
>   server/cldb.h          |    2 +-
>   server/msg.c           |  304 ++++++----------
>   server/server.c        |  743 ++++++++++++++++++++-----------------
>   server/session.c       |  396 ++++++++++-----------
>   test/load-file-event.c |   14 +-
>   test/save-file-event.c |    2 -
>   tools/cldcli.c         |   22 +-
>   20 files changed, 1744 insertions(+), 1609 deletions(-)
>   create mode 100644 include/cld_fmt.h
>   delete mode 100644 include/cld_msg.h
>   create mode 100644 lib/cld_fmt.c
>   create mode 100644 lib/cld_msg_rpc.x

OK, final review.  Please fix these problems and resend (some of these 
repeated from other emails; putting here to consolidate):

* needs to build patch builds and runs against a fresh git repo clone + 
./autogen.sh + ./configure + make distcheck

* one build fix I applied was adding the generated header to 
BUILT_SOURCES in lib/Makefile.am, referring to 
http://www.gnu.org/software/automake/manual/automake.html#Sources

* make sure the generated header can be found for distcheck builds.  For 
me, I needed to add lib/ to the INCLUDE dirs. YMMV.  As Pete says, 
better solutions could be found.  You could always put cld_msg_rpc.x in 
include/, generate the XDR header first thing (reorder SUBDIRS in 
./Makefile.am), and then generate the xdr.c file by referencing source 
file ../include/cld_msg_rpc.x.

   It's ugly, but cld_msg_rpc.h is definitely an exported header that
   needs special treatment.  This solution, as a side effect, would
   work within existing INCLUDES Makefile.am setups.

* include/Makefile.am needs cld_fmt.h added to include_HEADERS

* verify that 'make clean' and 'make distclean' work as expected, 
removing the built objects (clean) and generated source files 
(distclean).  distclean requires a ./configure step before the tree is 
buildable...  ie. it takes the working tree back to 
just-unpacked-from-tarball state.

* you increased the size of struct cldc_call_opts to over 256k!  That is 
unworkable in multi-thread programs, where struct cldc_call_opts may be 
allocated on the stack -- as it is in most of the tool and test code 
using libcldc.  another solution, acceptable to usage within a thread 
(often limited to 8k stacks) must be found.

* it would be nice if this patch were separated out a bit, as there are 
a lot of semi-related changes and cleanups mixed in amongst the XDR 
conversion.  These changes are technically correct, such as

> -               HAIL_DEBUG(&sess->log, "rx_gen: comparing req->xid (%llu) "
> -                       "with resp->xid_in (%llu)",
> -                       (unsigned long long) le64_to_cpu(req->xid),
> -                       (unsigned long long) le64_to_cpu(resp->xid_in));
> +               HAIL_DEBUG(&sess->log, "%s: comparing req->xid (%llu) "
> +                               "with resp.xid_in (%llu)",
> +                               __func__,
> +                               (unsigned long long) req->xid,
> +                               (unsigned long long) resp.xid_in);

but adding __func__ to HAIL_DEBUG() statement is clearly not something 
strictly related to XDR conversion.  Pulling out these cleanups into a 
patch 1/2 would make XDR patch, 2/2, a much smaller patch.  It's not a 
_huge_ deal, if the split is large PITA, but it is preferred to separate 
out logical change sets.


other, minor stuff:
-------------------
* do we really need separate pkt_is_first() and pkt_is_last() ?  I tend 
to prefer a flags-based approach, where the code tests bits in a 'flags' 
variable.

* the stuff in cld_fmt.h is essentially global application identifier 
namespace.  as such, exporting functions like 'dump_buf' or 'opstr' may 
run into trouble.  suggest '__cld_' prefix, perhaps.



Overall, though, great work!  The change is accepted in principle; I 
think the patch needs some revisions, but we definitely want this 
change.  It will make non-C language clients easier to write and 
maintain, better document the network protocol, and reduce the 
difficulty of changing the network protocol.

	Jeff