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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 20, 2010 at 5:38 PM, Jeff Garzik <jeff@xxxxxxxxxx> 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<cmccabe@xxxxxxxxxxxxxx>
>> ---
>>  .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
--
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

[Index of Archives]     [Fedora Clound]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux