[PATCH 0/3] vl: add -object support back into -readconfig

[PATCH 0/3] vl: add -object support back into -readconfig

[Paolo Bonzini]

LXD developers have reported that [object] stanzas have stopped
working in configuration files.

The problem is that QEMU 6.0 switched the creation of objects from
qemu_opts_foreach to a bespoke QTAILQ, in preparation for supporting
JSON syntax in -object.  Entries from the configuration file however
do not go through object_option_parse, and are thus lost.  Of the many
fixes that are possible, I chose one that is slightly more invasive but
more consistent with the plans for keyval-ification of options such as
-M and -accel.

-set was also broken by the same change, but for simplicity I chose
not to add it back yet.  However, this series will report the
breakage instead of failing silently.

The first two patches of this series are thus a reduced version of
https://patchew.org/QEMU/20210513162901.1310239-1-pbonzini@redhat.com/
([PATCH 00/14] vl: compound properties for machines and accelerators),
with the -set infrastructure removed.  The third is very simple and
uses the newly-provided hooks to parse objects from configuration files.

Paolo Bonzini (3):
  qemu-config: parse configuration files to a QDict
  vl: plumb keyval-based options into -readconfig
  vl: plug -object back into -readconfig

 include/block/qdict.h      |   2 -
 include/qapi/qmp/qdict.h   |   3 ++
 include/qemu/config-file.h |   7 ++-
 softmmu/vl.c               | 104 ++++++++++++++++++++++++++-----------
 util/qemu-config.c         |  91 +++++++++++++++++++++-----------
 5 files changed, 145 insertions(+), 62 deletions(-)

-- 
2.27.0

[PATCH 1/3] qemu-config: parse configuration files to a QDict

[Paolo Bonzini]

Change the parser to put the values into a QDict and pass them
to a callback.  qemu_config_parse's QemuOpts creation is
itself turned into a callback function.

This is useful for -readconfig to support keyval-based options;
getting a QDict from the parser removes a roundtrip from
QDict to QemuOpts and then back to QDict.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/config-file.h |  7 ++-
 softmmu/vl.c               |  4 +-
 util/qemu-config.c         | 91 +++++++++++++++++++++++++-------------
 3 files changed, 69 insertions(+), 33 deletions(-)

diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 0500b3668d..f605423321 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -1,6 +1,8 @@
 #ifndef QEMU_CONFIG_FILE_H
 #define QEMU_CONFIG_FILE_H
 
+typedef void QEMUConfigCB(const char *group, QDict *qdict, void *opaque, Error **errp);
+
 void qemu_load_module_for_opts(const char *group);
 QemuOptsList *qemu_find_opts(const char *group);
 QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
@@ -14,7 +16,10 @@ void qemu_config_write(FILE *fp);
 int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname,
                       Error **errp);
 
-int qemu_read_config_file(const char *filename, Error **errp);
+/* A default callback for qemu_read_config_file().  */
+void qemu_config_do_parse(const char *group, QDict *qdict, void *opaque, Error **errp);
+
+int qemu_read_config_file(const char *filename, QEMUConfigCB *f, Error **errp);
 
 /* Parse QDict options as a replacement for a config file (allowing multiple
    enumerated (0..(n-1)) configuration "sections") */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index d55424e634..594de0864c 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2121,7 +2121,7 @@ static void qemu_read_default_config_file(Error **errp)
     int ret;
     g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR "/qemu.conf");
 
