2011/5/10 Daniel P. Berrange <berrange@xxxxxxxxxx>:
> On Tue, May 10, 2011 at 05:23:23PM +0200, Matthias Bolte wrote:
>> 2011/5/10 Daniel P. Berrange <berrange@xxxxxxxxxx>:
>> > On Mon, May 09, 2011 at 03:45:29PM -0600, Eric Blake wrote:
>> >> On 05/07/2011 06:28 AM, Matthias Bolte wrote:
>> >> > ---
>> >> > Âdaemon/Makefile.am         |  20 ++++-
>> >> > Âdaemon/qemu_dispatch.blacklist   |  Â3 +
>> >> > Âdaemon/qemu_dispatch.whitelist   |  Â1 +
>> >> > Âdaemon/remote_dispatch.blacklist  |  37 ++++++++
>> >> > Âdaemon/remote_dispatch.whitelist  | Â169 +++++++++++++++++++++++++++++++++++
>> >> > Âdaemon/remote_generator.pl     | Â171 +++++++++++++-----------------------
>> >> > Âsrc/Makefile.am          Â|  24 ++++-
>> >> > Âsrc/remote/qemu_client.blacklist  |  Â3 +
>> >> > Âsrc/remote/qemu_client.whitelist  |  Â1 +
>> >> > Âsrc/remote/remote_client.blacklist | Â 47 ++++++++++
>> >> > Âsrc/remote/remote_client.whitelist | Â159 +++++++++++++++++++++++++++++++++
>> >>
>> >> Hmm. ÂGiven the difference in sizes between
>> >> daemon/remote_dispatch.whitelist and src/remote/remote_client.whitelist,
>> >> there are some functions where we are only doing half the job? ÂThat
>> >> means every new API has to touch two, rather than one, file, and that's
>> >> out of a choice of four files.
>> >>
>> >> Maybe a better thing to do would be having a single file, that lists
>> >> every API, along with two states, as in:
>> >>
>> >>
>> >> # name  daemon Âsrc/remote
>> >> function yes   no
>> >>
>> >> In fact, rather than maintaining separate files, could we instead
>> >> maintain this list directly in {remote,qemu}_protocol.x, via stylized
>> >> comments?
>> >>
>> >> enum remote_procedure {
>> >> Â Â /* Each function must have a two-word comment. ÂThe first word is
>> >> Â Â Â* whether remote_generator.pl handles daemon, the second whether
>> >> Â Â Â* it handles src/remote. Â*/
>> >> Â Â REMOTE_PROC_OPEN = 1, /* yes no */
>> >> ...
>> >>
>> >> That way, when we add a new API, we are _already_ editing the file that
>> >> contains the white/blacklist, and have the precedence of the lines
>> >> beforehand to remind us whether we need to write manual code or rely on
>> >> the generator.
>> >>
>> >> Although I think that this patch does a good job as-is, I think it is
>> >> worth a v2 that avoids the extra files (the fewer files you have to edit
>> >> when adding a new API, the better).
>> >
>> > Having annotations in the .x is a nice idea. We could also annotate the
>> > methods with 'readonly' and 'readwrite' keywords, and use that to auto
>> > generate some readonly ACL checks in the dispatch code as an extra layer
>> > of defence.
>>
>> Do you mean checks regarding the VIR_CONNECT_RO flag?
>
> Yeah, either we duplicate the VIR_CONNECT_RO flag checks in
> the remote dispatch, or we could actually generate the libvirt.c
> and driver.h files too, based on the RPC.
That's an interesting idea, we can probably generate 1/2 or 2/3 of
libvirt.c that way.
> Don't need to implement this right now though.
We should get the XDR protocol, the public API and the driver API in
sync first (regarding (un)signed mismatches etc). But then I'd like to
give that a try and see how far we can get with this :)
Matthias
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list