Am 18.09.2014 um 16:06 hat Markus Armbruster geschrieben:
> Eric Blake <ebl...@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 <kw...@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 <ebl...@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
