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

On 01/15/2010 12:49 PM, 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.
>
> This patch introduces a minor libcldc API change in struct cldc_call_opts.
>
> v2 changes:
> * Added __cld prefix to functions in cld_fmt.c
>
> * When decoding CMT_GET messages, we should store the payload in the
> session structure, rather than in cldc_call_opts.
>
> * Got rid of pkt_is_first, pkt_is_last, in favor of a flags-based approach.
>
> * Killed CLD_MAX_PKT_MSG. It's more efficient to only allocate as much space
> as you need, rather than always allocating space for 128 packets in a message.
>
> * Created CLD_MAX_PAYLOAD_SZ to mean the maximum size of the data that can be
> sent with GET or PUT. This is different (and smaller than!) the maximum
> message size.
>
> * automake: Add cld_msg_rpc.h to BUILT_SOURCES
>
> * automake: Add lib dir to INCLUDES
>
> * automake: "make clean" now deletes XDR build products
>
> Signed-off-by: Colin McCabe<[email protected]>
> ---
>   .gitignore             |    4 +
>   include/Makefile.am    |    2 +-
>   include/cld_common.h   |   10 +
>   include/cld_fmt.h      |   89 +++++
>   include/cld_msg.h      |  252 -------------
>   include/cldc.h         |   36 +-
>   lib/Makefile.am        |   19 +-
>   lib/cld_fmt.c          |  193 ++++++++++
>   lib/cld_msg_rpc.x      |  218 +++++++++++
>   lib/cldc.c             |  966 +++++++++++++++++++++---------------------------
>   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/Makefile.am       |    5 +-
>   test/load-file-event.c |   14 +-
>   test/save-file-event.c |    2 -
>   tools/cldcli.c         |   22 +-
>   21 files changed, 1777 insertions(+), 1612 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

Looking good...  we are definitely getting closer.

Currently, it still does not pass "make distcheck", which presages a lot 
of installation problems.  Notably,
	#include <lib/cld_msg_rpc.h>
is not consistent with the rest of the headers, producing things such as 
"make distcheck" build breakage:

> libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I../../lib -I.. -I../../include -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -g -O2 -MT cldc.lo -MD -MP -MF .deps/cldc.Tpo -c ../../lib/cldc.c  -fPIC -DPIC -o .libs/cldc.o
> In file included from ../../lib/cldc.c:37:
> ../../include/cldc.h:26:29: error: lib/cld_msg_rpc.h: No such file or directory

You will either need to move the header to $cld/include/ or update 
INCLUDES in various */Makefile.am files to reference $(top_srcdir)/lib 
(which I see you actually did, in a few cases).

Also, it would be nice if this did not introduce warnings.  A big point 
with the testsuite, and minor point with compiler warnings, is that -- 
more than anything else -- we do not want to introduce regressions.

	Jeff