Re: [PATCHv2] cld: use XDR for all messages
Colin McCabe <[email protected]>
| Newsgroups | org.kernel.vger.hail-devel |
|---|---|
| Message-ID | <[email protected]> |
On Wed, Jan 20, 2010 at 5:38 PM, Jeff Garzik <[email protected]> wrote: > 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: I guess distcheck is pretty important to run when making a change like this. For some reason I forgot to try that. :( > >> 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. When I did "make all" with -Wall, the only warnings I saw were from the generated xdr serialization files. I'd like to figure out how to suppress those (we really should somehow tack -Wnone onto the CFLAGs of machine-generated files), but I haven't figured out how to do that yet. I was thinking of doing it in a follow-on change, since this change is already pretty big. Colin