[PATCH] vl, qemu-config: remove -set

Paolo Bonzini posted 1 patch 5 years ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201111135716.1209421-1-pbonzini@redhat.com
Maintainers: 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(-)
[PATCH] vl, qemu-config: remove -set
Posted by Paolo Bonzini 5 years ago
-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(-)

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;
-- 
2.26.2


Re: [PATCH] vl, qemu-config: remove -set
Posted by Daniel P. Berrangé 5 years ago
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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PATCH] vl, qemu-config: remove -set
Posted by Gerd Hoffmann 5 years ago
  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


Re: [PATCH] vl, qemu-config: remove -set
Posted by Paolo Bonzini 5 years ago
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


Re: [PATCH] vl, qemu-config: remove -set
Posted by Paolo Bonzini 5 years ago
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


Re: [PATCH] vl, qemu-config: remove -set
Posted by Markus Armbruster 5 years ago
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"?


Re: [PATCH] vl, qemu-config: remove -set
Posted by Paolo Bonzini 5 years ago
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


>