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

Hi all,

I'm currently reworking this patch to address the concerns mentioned
here (mostly autotools-related). I'm also going to separate it into
multiple patches.

A few questions:

I would really like cld_msg_rpc.h to show up in "include," for
consistency with the other headers.

Is it possible for cld_msg_rpc.x to live in lib/ and export the header
into the include/ directory?
I've been reading everything I can about automake, but I still haven't
been able to figure out if this kind of cross-directory dependency
between two directories at the same depth is possible.

I really don't want to put cld_msg_rpc_xdr.c in include/. But so far I
can't figure out how to have the header there, without the .c file...

Colin


On Wed, Jan 20, 2010 at 6:38 PM, Colin McCabe <[email protected]> wrote:
> 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
>