Re: [Qemu-devel] [PATCH 3/6] qemu-option: Add qemu_config_parse_qdict()

[Max Reitz]

On 22.11.2013 09:08, Fam Zheng wrote:
On 2013年11月22日 03:05, Max Reitz wrote:
This function basically parses command-line options given as a QDict
replacing a config file.
For instance, the QDict {"section.opt1": 42, "section.opt2": 23}
corresponds to the config file:

[section]
opt1 = 42
opt2 = 23

It is possible to specify multiple sections and also multiple sections
of the same type. On the command line, this looks like the following:

inject-error.0.event=reftable_load,\
inject-error.1.event=l2_load,\
set-state.event=l1_update

This would correspond to the following config file:

[inject-error "inject-error.0"]
event = reftable_load

[inject-error "inject-error.1"]
event = l2_load

[set-state]
event = l1_update

Signed-off-by: Max Reitz <mre...@redhat.com>
---
  include/qemu/config-file.h |   6 +++
  util/qemu-config.c         | 111 
+++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 117 insertions(+)

diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 508428f..dbd97c4 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -4,6 +4,7 @@
  #include <stdio.h>
  #include "qemu/option.h"
  #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"

  QemuOptsList *qemu_find_opts(const char *group);
  QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
@@ -18,6 +19,11 @@ int qemu_config_parse(FILE *fp, QemuOptsList 
**lists, const char *fname);

  int qemu_read_config_file(const char *filename);

+/* Parse QDict options as a replacement for a config file (allowing 
multiple
+   enumerated (0..(n-1)) configuration "sections") */
+void qemu_config_parse_qdict(QDict *options, QemuOptsList **lists,
+                             Error **errp);
+
  /* Read default QEMU config files
   */
  int qemu_read_default_config_files(bool userconfig);
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 04da942..80d82d4 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -356,3 +356,114 @@ int qemu_read_config_file(const char *filename)
          return -EINVAL;
      }
  }
+
+static void config_parse_qdict_section(QDict *options, QemuOptsList 
*opts,
+                                       Error **errp)
+{
+    QemuOpts *subopts, *subopts_i;
+    QDict *subqdict, *subqdict_i = NULL;
+    char *prefix = g_strdup_printf("%s.", opts->name);
+    Error *local_err = NULL;
+    size_t orig_size, enum_size;
+
+    qdict_extract_subqdict(options, &subqdict, prefix);
+    orig_size = qdict_size(subqdict);
+    if (!orig_size) {
+        goto out;
+    }
+
+    subopts = qemu_opts_create_nofail(opts);
+    qemu_opts_absorb_qdict(subopts, subqdict, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
+
+    enum_size = qdict_size(subqdict);
+    if (enum_size < orig_size && enum_size) {
+        error_setg(errp, "Unknown option '%s' for '%s'",
+                   qdict_first(subqdict)->key, opts->name);
+        goto out;
+    }
+
+    if (enum_size) {
+        /* Multiple, enumerated rules */
+        int i;
+
+        /* Not required anymore */
+        qemu_opts_del(subopts);
+
+        for (i = 0; i < INT_MAX; i++) {
+            char i_prefix[32], opt_name[48];
+            size_t snprintf_ret;
+
+            snprintf_ret = snprintf(i_prefix, 32, "%i.", i);
+            assert(snprintf_ret < 32);
+
+            snprintf_ret = snprintf(opt_name, 48, "%s.%i", 
opts->name, i);
+            assert(snprintf_ret < 48);

Is there a length limit of opts->name? I.e. is this an error case 
rather than an assertion case?

Hm, yes, I had most of the code directly in block/blkdebug.c at first, 
that's why there are some issues here which work fine just for blkdebug, 
but are not that good in the general case. I'll use g_strdup_sprintf 
instead.


+
+            qdict_extract_subqdict(subqdict, &subqdict_i, i_prefix);
+            if (!qdict_size(subqdict_i)) {
+                break;
+            }
+
+            subopts_i = qemu_opts_create(opts, opt_name, 1, NULL);

Could pass in errp and skip error_setg below.

Oh, right, missed that, thanks.


+            if (!subopts_i) {
+                error_setg(errp, "Option ID '%s' already in use", 
opt_name);
+                goto out;
+            }
+
+            qemu_opts_absorb_qdict(subopts_i, subqdict_i, &local_err);
+            if (error_is_set(&local_err)) {
+                error_propagate(errp, local_err);
+                qemu_opts_del(subopts_i);
+                goto out;
+            }
+
+            if (qdict_size(subqdict_i)) {
+                error_setg(errp, "[%s] rules don't support the 
option '%s'",

s/rules don't/section doesn't/

Yes, this one also is fine for blkdebug, but not so much for the general 
case.


+                           opts->name, qdict_first(subqdict_i)->key);
+                qemu_opts_del(subopts_i);
+                goto out;
+            }
+
+            /*
+            if (add_rule(subopts_i, (void *)ard) < 0) {
+                error_setg(errp, "Could not add [%s] rule %i", 
opts->name, i);

s/rule/section/
Or just remove this hunk?

Hm, I probably should have completely reviewed my own code before 
sending it. Will do that in v2, promise. ;-)


+                goto out;
+            }
+            */
+
+            QDECREF(subqdict_i);
+            subqdict_i = NULL;
+        }
+
+        if (qdict_size(subqdict)) {
+            error_setg(errp, "Unused option '%s' for [%s]",
+                       qdict_first(subqdict)->key, opts->name);
+            goto out;
+        }
+    }
+
+out:
+    QDECREF(subqdict);
+    QDECREF(subqdict_i);
+
+    free(prefix);
+}
+
+void qemu_config_parse_qdict(QDict *options, QemuOptsList **lists,
+                             Error **errp)
+{
+    int i;
+    Error *local_err = NULL;
+
+    for (i = 0; lists[i]; i++) {
+        config_parse_qdict_section(options, lists[i], &local_err);
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+}


This does the work, but also seems ad-hoc. Providing an array of dicts 
in the command line should be a general logic, which would also be 
very useful for Quorum driver.

Yes, I thought about quorum, too. I don't think just using QemuOpts 
there would be too bad, since the above function could do all the work 
there as well. I could probably introduce a QDict function which splits 
a QDict with keys enumerated in this way into a QList of QDicts. I'll 
have a look into how much more complex that will be and, depending on 
that, include it in v2 or not.

Thanks for reviewing,

Max