9:32 a.m.
2011/5/6 Daniel P. Berrange <berrange(a)redhat.com>:
On Sun, Apr 24, 2011 at 11:13:47AM +0200, Matthias Bolte wrote:
> Richard W.M. Jones suggested [1] that the code that directly deals with the
> XDR protocol should be generated. The remote_generate_stubs.pl script
> already generates all the headers, just the bodies in the daemon and remote
> driver are manually written. But most of the functions just follow simple
> patterns. So I extended the generator to exploit this patterns and move
> 11 kLOC code from manually written to generated code.
As expected the generator itself is pretty complex & hard to understand,
but given the complexity of our API there's nothing that can really be
done about this.
It probably makes it harder that the commits just document the process
of how I moved groups of functions to the generator. I meant to add
patch 23/22 to the series that adds more documentation in the guts of
the generator to explain its mechanics, but I didn't come around to do
that yet.
I see it takes a blacklist approach, so I'm wondering what the
behaviour
is if someone adds a new function that it isn't designed to cope with
yet. Is it able to raise immediate errors for stuff it can't cope with,
or does it silently generate bogus code like our python generator often
does ? Ideally the former, but it isn't critical - just something we
need to be aware of for future API patch review
Hm, I didn't think of this problem yet. But I would expect that in
almost all cases the generator will complain about stuff it can't
handle. Currently there are some general-catch-the-rest-else cases in
the generator. Those can be made much stricter. I think that it'll be
possible to make the generator reliably report stuff it can't handle.
I'll work on that.
To be really sure, we might add a whitelist aside the backlist and
once the generator sees something that's not on one of the lists it
complains with an error that tells the programmer to either add the
function to whitelist and check that the generated code does the right
thing, or add it to the blacklist and add a function body manually.
On the other hand, stuff like flags parameter being unsigned in the
public API but signed in the XDR protocol need manual special cases in
the generator. We could add some general sanity checks to avoid
something like this in the future. The generator could complain when a
parameter is called flags but is signed. I'll work on that too.
Also, as per my other patch today, I think we shouldn't actually
store
the generated bodies in GIT, once this is done.
That's true.
> During this I came a cross many small variations and problems in
the XDR
> protocol. For example, NWFilterDefineXML has a flags parameter in the public
> API, but it's not transferred in the XDR protocol. Another things is the
> variations in the usage of unsigned VS signed types. This comes in two forms.
> public API VS XDR procotol and in between different functions. For example,
> some functions use int for the flags paramater and some use unsigned int.
>
> This results in quite a lot of special case handling in the generator.
>
> cfg.mk | 10 +-
> daemon/Makefile.am | 46 +-
> daemon/qemu_dispatch_args.h | 2 +-
> daemon/qemu_dispatch_bodies.c | 6 +
> daemon/qemu_dispatch_prototypes.h | 2 +-
> daemon/qemu_dispatch_ret.h | 2 +-
> daemon/qemu_dispatch_table.h | 2 +-
> daemon/remote.c | 5765 +----------------------------------
> daemon/remote_dispatch_args.h | 2 +-
> daemon/remote_dispatch_bodies.c | 5933 +++++++++++++++++++++++++++++++++++
> daemon/remote_dispatch_prototypes.h | 80 +-
> daemon/remote_dispatch_ret.h | 2 +-
> daemon/remote_dispatch_table.h | 158 +-
> daemon/remote_generate_stubs.pl | 195 --
> daemon/remote_generator.pl | 1198 +++++++
> po/POTFILES.in | 1 +
> src/Makefile.am | 13 +-
> src/remote/qemu_client_bodies.c | 4 +
> src/remote/qemu_protocol.c | 2 +-
> src/remote/qemu_protocol.h | 2 +-
> src/remote/qemu_protocol.x | 2 +-
> src/remote/remote_client_bodies.c | 4664 +++++++++++++++++++++++++++
> src/remote/remote_driver.c | 4907 +----------------------------
> src/remote/remote_protocol.c | 26 +-
> src/remote/remote_protocol.h | 26 +-
> src/remote/remote_protocol.x | 34 +-
> src/remote_protocol-structs | 26 +-
> 27 files changed, 12093 insertions(+), 11017 deletions(-)
It is pretty hard to review this, but I've looked at the end result
remote_client_bodies.c and remote_dispatch_bodies.c files and they
both look sane. Also, the protocol definition itself hasn't been
changed, so I'm inclined to ACK this and let us deal with any fallout
as followups.
Daniel
Thanks, for reviewing this huge patch series.
I'm quite sure I didn't break anything major because the TCK still
runs fine. I also checked per function that the generated code matches
the original code in functionality. I hope that nothing slipped but I
cannot guarantee it.
So, are you just inclined to ACK or do you ACK this? :)
I'd suggest that I push this series as is to get it in the wild early
in the 0.9.2 cycle, so it can get enough testing.
I'll make the general-catch-the-rest-else cases in the generator
stricter to make the generator more robust against future additions to
the XDR protocol. I might also add the additional whitelist when we
decide that its necessary.
Matthias