Re: [Qemu-block] [PATCH v4 05/12] block/nbd: Add nbd_has_filename_options_conflict()

[Max Reitz]

On 03.10.2016 20:46, Eric Blake wrote:
> On 09/28/2016 03:55 PM, Max Reitz wrote:
>> Right now, we have four possible options that conflict with specifying
>> an NBD filename, and a future patch will add another one ("address").
>> This future option is a nested QDict that is flattened at this point,
>> requiring us to test each option whether its key has an "address."
>> prefix. Therefore, we will then need to iterate through all options.
> 
> How does the plans to add 'address.' interact with Dan's patches for
> auto-nesting when parsing command line options?
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg08238.html
> 
> I'm wondering if your patches can be made a bit smaller by basing on top
> of that work (allowing the command-line user to omit 'address.' and the
> parser auto-nests as a result, while the QMP form is always nested).

It may work, but I don't think it's going to make the patches much simpler.

First, there is no "type" option in the legacy syntax, so I'll need some
way to infer the type anyway and put the correct @type argument back
into the options QDict so that the auto-nesting would work. Therefore,
nbd_process_legacy_options() would still exist, it would just lose three
qdict_put() calls (if I can lose them at all, because with the current
state of the code I'd still have to copy host, port and path from the
QemuOpts object).

Second, I'd have write more code to remove all the options that we used
from the options QDict so that the block layer does not complain about
unused options. Currently this is done by qemu_opts_absorb_qdict() for
legacy options and qdict_extract_subqdict() for @address.

Therefore, I think writing code that makes use of auto-nesting will
actually increase the number of lines.

In any case, I'll have to rebase anyway, if nothing else then for the
renaming of the input/output visitors.

>> Adding this iteration logic now will simplify adding the new option
>> later. A nice side effect is that the user will not receive a long list
>> of five options which are not supposed to be specified with a filename,
>> but we can actually print the problematic option.
> 
> On its own, this patch looks reasonable, but I'm a bit worried that with
> all the other parsing changes going in, it may result in unused code.

I'm not sure how this code would be unused. One could argue that it's
not necessary because we have the same logic elsewhere, but I think that
using that other logic will be more complicated than just translating
the legacy syntax into a SocketAddress manually.

>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  block/nbd.c | 26 ++++++++++++++++++++------
>>  1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index c539fb5..cdab20f 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -123,6 +123,25 @@ out:
>>      return ret;
>>  }
>>  
>> +static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
>> +{
>> +    const QDictEntry *e;
>> +
>> +    for (e = qdict_first(options); e; e = qdict_next(options, e)) {
>> +        if (!strcmp(e->key, "host") ||
>> +            !strcmp(e->key, "port") ||
>> +            !strcmp(e->key, "path") ||
>> +            !strcmp(e->key, "export"))
>> +        {
>> +            error_setg(errp, "Option '%s' cannot be used with a file name",
>> +                       e->key);
>> +            return true;
>> +        }
>> +    }
> 
> Or put another way, while manual parsing looks fine, it's even better if
> we can avoid manual parsing (and having to remember to update it when
> the schema changes) by letting the schema itself automatically reject
> invalid option pairs, even if the automatic rejection doesn't give quite
> as nice of an error message.

How would the schema reject invalid option pairs? This function does not
reject invalid options pairs, it just rejects options that may not be
given when specifying the target image through a filename. I don't think
we can express that in the schema, because the schema doesn't know about
this kind of filenames (i.e. rich filenames that are translated into
options by the block driver before usage).

Other than that, there are really invalid option pairs like @host and
@path, you cannot specify both at the same time. But auto-nesting won't
solve this, as it expects the @type argument to distinguish -- which I'd
have to generate manually (at which point I would have to detect the
clash of @host and @path manually anyway).

>> +
>> +    return false;
>> +}
>> +
>>  static void nbd_parse_filename(const char *filename, QDict *options,
>>                                 Error **errp)
>>  {
>> @@ -131,12 +150,7 @@ static void nbd_parse_filename(const char *filename, 
>> QDict *options,
>>      const char *host_spec;
>>      const char *unixpath;
>>  
>> -    if (qdict_haskey(options, "host")
>> -        || qdict_haskey(options, "port")
>> -        || qdict_haskey(options, "path"))
>> -    {
> 
> Also, this patch looks like you are doing a bug fix mixed with code
> motion - the old code manually checked for duplicates on only three
> options, but the new code adds 'export' to the mix.

Well, it's not just code motion anyway. As I said in the commit message,
I'm employing a different logic anyway. I can move the fix into an own
patch, though, or add a note to the commit message, if you prefer.

Max

> 
>> -        error_setg(errp, "host/port/path and a file name may not be 
>> specified "
>> -                         "at the same time");
>> +    if (nbd_has_filename_options_conflict(options, errp)) {
>>          return;
>>      }
>>  
>>
>

signature.asc
Description: OpenPGP digital signature