-    ret = qemu_read_config_file(file, errp);
+    ret = qemu_read_config_file(file, qemu_config_do_parse, errp);
     if (ret < 0) {
         if (ret == -ENOENT) {
             error_free(*errp);
@@ -3387,7 +3387,7 @@ void qemu_init(int argc, char **argv, char **envp)
                 qemu_plugin_opt_parse(optarg, &plugin_list);
                 break;
             case QEMU_OPTION_readconfig:
-                qemu_read_config_file(optarg, &error_fatal);
+                qemu_read_config_file(optarg, qemu_config_do_parse, &error_fatal);
                 break;
             case QEMU_OPTION_spice:
                 olist = qemu_find_opts_err("spice", NULL);
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 34974c4b47..7bca153c6c 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -351,19 +351,19 @@ void qemu_config_write(FILE *fp)
 }
 
 /* Returns number of config groups on success, -errno on error */
-int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error **errp)
+static int qemu_config_foreach(FILE *fp, QEMUConfigCB *cb, void *opaque,
+                               const char *fname, Error **errp)
 {
-    char line[1024], group[64], id[64], arg[64], value[1024];
+    char line[1024], prev_group[64], group[64], arg[64], value[1024];
     Location loc;
-    QemuOptsList *list = NULL;
     Error *local_err = NULL;
-    QemuOpts *opts = NULL;
+    QDict *qdict = NULL;
     int res = -EINVAL, lno = 0;
     int count = 0;
 
     loc_push_none(&loc);
     while (fgets(line, sizeof(line), fp) != NULL) {
-        loc_set_file(fname, ++lno);
+        ++lno;
         if (line[0] == '\n') {
             /* skip empty lines */
             continue;
@@ -372,39 +372,39 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error *
             /* comment */
             continue;
         }
-        if (sscanf(line, "[%63s \"%63[^\"]\"]", group, id) == 2) {
-            /* group with id */
-            list = find_list(lists, group, &local_err);
-            if (local_err) {
-                error_propagate(errp, local_err);
-                goto out;
+        if (line[0] == '[') {
+            QDict *prev = qdict;
+            if (sscanf(line, "[%63s \"%63[^\"]\"]", group, value) == 2) {
+                qdict = qdict_new();
+                qdict_put_str(qdict, "id", value);
+                count++;
+            } else if (sscanf(line, "[%63[^]]]", group) == 1) {
+                qdict = qdict_new();
+                count++;
             }
-            opts = qemu_opts_create(list, id, 1, NULL);
-            count++;
-            continue;
-        }
-        if (sscanf(line, "[%63[^]]]", group) == 1) {
-            /* group without id */
-            list = find_list(lists, group, &local_err);
-            if (local_err) {
-                error_propagate(errp, local_err);
-                goto out;
+            if (qdict != prev) {
+                if (prev) {
+                    cb(prev_group, prev, opaque, &local_err);
+                    qobject_unref(prev);
+                    if (local_err) {
+                        error_propagate(errp, local_err);
+                        goto out;
+                    }
+                }
+                strcpy(prev_group, group);
+                continue;
             }
-            opts = qemu_opts_create(list, NULL, 0, &error_abort);
-            count++;
-            continue;
         }
+        loc_set_file(fname, lno);
         value[0] = '\0';
         if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 ||
             sscanf(line, " %63s = \"\"", arg) == 1) {
             /* arg = value */
-            if (opts == NULL) {
+            if (qdict == NULL) {
                 error_setg(errp, "no group defined");
                 goto out;
             }
-            if (!qemu_opt_set(opts, arg, value, errp)) {
-                goto out;
-            }
+            qdict_put_str(qdict, arg, value);
             continue;
         }
         error_setg(errp, "parse error");
@@ -418,10 +418,41 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error *
     res = count;
 out:
     loc_pop(&loc);
+    if (qdict) {
+        cb(group, qdict, opaque, errp);
+        qobject_unref(qdict);
+    }
     return res;
 }
 
-int qemu_read_config_file(const char *filename, Error **errp)
+void qemu_config_do_parse(const char *group, QDict *qdict, void *opaque, Error **errp)
+{
+    QemuOptsList **lists = opaque;
+    const char *id = qdict_get_try_str(qdict, "id");
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = find_list(lists, group, errp);
+    if (!list) {
+        return;
+    }
+
+    opts = qemu_opts_create(list, id, 1, errp);
+    if (!opts) {
+        return;
+    }
+    if (id) {
+        qdict_del(qdict, "id");
+    }
+    qemu_opts_absorb_qdict(opts, qdict, errp);
+}
+
+int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error **errp)
+{
+    return qemu_config_foreach(fp, qemu_config_do_parse, lists, fname, errp);
+}
+
+int qemu_read_config_file(const char *filename, QEMUConfigCB *cb, Error **errp)
 {
     FILE *f = fopen(filename, "r");
     int ret;
@@ -431,7 +462,7 @@ int qemu_read_config_file(const char *filename, Error **errp)
         return -errno;
     }
 
-    ret = qemu_config_parse(f, vm_config_groups, filename, errp);
+    ret = qemu_config_foreach(f, cb, vm_config_groups, filename, errp);
     fclose(f);
     return ret;
 }
-- 
2.27.0

[PATCH 2/3] vl: plumb keyval-based options into -readconfig

[Paolo Bonzini]

Let -readconfig support parsing configuration file groups into QDict in
addition to the previous behavior of recording them into QemuOpts.
This will be used to add back support for objects in -readconfig.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/block/qdict.h    |  2 -
 include/qapi/qmp/qdict.h |  3 ++
 softmmu/vl.c             | 82 ++++++++++++++++++++++++++++------------
 3 files changed, 61 insertions(+), 26 deletions(-)

diff --git a/include/block/qdict.h b/include/block/qdict.h
index d8cb502d7d..ced2acfb92 100644
--- a/include/block/qdict.h
+++ b/include/block/qdict.h
@@ -20,8 +20,6 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite);
 void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
 void qdict_array_split(QDict *src, QList **dst);
 int qdict_array_entries(QDict *src, const char *subqdict);
-QObject *qdict_crumple(const QDict *src, Error **errp);
-void qdict_flatten(QDict *qdict);
 
 typedef struct QDictRenames {
     const char *from;
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 9934539c1b..d5b5430e21 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -64,4 +64,7 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key);
 
 QDict *qdict_clone_shallow(const QDict *src);
 
+QObject *qdict_crumple(const QDict *src, Error **errp);
+void qdict_flatten(QDict *qdict);
+
 #endif /* QDICT_H */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 594de0864c..90e491cc0c 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -121,6 +121,7 @@
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qapi-visit-qom.h"
 #include "qapi/qapi-commands-ui.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "sysemu/iothread.h"
 #include "qemu/guest-random.h"
@@ -2115,13 +2116,52 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
     return 0;
 }
 
