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

Well, this definitely does not build as-is.  lib/Makefile.am needs

	BUILT_SOURCES           = cld_msg_rpc.h

otherwise nothing builds at all, because cld_msg_rpc.h does not exist. 
test/Makefile.am and tools/Makefile.am both need

	-I$(top_srcdir)/lib

added to their INCLUDES.  And you should probably examine 
http://www.gnu.org/software/automake/manual/automake.html#Clean to make 
sure that "make distclean" and "make clean" work.

Still playing with it.  You definitely need to make sure your patch 
builds and runs against a fresh git repo check + autogen.sh + 
./configure run.

	Jeff