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

Re: [libvirt] [PATCH 5/5] remote generator: Move blacklist to a file and add explicit whitelist



On Tue, May 10, 2011 at 05:23:23PM +0200, Matthias Bolte wrote:
> 2011/5/10 Daniel P. Berrange <berrange redhat com>:
> > 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.

Don't need to implement this right now though.

> > So instead of yes|no, how about just "skipgen|autogen"
> 
> Yep, more explicit, I'll go with that.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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