+/*
+ * Return whether configuration group @group is stored in QemuOpts, or
+ * recorded as one or more QDicts by qemu_record_config_group.
+ */
+static bool is_qemuopts_group(const char *group)
+{
+    return true;
+}
+
+static void qemu_record_config_group(const char *group, QDict *dict, bool from_json, Error **errp)
+{
+    abort();
+}
+
+/*
+ * Parse non-QemuOpts config file groups, pass the rest to
+ * qemu_config_do_parse.
+ */
+static void qemu_parse_config_group(const char *group, QDict *qdict,
+                                    void *opaque, Error **errp)
+{
+    QObject *crumpled;
+    if (is_qemuopts_group(group)) {
+        qemu_config_do_parse(group, qdict, opaque, errp);
+        return;
+    }
+
+    crumpled = qdict_crumple(qdict, errp);
+    if (!crumpled) {
+        return;
+    }
+    if (qobject_type(crumpled) != QTYPE_QDICT) {
+        assert(qobject_type(crumpled) == QTYPE_QLIST);
+        error_setg(errp, "Lists cannot be at top level of a configuration section");
+        return;
+    }
+    qemu_record_config_group(group, qobject_to(QDict, crumpled), false, errp);
+}
+
 static void qemu_read_default_config_file(Error **errp)
 {
     ERRP_GUARD();
     int ret;
     g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR "/qemu.conf");
 
-    ret = qemu_read_config_file(file, qemu_config_do_parse, errp);
+    ret = qemu_read_config_file(file, qemu_parse_config_group, errp);
     if (ret < 0) {
         if (ret == -ENOENT) {
             error_free(*errp);
@@ -2130,9 +2170,8 @@ static void qemu_read_default_config_file(Error **errp)
     }
 }
 
-static int qemu_set_option(const char *str)
+static void qemu_set_option(const char *str, Error **errp)
 {
-    Error *local_err = NULL;
     char group[64], id[64], arg[64];
     QemuOptsList *list;
     QemuOpts *opts;
@@ -2140,27 +2179,23 @@ static int qemu_set_option(const char *str)
 
     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;
+        error_setg(errp, "can't parse: \"%s\"", str);
+        return;
     }
 
