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@xxxxxxxxxxxxxx>
---
.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
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
