qemu-block
[Top][All Lists]
Advanced

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

Re: [Qemu-block] [PATCH v4 05/12] block/nbd: Add nbd_has_filename_option


From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v4 05/12] block/nbd: Add nbd_has_filename_options_conflict()
Date: Mon, 3 Oct 2016 13:46:40 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0

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

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

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

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

> -        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;
>      }
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]