-    if (!qemu_opt_set(opts, arg, str + offset + 1, &local_err)) {
-        error_report_err(local_err);
-        return -1;
+    if (!is_qemuopts_group(group)) {
+        error_setg(errp, "-set is not supported with %s", group);
+    } else {
+        list = qemu_find_opts_err(group, errp);
+        if (list) {
+            opts = qemu_opts_find(list, id);
+            if (!opts) {
+                error_setg(errp, "there is no %s \"%s\" defined", group, id);
+                return;
+            }
+            qemu_opt_set(opts, arg, str + offset + 1, errp);
+        }
     }
-    return 0;
 }
 
 static void user_register_global_props(void)
@@ -2754,8 +2789,7 @@ void qemu_init(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_set:
-                if (qemu_set_option(optarg) != 0)
-                    exit(1);
+                qemu_set_option(optarg, &error_fatal);
                 break;
             case QEMU_OPTION_global:
                 if (qemu_global_option(optarg) != 0)
@@ -3387,7 +3421,7 @@ void qemu_init(int argc, char **argv, char **envp)
                 qemu_plugin_opt_parse(optarg, &plugin_list);
                 break;
             case QEMU_OPTION_readconfig:
-                qemu_read_config_file(optarg, qemu_config_do_parse, &error_fatal);
+                qemu_read_config_file(optarg, qemu_parse_config_group, &error_fatal);
                 break;
             case QEMU_OPTION_spice:
                 olist = qemu_find_opts_err("spice", NULL);
-- 
2.27.0

[PATCH 3/3] vl: plug -object back into -readconfig

[Paolo Bonzini]

Commit bc2f4fcb1d ("qom: move user_creatable_add_opts logic to vl.c
and QAPIfy it", 2021-03-19) switched the creation of objects from
qemu_opts_foreach to a bespoke QTAILQ in preparation for supporting JSON
syntax in -object.

Unfortunately in doing so it lost support for [object] stanzas in
configuration files and also for "-set object.ID.KEY=VAL".  The latter
is hard to re-establish and probably best solved by deprecating -set.
This patch uses the infrastructure introduced by the previous two
patches in order to parse QOM objects correctly from configuration
files.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/vl.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 90e491cc0c..a8abf8e740 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1708,9 +1708,15 @@ static void object_option_foreach_add(bool (*type_opt_predicate)(const char *))
     }
 }
 
+static void object_option_add_visitor(Visitor *v)
+{
+    ObjectOption *opt = g_new0(ObjectOption, 1);
+    visit_type_ObjectOptions(v, NULL, &opt->opts, &error_fatal);
+    QTAILQ_INSERT_TAIL(&object_opts, opt, next);
+}
+
 static void object_option_parse(const char *optarg)
 {
-    ObjectOption *opt;
     QemuOpts *opts;
     const char *type;
     Visitor *v;
@@ -1738,11 +1744,8 @@ static void object_option_parse(const char *optarg)
         v = opts_visitor_new(opts);
     }
 
-    opt = g_new0(ObjectOption, 1);
-    visit_type_ObjectOptions(v, NULL, &opt->opts, &error_fatal);
+    object_option_add_visitor(v);
     visit_free(v);
