Paolo Bonzini <pbonz...@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 <pbonz...@redhat.com> I'll now look at the next patch, then get back to this one.
