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(a)redhat.com&gt;: > > 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.