| Message ID | 20201111135716.1209421-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vl, qemu-config: remove -set | expand |
On Wed, Nov 11, 2020 at 08:57:16AM -0500, Paolo Bonzini wrote: > -set as far as I can see has basically no use. It was intended as an override > mechanism for configuration files, but even configuration files themselves > are hardly used. Drop it with prejudice. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > docs/system/deprecated.rst | 6 ++++++ > include/qemu/config-file.h | 1 - > qemu-options.hx | 9 --------- > softmmu/vl.c | 4 ---- > util/qemu-config.c | 33 --------------------------------- > 5 files changed, 6 insertions(+), 47 deletions(-) iotest 068 uses -set and qtest vhost-user-text.c also does IOW, it looks like it is valid to use -set, even if you're not using -readconfig. Libvirt doesn't use -set, but we've had users who make use of libvirt command line passthrough for QEMU with -set. IOW, I'm not convinced real world usage is near zero as suggested. Regards, Daniel
On 11/11/20 16:03, Daniel P. Berrangé wrote: > iotest 068 uses -set and qtest vhost-user-text.c also does > > IOW, it looks like it is valid to use -set, even if you're not using > -readconfig. > > Libvirt doesn't use -set, but we've had users who make use of libvirt > command line passthrough for QEMU with -set. Hmm, indeed: https://patchwork.kernel.org/project/qemu-devel/patch/20181218041625.24969-16-mst@redhat.com/ > IOW, I'm not convinced real world usage is near zero as suggested. Yes, perhaps it's not. :) Though for both tests you pointed out it's even cleaner not to use it, there seems to be real world usage at least with "device". It is probably more viable to deprecate or even forbid usage of "-set" with anything but "device". vhost-user-test.c would still be affected, but it's a relatively small patch. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 11/11/20 16:03, Daniel P. Berrangé wrote: >> On Wed, Nov 11, 2020 at 08:57:16AM -0500, Paolo Bonzini wrote: >>> -set as far as I can see has basically no use. It was intended as an override >>> mechanism for configuration files, but even configuration files themselves >>> are hardly used. Drop it with prejudice. >>> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> docs/system/deprecated.rst | 6 ++++++ >>> include/qemu/config-file.h | 1 - >>> qemu-options.hx | 9 --------- >>> softmmu/vl.c | 4 ---- >>> util/qemu-config.c | 33 --------------------------------- >>> 5 files changed, 6 insertions(+), 47 deletions(-) >> >> iotest 068 uses -set and qtest vhost-user-text.c also does >> IOW, it looks like it is valid to use -set, even if you're not using >> -readconfig. Of course that's valid. >> Libvirt doesn't use -set, but we've had users who make use of >> libvirt >> command line passthrough for QEMU with -set. > > Hmm, indeed: > > https://patchwork.kernel.org/project/qemu-devel/patch/20181218041625.24969-16-mst@redhat.com/ Such monkey-patching may not be wise, but unwise != invalid. >> IOW, I'm not convinced real world usage is near zero as suggested. Guessing the gamut of usage out there in the real world correctly is always a tall order :) > Yes, perhaps it's not. :) Though for both tests you pointed out it's > even cleaner not to use it, there seems to be real world usage at > least with "device". I have common test configurations files for -readconfig. I've used -set for quick monkey-patching once in a great while. Now, such ad hoc use is a *weak* argument against ditching the feature. But it does undermine the "basically no use" proposition. > It is probably more viable to deprecate or even forbid usage of "-set" > with anything but "device". vhost-user-test.c would still be > affected, but it's a relatively small patch. Deprecating only some uses buys us next to nothing, I think. If we want to deprecate it, just deprecate it. Immediate removal of -set / rejection of -set for some option groups needs more justification than just "I think we can get away with it": there has to be a tangible benefit. What would immediate removal buy us over the orthodox "deprecate, wait for grace period to expire, remove"?
Il gio 12 nov 2020, 07:55 Markus Armbruster <armbru@redhat.com> ha scritto: > Immediate removal of -set / rejection of -set for some option groups > needs more justification than just "I think we can get away with it": > there has to be a tangible benefit. The benefit is being able to switch parsers (for everything but -device to begin with) away from QemuOpts. Non-scalar priorities have a use for machine, accel and object, but for now not for device. Alternatively I can: 1) keep QemuOpts as the parser, and turn the options into qdict, instead of using keyval 2) move -set out of config-file.c and into vl.c, so that I can special case the monkey patching. What would immediate removal buy us > over the orthodox "deprecate, wait for grace period to expire, remove"? > The problem is that, at least for -device, I don't think we can reasonably remove it without a replacement. Paolo >
Hi, > IOW, it looks like it is valid to use -set, even if you're not using > -readconfig. Yes, that is quite useful for setting device properties which are not (yet) supported by libvirt, like this: <qemu:commandline> <qemu:arg value='-set'/> <qemu:arg value='device.video0.edid=on'/> </qemu:commandline> Grepping through my libvirt domain config files I see 90% is indeed "-set device". But I've also found netdev (set tftp+bootfile for "<interface type='user'>"). take care, Gerd
On 12/11/20 12:26, Gerd Hoffmann wrote: > Yes, that is quite useful for setting device properties which are not > (yet) supported by libvirt, like this: > > <qemu:commandline> > <qemu:arg value='-set'/> > <qemu:arg value='device.video0.edid=on'/> > </qemu:commandline> > > Grepping through my libvirt domain config files I see 90% is > indeed "-set device". But I've also found netdev (set tftp+bootfile > for "<interface type='user'>"). Hmm... Looks like I will move -set from config-file to vl.c and handle both QemuOpts and keyval-based options. Paolo
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 9aa1ab268b..f7eede1b68 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -719,6 +719,12 @@ topologies described with -smp include all possible cpus, i.e. The ``enforce-config-section`` property was replaced by the ``-global migration.send-configuration={on|off}`` option. +``-set`` (removed 6.0) +'''''''''''''''''''''' + +The ``-set`` option was removed with no replacement and no prior +deprecation period. + Block devices ------------- diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h index d74f920152..29226107bd 100644 --- a/include/qemu/config-file.h +++ b/include/qemu/config-file.h @@ -8,7 +8,6 @@ QemuOpts *qemu_find_opts_singleton(const char *group); void qemu_add_opts(QemuOptsList *list); void qemu_add_drive_opts(QemuOptsList *list); -int qemu_set_option(const char *str); int qemu_global_option(const char *str); void qemu_config_write(FILE *fp); diff --git a/qemu-options.hx b/qemu-options.hx index 2c83390504..45c503cb8f 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -371,15 +371,6 @@ SRST -drive file=/dev/fdset/2,index=0,media=disk ERST -DEF("set", HAS_ARG, QEMU_OPTION_set, - "-set group.id.arg=value\n" - " set <arg> parameter for item <id> of type <group>\n" - " i.e. -set drive.$id.file=/path/to/image\n", QEMU_ARCH_ALL) -SRST -``-set group.id.arg=value`` - Set parameter arg for item id of type group -ERST - DEF("global", HAS_ARG, QEMU_OPTION_global, "-global driver.property=value\n" "-global driver=driver,property=property,value=value\n" diff --git a/softmmu/vl.c b/softmmu/vl.c index 50c4850064..f66a4eb9c2 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2652,10 +2652,6 @@ void qemu_init(int argc, char **argv, char **envp) exit(1); } break; - case QEMU_OPTION_set: - if (qemu_set_option(optarg) != 0) - exit(1); - break; case QEMU_OPTION_global: if (qemu_global_option(optarg) != 0) exit(1); diff --git a/util/qemu-config.c b/util/qemu-config.c index bcae298b71..1a7b7e951c 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -313,39 +313,6 @@ void qemu_add_opts(QemuOptsList *list) abort(); } -int qemu_set_option(const char *str) -{ - Error *local_err = NULL; - char group[64], id[64], arg[64]; - QemuOptsList *list; - QemuOpts *opts; - int rc, offset; - - rc = sscanf(str, "%63[^.].%63[^.].%63[^=]%n", group, id, arg, &offset); - if (rc < 3 || str[offset] != '=') { - error_report("can't parse: \"%s\"", str); - return -1; - } - - list = qemu_find_opts(group); - if (list == NULL) { - return -1; - } - - opts = qemu_opts_find(list, id); - if (!opts) { - error_report("there is no %s \"%s\" defined", - list->name, id); - return -1; - } - - if (!qemu_opt_set(opts, arg, str + offset + 1, &local_err)) { - error_report_err(local_err); - return -1; - } - return 0; -} - struct ConfigWriteData { QemuOptsList *list; FILE *fp;
-set as far as I can see has basically no use. It was intended as an override mechanism for configuration files, but even configuration files themselves are hardly used. Drop it with prejudice. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/system/deprecated.rst | 6 ++++++ include/qemu/config-file.h | 1 - qemu-options.hx | 9 --------- softmmu/vl.c | 4 ---- util/qemu-config.c | 33 --------------------------------- 5 files changed, 6 insertions(+), 47 deletions(-)