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

[Matthias Bolte]

2011/5/9 Eric Blake <eblake at redhat.com>:
> 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?

Yes, seems so. I didn't yet check what's the difference and why there is one.

>  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).

Yes, that's a way better approach. I'll rework this patch to do that instead.

>> -remote_dispatch_bodies.h: $(srcdir)/remote_generator.pl $(REMOTE_PROTOCOL)
>> -     $(AM_V_GEN)perl -w $(srcdir)/remote_generator.pl -c -b remote $(REMOTE_PROTOCOL) > $@
>> +remote_dispatch_bodies.h: $(srcdir)/remote_generator.pl $(REMOTE_PROTOCOL) \
>> +             $(REMOTE_PROTOCOL_WHITELIST) $(REMOTE_PROTOCOL_BLACKLIST)
>> +     $(AM_V_GEN)perl -w $(srcdir)/remote_generator.pl -c -b remote $(REMOTE_PROTOCOL) \
>> +       $(REMOTE_PROTOCOL_WHITELIST) $(REMOTE_PROTOCOL_BLACKLIST) > $@
>
> In fact, if the white/black list is part of the .x file, then you don't
> need to tweak this part of the Makefile to pass in the names of extra files.
>
>> +++ b/daemon/remote_generator.pl
>> @@ -23,9 +23,29 @@ use Getopt::Std;
>>  our ($opt_p, $opt_t, $opt_a, $opt_r, $opt_d, $opt_c, $opt_b, $opt_k);
>>  getopts ('ptardcbk');
>>
>> -my $structprefix = $ARGV[0];
>> +my $structprefix = shift or die "missing prefix argument";
>> +my $protocol = shift or die "missing protocol argument";
>> +my $whitelistname;
>> +my $blacklistname;
>> +my @whitelist;
>> +my @blacklist;
>> +
>> +if ($opt_b or $opt_k) {
>> +    $whitelistname = shift or die "missing whitelist argument";
>> +    $blacklistname = shift or die "missing blacklist argument";
>> +
>> +    open(WHITELIST, "<$whitelistname") or die "cannot open $whitelistname: $!";
>> +    @whitelist = <WHITELIST>;
>> +    close(WHITELIST);
>> +    chomp(@whitelist);
>> +
>> +    open(BLACKLIST, "<$blacklistname") or die "cannot open $blacklistname: $!";
>> +    @blacklist = <BLACKLIST>;
>> +    close(BLACKLIST);
>> +    chomp(@blacklist);
>
> Of course, if the whitelist is maintained in the .x file itself, you'll
> have to rework this (I guess you have to read the whitelist comments
> before doing any further processing on individual functions and struct
> generation that occurred earlier in the file, but doing two passes
> through the .x file in the perl script isn't that bad).

This can still be handled in one pass. Before any output is generated
the complete .x is parsed. The while and blacklist information is
completely read before any output is generated.

>>      foreach (@keys) {
>>          # skip things which are REMOTE_MESSAGE
>>          next if $calls{$_}->{msg};
>>
>> -        if (exists($ug{$calls{$_}->{ProcName}})) {
>> +        # ignore procedures on the blacklist
>> +        if (exists($black{$calls{$_}->{ProcName}})) {
>> +            if (exists($white{$calls{$_}->{ProcName}})) {
>> +                die "procedure $calls{$_}->{ProcName} on whitelist and blacklist";
>
> And by maintaining a 'yes|no' comment in the .x file, instead of two
> separate .whitelist and .blacklist files, you merely need check for a
> well-formed comment rather than comparing two lists for duplicates or
> omissions.

Yep, embedding the while/black information in the .x files makes things simpler.

Matthias