From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Garzik Subject: Re: [PATCH] cld: use XDR for all messages Date: Wed, 13 Jan 2010 19:10:32 -0500 Message-ID: <4B4E60F8.7090304@garzik.org> References: <1263135651-5407-1-git-send-email-cmccabe@alumni.cmu.edu> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:sender:message-id:date:from :user-agent:mime-version:to:cc:subject:references:in-reply-to :content-type:content-transfer-encoding; bh=/yut0vjvlDyYkmYEXf8U/WUhm408oDIEwJTWKQVmzak=; b=W4y1V0YgYCkmmTWePYe2/FmSmoEZYJgcz/mowtxyc7CMTE9sw4Juvvayl5/ENa6QRJ EJucisHqpGBTrhzhwUOKfSMX3noLV3YMg+7NDZYdC54mUPfLHTAW8iugNUiDkUGqJ/uP +KrFpVlppfF8vNG4ZKVkJYJk8Iz8DkOSIU1uQ= In-Reply-To: <1263135651-5407-1-git-send-email-cmccabe@alumni.cmu.edu> Sender: hail-devel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii"; format="flowed" To: Colin McCabe Cc: Project Hail List , Pete Zaitcev 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 > --- > .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