[Qemu-devel] [PATCH] qemu-option: Guard against qemu_opts_set_defaults() misuse

* [Qemu-devel] [PATCH] qemu-option: Guard against qemu_opts_set_defaults() misuse
@ 2013-08-02  7:34 Markus Armbruster
  2013-08-02 13:37 ` Eric Blake
  2013-08-14 16:28 ` Anthony Liguori
  0 siblings, 2 replies; 4+ messages in thread
From: Markus Armbruster @ 2013-08-02  7:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, aliguori

Commit 6d4cd40 fixed qemu_opts_set_defaults() for an existing corner
case, but broke it for another one that can't be reached in current
code.

Quote from its commit message:

    I believe [opts_parse()] attempts to do the following:

        If options don't yet exist, create new options
        Else, if defaults, modify the existing options
        Else, if list->merge_lists, modify the existing options
        Else, fail

The only caller that passes true for defaults is
qemu_opts_set_defaults().

The commit message then claims:

    A straightforward call of qemu_opts_create() does exactly that.

Wrong.  When !list->merge_lists, and the option string doesn't contain
id=, and options without ID exist, then we don't actually modify the
existing options, we create new ones.

Not reachable, because we never pass lists with !list->merge_lists to
qemu_opts_set_defaults().

Guard against possible (if unlikely) future misuse with assert().

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 7a1552a..4ebdc4c 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -928,6 +928,15 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         get_opt_value(value, sizeof(value), p+4);
         id = value;
     }
+
+    /*
+     * This code doesn't work for defaults && !list->merge_lists: when
+     * params has no id=, and list has an element with !opts->id, it
+     * appends a new element instead of returning the existing opts.
+     * However, we got no use for this case.  Guard against possible
+     * (if unlikely) future misuse:
+     */
+    assert(!defaults || list->merge_lists);
     opts = qemu_opts_create(list, id, !defaults, &local_err);
     if (opts == NULL) {
         if (error_is_set(&local_err)) {
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 4+ messages in thread