-
-    QTAILQ_INSERT_TAIL(&object_opts, opt, next);
 }
 
 /*
@@ -2122,12 +2125,21 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
  */
 static bool is_qemuopts_group(const char *group)
 {
+    if (g_str_equal(group, "object")) {
+        return false;
+    }
     return true;
 }
 
 static void qemu_record_config_group(const char *group, QDict *dict, bool from_json, Error **errp)
 {
-    abort();
+    if (g_str_equal(group, "object")) {
+        Visitor *v = qobject_input_visitor_new_keyval(QOBJECT(dict));
+        object_option_add_visitor(v);
+        visit_free(v);
+    } else {
+        abort();
+    }
 }
 
 /*
-- 
2.27.0

Re: [PATCH 0/3] vl: add -object support back into -readconfig

[Kevin Wolf]

Am 18.05.2021 um 17:40 hat Paolo Bonzini geschrieben:
> LXD developers have reported that [object] stanzas have stopped
> working in configuration files.
> 
> The problem is that QEMU 6.0 switched the creation of objects from
> qemu_opts_foreach to a bespoke QTAILQ, in preparation for supporting
> JSON syntax in -object.  Entries from the configuration file however
> do not go through object_option_parse, and are thus lost.  Of the many
> fixes that are possible, I chose one that is slightly more invasive but
> more consistent with the plans for keyval-ification of options such as
> -M and -accel.
> 
> -set was also broken by the same change, but for simplicity I chose
> not to add it back yet.  However, this series will report the
> breakage instead of failing silently.
> 
> The first two patches of this series are thus a reduced version of
> https://patchew.org/QEMU/20210513162901.1310239-1-pbonzini@redhat.com/
> ([PATCH 00/14] vl: compound properties for machines and accelerators),
> with the -set infrastructure removed.  The third is very simple and
> uses the newly-provided hooks to parse objects from configuration files.

Looks like this is
Based-on: <20210518131542.2941207-1-pbonzini@redhat.com>

Kevin

Re: [PATCH 0/3] vl: add -object support back into -readconfig

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210518154014.2999326-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210518154014.2999326-1-pbonzini@redhat.com
Subject: [PATCH 0/3] vl: add -object support back into -readconfig

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210518154014.2999326-1-pbonzini@redhat.com -> patchew/20210518154014.2999326-1-pbonzini@redhat.com
 - [tag update]      patchew/20210519104433.16870-1-jcmvbkbc@gmail.com -> patchew/20210519104433.16870-1-jcmvbkbc@gmail.com
 - [tag update]      patchew/20210519113528.12474-1-davoronetskiy@gmail.com -> patchew/20210519113528.12474-1-davoronetskiy@gmail.com
Switched to a new branch 'test'
0124ce7 vl: plug -object back into -readconfig
e0cb857 vl: plumb keyval-based options into -readconfig
2b01956 qemu-config: parse configuration files to a QDict

=== OUTPUT BEGIN ===
1/3 Checking commit 2b0195631f20 (qemu-config: parse configuration files to a QDict)
WARNING: line over 80 characters
#35: FILE: include/qemu/config-file.h:4:
+typedef void QEMUConfigCB(const char *group, QDict *qdict, void *opaque, Error **errp);

WARNING: line over 80 characters
#46: FILE: include/qemu/config-file.h:20:
+void qemu_config_do_parse(const char *group, QDict *qdict, void *opaque, Error **errp);

WARNING: line over 80 characters
#70: FILE: softmmu/vl.c:3389:
+                qemu_read_config_file(optarg, qemu_config_do_parse, &error_fatal);

WARNING: line over 80 characters
#178: FILE: util/qemu-config.c:428:
+void qemu_config_do_parse(const char *group, QDict *qdict, void *opaque, Error **errp)

WARNING: line over 80 characters
#200: FILE: util/qemu-config.c:450:
+int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error **errp)

total: 0 errors, 5 warnings, 171 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit e0cb8571d7a2 (vl: plumb keyval-based options into -readconfig)
ERROR: line over 90 characters
#73: FILE: softmmu/vl.c:2127:
+static void qemu_record_config_group(const char *group, QDict *dict, bool from_json, Error **errp)

WARNING: line over 80 characters
#97: FILE: softmmu/vl.c:2151:
+        error_setg(errp, "Lists cannot be at top level of a configuration section");

WARNING: line over 80 characters
#182: FILE: softmmu/vl.c:3423:
+                qemu_read_config_file(optarg, qemu_parse_config_group, &error_fatal);

total: 1 errors, 2 warnings, 143 lines checked

Patch 2/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/3 Checking commit 0124ce72a634 (vl: plug -object back into -readconfig)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210518154014.2999326-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/3] qemu-config: parse configuration files to a QDict

[Kevin Wolf]

Am 18.05.2021 um 17:40 hat Paolo Bonzini geschrieben:
> Change the parser to put the values into a QDict and pass them
> to a callback.  qemu_config_parse's QemuOpts creation is
> itself turned into a callback function.
> 
> This is useful for -readconfig to support keyval-based options;
> getting a QDict from the parser removes a roundtrip from
> QDict to QemuOpts and then back to QDict.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 34974c4b47..7bca153c6c 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -351,19 +351,19 @@ void qemu_config_write(FILE *fp)
>  }
>  
>  /* Returns number of config groups on success, -errno on error */
> -int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error **errp)
> +static int qemu_config_foreach(FILE *fp, QEMUConfigCB *cb, void *opaque,
> +                               const char *fname, Error **errp)
>  {
> -    char line[1024], group[64], id[64], arg[64], value[1024];
> +    char line[1024], prev_group[64], group[64], arg[64], value[1024];
>      Location loc;
> -    QemuOptsList *list = NULL;
>      Error *local_err = NULL;
> -    QemuOpts *opts = NULL;
> +    QDict *qdict = NULL;
>      int res = -EINVAL, lno = 0;
>      int count = 0;
>  
>      loc_push_none(&loc);
>      while (fgets(line, sizeof(line), fp) != NULL) {
> -        loc_set_file(fname, ++lno);
> +        ++lno;
>          if (line[0] == '\n') {
>              /* skip empty lines */
>              continue;
> @@ -372,39 +372,39 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error *
>              /* comment */
>              continue;
>          }
> -        if (sscanf(line, "[%63s \"%63[^\"]\"]", group, id) == 2) {
> -            /* group with id */
> -            list = find_list(lists, group, &local_err);
> -            if (local_err) {
> -                error_propagate(errp, local_err);
> -                goto out;
> +        if (line[0] == '[') {
> +            QDict *prev = qdict;
> +            if (sscanf(line, "[%63s \"%63[^\"]\"]", group, value) == 2) {
> +                qdict = qdict_new();
> +                qdict_put_str(qdict, "id", value);
> +                count++;
> +            } else if (sscanf(line, "[%63[^]]]", group) == 1) {
> +                qdict = qdict_new();
> +                count++;
>              }
> -            opts = qemu_opts_create(list, id, 1, NULL);
> -            count++;
> -            continue;
> -        }
> -        if (sscanf(line, "[%63[^]]]", group) == 1) {
> -            /* group without id */
> -            list = find_list(lists, group, &local_err);
> -            if (local_err) {
> -                error_propagate(errp, local_err);
> -                goto out;
> +            if (qdict != prev) {
> +                if (prev) {
> +                    cb(prev_group, prev, opaque, &local_err);
> +                    qobject_unref(prev);
> +                    if (local_err) {
> +                        error_propagate(errp, local_err);
> +                        goto out;
> +                    }
> +                }
> +                strcpy(prev_group, group);
> +                continue;
>              }
> -            opts = qemu_opts_create(list, NULL, 0, &error_abort);
> -            count++;
> -            continue;
>          }
> +        loc_set_file(fname, lno);

Error reporting is going to suffer quite a bit from this delayed
parsing, reporting the last line of a group for most cases now.

I think it's acceptable for an option that we want to get rid of in the
long term anyway, but worth noting.

>          value[0] = '\0';
>          if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 ||
>              sscanf(line, " %63s = \"\"", arg) == 1) {
>              /* arg = value */
> -            if (opts == NULL) {
> +            if (qdict == NULL) {
>                  error_setg(errp, "no group defined");
>                  goto out;
>              }
> -            if (!qemu_opt_set(opts, arg, value, errp)) {
> -                goto out;
> -            }
> +            qdict_put_str(qdict, arg, value);
>              continue;
>          }
>          error_setg(errp, "parse error");
> @@ -418,10 +418,41 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error *
>      res = count;
>  out:
>      loc_pop(&loc);
> +    if (qdict) {
> +        cb(group, qdict, opaque, errp);
> +        qobject_unref(qdict);
> +    }
>      return res;
>  }
>  
> -int qemu_read_config_file(const char *filename, Error **errp)
> +void qemu_config_do_parse(const char *group, QDict *qdict, void *opaque, Error **errp)
> +{
> +    QemuOptsList **lists = opaque;
> +    const char *id = qdict_get_try_str(qdict, "id");
> +    QemuOptsList *list;
> +    QemuOpts *opts;
> +
> +    list = find_list(lists, group, errp);
> +    if (!list) {
> +        return;
> +    }
> +
> +    opts = qemu_opts_create(list, id, 1, errp);
> +    if (!opts) {
> +        return;
> +    }
> +    if (id) {
> +        qdict_del(qdict, "id");
> +    }
> +    qemu_opts_absorb_qdict(opts, qdict, errp);

Shouldn't we check that qdict is empty now and return an error if there
are any options that the QemuOptsList doesn't accept?

Kevin

Re: [PATCH 0/3] vl: add -object support back into -readconfig

[Paolo Bonzini]

On 19/05/21 15:58, Kevin Wolf wrote:
> Am 18.05.2021 um 17:40 hat Paolo Bonzini geschrieben:
>> LXD developers have reported that [object] stanzas have stopped
>> working in configuration files.
>>
>> The problem is that QEMU 6.0 switched the creation of objects from
>> qemu_opts_foreach to a bespoke QTAILQ, in preparation for supporting
>> JSON syntax in -object.  Entries from the configuration file however
>> do not go through object_option_parse, and are thus lost.  Of the many
>> fixes that are possible, I chose one that is slightly more invasive but
>> more consistent with the plans for keyval-ification of options such as
>> -M and -accel.
>>
>> -set was also broken by the same change, but for simplicity I chose
>> not to add it back yet.  However, this series will report the
>> breakage instead of failing silently.
>>
>> The first two patches of this series are thus a reduced version of
>> https://patchew.org/QEMU/20210513162901.1310239-1-pbonzini@redhat.com/
>> ([PATCH 00/14] vl: compound properties for machines and accelerators),
>> with the -set infrastructure removed.  The third is very simple and
>> uses the newly-provided hooks to parse objects from configuration files.
> 
> Looks like this is
> Based-on: <20210518131542.2941207-1-pbonzini@redhat.com>

Just context, but yes it is.

Paolo

Re: [PATCH 2/3] vl: plumb keyval-based options into -readconfig

[Kevin Wolf]

Am 18.05.2021 um 17:40 hat Paolo Bonzini geschrieben:
> Let -readconfig support parsing configuration file groups into QDict in
> addition to the previous behavior of recording them into QemuOpts.
> This will be used to add back support for objects in -readconfig.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/block/qdict.h    |  2 -
>  include/qapi/qmp/qdict.h |  3 ++
>  softmmu/vl.c             | 82 ++++++++++++++++++++++++++++------------
>  3 files changed, 61 insertions(+), 26 deletions(-)
> 
> diff --git a/include/block/qdict.h b/include/block/qdict.h
> index d8cb502d7d..ced2acfb92 100644
> --- a/include/block/qdict.h
> +++ b/include/block/qdict.h
> @@ -20,8 +20,6 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite);
>  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
>  void qdict_array_split(QDict *src, QList **dst);
>  int qdict_array_entries(QDict *src, const char *subqdict);
> -QObject *qdict_crumple(const QDict *src, Error **errp);
> -void qdict_flatten(QDict *qdict);
>  
>  typedef struct QDictRenames {
>      const char *from;
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 9934539c1b..d5b5430e21 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -64,4 +64,7 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key);
>  
>  QDict *qdict_clone_shallow(const QDict *src);
>  
> +QObject *qdict_crumple(const QDict *src, Error **errp);
> +void qdict_flatten(QDict *qdict);
> +
>  #endif /* QDICT_H */
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 594de0864c..90e491cc0c 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -121,6 +121,7 @@
>  #include "qapi/qapi-commands-misc.h"
>  #include "qapi/qapi-visit-qom.h"
>  #include "qapi/qapi-commands-ui.h"
> +#include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
>  #include "sysemu/iothread.h"
>  #include "qemu/guest-random.h"
> @@ -2115,13 +2116,52 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      return 0;
>  }
>  
> +/*
> + * Return whether configuration group @group is stored in QemuOpts, or
> + * recorded as one or more QDicts by qemu_record_config_group.
> + */
> +static bool is_qemuopts_group(const char *group)
> +{
> +    return true;
> +}
> +
> +static void qemu_record_config_group(const char *group, QDict *dict, bool from_json, Error **errp)

This is a bit more than 80 characters.

> +{
> +    abort();
> +}
> +
> +/*
> + * Parse non-QemuOpts config file groups, pass the rest to
> + * qemu_config_do_parse.
> + */
> +static void qemu_parse_config_group(const char *group, QDict *qdict,
> +                                    void *opaque, Error **errp)
> +{
> +    QObject *crumpled;
> +    if (is_qemuopts_group(group)) {
> +        qemu_config_do_parse(group, qdict, opaque, errp);
> +        return;
> +    }
> +
> +    crumpled = qdict_crumple(qdict, errp);
> +    if (!crumpled) {
> +        return;
> +    }
> +    if (qobject_type(crumpled) != QTYPE_QDICT) {
> +        assert(qobject_type(crumpled) == QTYPE_QLIST);
> +        error_setg(errp, "Lists cannot be at top level of a configuration section");

This line is too long, too.

> +        return;
> +    }
> +    qemu_record_config_group(group, qobject_to(QDict, crumpled), false, errp);
> +}
> +
>  static void qemu_read_default_config_file(Error **errp)
>  {
>      ERRP_GUARD();
>      int ret;
>      g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR "/qemu.conf");
>  
> -    ret = qemu_read_config_file(file, qemu_config_do_parse, errp);
> +    ret = qemu_read_config_file(file, qemu_parse_config_group, errp);
>      if (ret < 0) {
>          if (ret == -ENOENT) {
>              error_free(*errp);
> @@ -2130,9 +2170,8 @@ static void qemu_read_default_config_file(Error **errp)
>      }
>  }
>  
> -static int qemu_set_option(const char *str)
> +static void qemu_set_option(const char *str, Error **errp)
>  {
> -    Error *local_err = NULL;
>      char group[64], id[64], arg[64];
>      QemuOptsList *list;
>      QemuOpts *opts;
> @@ -2140,27 +2179,23 @@ static int qemu_set_option(const char *str)
>  
>      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;
> +        error_setg(errp, "can't parse: \"%s\"", str);
> +        return;
>      }
>  
> -    if (!qemu_opt_set(opts, arg, str + offset + 1, &local_err)) {
> -        error_report_err(local_err);
> -        return -1;
> +    if (!is_qemuopts_group(group)) {
> +        error_setg(errp, "-set is not supported with %s", group);
> +    } else {
> +        list = qemu_find_opts_err(group, errp);
> +        if (list) {

I think testing for error (like before) is more obvious than testing for
success.  Future patches extending the function could easily miss that
we didn't return in an error case and errp is already set.

With the long lines fixed, and with or without changing this one:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 3/3] vl: plug -object back into -readconfig

[Kevin Wolf]

Am 18.05.2021 um 17:40 hat Paolo Bonzini geschrieben:
> Commit bc2f4fcb1d ("qom: move user_creatable_add_opts logic to vl.c
> and QAPIfy it", 2021-03-19) switched the creation of objects from
> qemu_opts_foreach to a bespoke QTAILQ in preparation for supporting JSON
> syntax in -object.
> 
> Unfortunately in doing so it lost support for [object] stanzas in
> configuration files and also for "-set object.ID.KEY=VAL".  The latter
> is hard to re-establish and probably best solved by deprecating -set.
> This patch uses the infrastructure introduced by the previous two
> patches in order to parse QOM objects correctly from configuration
> files.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 1/3] qemu-config: parse configuration files to a QDict

[Paolo Bonzini]

On 19/05/21 16:40, Kevin Wolf wrote:
>> +    qemu_opts_absorb_qdict(opts, qdict, errp);
> Shouldn't we check that qdict is empty now and return an error if there
> are any options that the QemuOptsList doesn't accept?

Indeed, my bad for not checking exactly the contract of 
qemu_opts_absorb_qdict.

Paolo