[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 04/25] keyval: accept escaped commas in implied option
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 04/25] keyval: accept escaped commas in implied option |
Date: |
Thu, 21 Jan 2021 13:58:38 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
Paolo Bonzini <pbonzini@redhat.com> writes:
> This is used with the weirdly-named device "SUNFD,fdtwo":
"SUNW,fdtwo"
Suggest "with weirdly-named devices such as "SUNW,fdtwo:", because we've
got more weirdos.
> $ qemu-system-sparc -device SUNW,,fdtwo,help
> SUNW,fdtwo options:
> drive=<str> - Node name or ID of a block device to use as a
> backend
> fallback=<FdcDriveType> - FDC drive type, 144/288/120/none/auto (default:
> "144")
> ...
>
> Therefore, accepting it is a preparatory step towards keyval-ifying
> -device and the device_add monitor command.
It's a preparatory step, but is it a necessary one? More on that below.
> In general, however, this
> unexpected wart of the keyval syntax leads to suboptimal errors compared
> to QemuOpts:
>
> $ ./qemu-system-x86_64 -object foo,,bar,id=obj
> qemu-system-x86_64: -object foo,,bar,id=obj: invalid object type: foo,bar
> $ storage-daemon/qemu-storage-daemon --object foo,,bar,id=obj
> qemu-storage-daemon: Invalid parameter ''
This is a second, independent argument supporting your patch.
As I remarked in reply to a prior post as "[PATCH 1/2] keyval: accept
escaped commas in implied option", the suboptimal errors could be
improved in a less invasive way. Your way has a distinct advantage,
though: a working patch.
A third argument you've put forward elsewhere, but modestly left out
here: nicer code. I'll get back to it after looking at the followup
cleanup in the next patch.
Either one argument could justify the patch, I think.
I'm this explicit to avoid the impression that the critique of the first
argument that comes next is me trying to find a reason to shoot down
your patch.
I don't think -device *needs* to accept anti-social device names.
We have a few devices with anti-social names, but none of them works
with -device, except in a help request.
We don't have to keep requests for human-readable help backwards
compible.
Anti-social device names are a usability issue with or without this
patch, with or without keyvalified -device. The patch ensures the
sugared form of the help request continues to work after keyvalification
(the unsugared from is unaffected). You could argue that loss of the
sugared form is a usability regression. Maybe. But usability is *poor*
in any case. If we really cared for it, we'd get rid of the anti-social
names.
My point is: we're sitting in a hole, and the commit message starts with
"we need to dig a bit deeper to keep us comfortable".
My first preference: get rid of the anti-social names, drop the first
argument from the commit message, and let the patch rest on the other
two.
Second preference: rephrase the commit message along the lines of "This
is a step towards keyval-ifying -device without fixing the anti-social
device names first, and without breaking backward compatibility for help
requests".
> To implement this, the flow of the parser is changed to first unescape
> everything up to the next comma or equal sign. This is done in a
> new function keyval_fetch_string for both the key and value part.
> Keys therefore are now parsed in unescaped form, but this makes no
> difference in practice because a comma is an invalid character for a
> QAPI name. Thus keys with a comma in them are rejected anyway, as
> demonstrated by the new testcase.
>
> As a side effect of the new code, parse errors are slightly improved as
> well: "Invalid parameter ''" becomes "Expected parameter before '='"
> when keyval is fed a string starting with an equal sign.
>
> The slightly baroque interface of keyval_fetch_string lets me keep the
> key parsing loop mostly untouched. It is simplified in the next patch,
> however.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
I'll now look at the next patch, then get back to this one.
- [PATCH 01/25] qemu-option: clean up id vs. list->merge_lists, (continued)
[PATCH 02/25] qemu-option: move help handling to get_opt_name_value, Paolo Bonzini, 2021/01/18
[PATCH 04/25] keyval: accept escaped commas in implied option, Paolo Bonzini, 2021/01/18
[PATCH 05/25] keyval: simplify keyval_parse_one, Paolo Bonzini, 2021/01/18
[PATCH 03/25] qemu-option: warn for short-form boolean options, Paolo Bonzini, 2021/01/18