Re: [Qemu-devel] [PATCH] block: Catch simultaneous usage of options and their aliases

[Kevin Wolf <kwolf@redhat.com>]

Am 18.09.2014 um 16:06 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 09/18/2014 03:57 AM, Kevin Wolf wrote:
> >> While thinking about precedence of conflicting block device options from
> >> different sources, I noticed that you can specify both an option and its
> >> legacy alias at the same time (e.g. readonly=on,read-only=off). Rather
> >> than specifying the order of precedence, we should simply forbid such
> >> combinations.
> >> 
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> ---
> >>  blockdev.c | 46 +++++++++++++++++++++++++++++++---------------
> >>  tests/qemu-iotests/051     | 23 +++++++++++++++++++++++
> >>  tests/qemu-iotests/051.out | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 99 insertions(+), 15 deletions(-)
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> >>  
> >> -static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)
> >> +static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
> >> +                            Error **errp)
> >>  {
> >>      const char *value;
> >>  
> >> +    if (*errp) {
> >> +        return;
> >> +    }
> >
> > Not the most typical usage, so it might be worth a comment that this
> > function can be called with errp already set.  But since it's static,
> > it's not too hard to figure out as-is.
> 
> The problem with this usage is it doesn't combine well with the common
> usage.  That's why I purged it from the code back in May:

Oh. I didn't know that it was gone there.

> But this code is annoyingly repetitive even before the patch!  Here's
> what I'd do:
> 
>     static struct {
>         const char *from, *to;
>     } rename[] = {
>         { "iops", "throttling.iops-total" },
>         { "iops_rd", "throttling.iops-read" },
>         { "iops_wr", "throttling.iops-write" },
>         { "bps", "throttling.bps-total" },
>         { "bps_rd", "throttling.bps-read" },
>         { "bps_wr", "throttling.bps-write" },
>         { "iops_max", "throttling.iops-total-max" },
>         { "iops_rd_max", "throttling.iops-read-max" },
>         { "iops_wr_max", "throttling.iops-write-max" },
>         { "bps_max", "throttling.bps-total-max" },
>         { "bps_rd_max", "throttling.bps-read-max" },
>         { "bps_wr_max", "throttling.bps-write-max" },
>         { "iops_size", "throttling.iops-size" },
>         { "readonly", "read-only" },
>     };
>     Error *local_err = NULL;
>     int i;
> 
>     for (i = 0; i < ARRAY_SIZE(rename); i++) {
>         qemu_opt_rename(all_opts, rename[i].from, rename[i].to,
>                         &local_err);
>         if (local_err) {
>             error_report("%s", error_get_pretty(local_err));
>             error_free(local_err);
>             return NULL;
>         }
>     }

Okay, fair enough.

Kevin