qemu_opts_set is used to create default network backends and to
parse sugar options -kernel, -initrd, -append, -bios and -dtb.
Switch the former to qemu_opts_parse, so that qemu_opts_set
is now only used on merge-lists QemuOptsList (for which it makes
the most sense indeed)... except in the testcase, which is
changed to use a merge-list QemuOptsList.

With this change we can remove the id parameter.  With the
parameter always NULL, we know that qemu_opts_create cannot fail
and can pass &error_abort to it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/option.h  |  3 +--
 softmmu/vl.c           | 19 +++++++------------
 tests/test-qemu-opts.c | 24 +++++++++++++++++++++---
 util/qemu-option.c     |  9 +++------
 4 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index ac69352e0e..f73e0dc7d9 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -119,8 +119,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
                            int fail_if_exists, Error **errp);
 void qemu_opts_reset(QemuOptsList *list);
 void qemu_opts_loc_restore(QemuOpts *opts);
-bool qemu_opts_set(QemuOptsList *list, const char *id,
-                   const char *name, const char *value, Error **errp);
+bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp);
 const char *qemu_opts_id(QemuOpts *opts);
 void qemu_opts_set_id(QemuOpts *opts, char *id);
 void qemu_opts_del(QemuOpts *opts);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index a71164494e..65607fe55e 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3107,20 +3107,16 @@ void qemu_init(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_kernel:
-                qemu_opts_set(qemu_find_opts("machine"), NULL, "kernel", optarg,
-                              &error_abort);
+                qemu_opts_set(qemu_find_opts("machine"), "kernel", optarg, &error_abort);
                 break;
             case QEMU_OPTION_initrd:
-                qemu_opts_set(qemu_find_opts("machine"), NULL, "initrd", optarg,
-                              &error_abort);
+                qemu_opts_set(qemu_find_opts("machine"), "initrd", optarg, &error_abort);
                 break;
             case QEMU_OPTION_append:
-                qemu_opts_set(qemu_find_opts("machine"), NULL, "append", optarg,
-                              &error_abort);
+                qemu_opts_set(qemu_find_opts("machine"), "append", optarg, &error_abort);
                 break;
             case QEMU_OPTION_dtb:
-                qemu_opts_set(qemu_find_opts("machine"), NULL, "dtb", optarg,
-                              &error_abort);
+                qemu_opts_set(qemu_find_opts("machine"), "dtb", optarg, &error_abort);
                 break;
             case QEMU_OPTION_cdrom:
                 drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
@@ -3230,8 +3226,7 @@ void qemu_init(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_bios:
-                qemu_opts_set(qemu_find_opts("machine"), NULL, "firmware", optarg,
-                              &error_abort);
+                qemu_opts_set(qemu_find_opts("machine"), "firmware", optarg, &error_abort);
                 break;
             case QEMU_OPTION_singlestep:
                 singlestep = 1;
@@ -4258,9 +4253,9 @@ void qemu_init(int argc, char **argv, char **envp)
 
     if (default_net) {
         QemuOptsList *net = qemu_find_opts("net");
-        qemu_opts_set(net, NULL, "type", "nic", &error_abort);
+        qemu_opts_parse(net, "nic", true, &error_abort);
 #ifdef CONFIG_SLIRP
-        qemu_opts_set(net, NULL, "type", "user", &error_abort);
+        qemu_opts_parse(net, "user", true, &error_abort);
 #endif
     }
 
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 297ffe79dd..322b32871b 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -84,11 +84,25 @@ static QemuOptsList opts_list_03 = {
     },
 };
 
+static QemuOptsList opts_list_04 = {
+    .name = "opts_list_04",
+    .head = QTAILQ_HEAD_INITIALIZER(opts_list_04.head),
+    .merge_lists = true,
+    .desc = {
+        {
+            .name = "str3",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
 static void register_opts(void)
 {
     qemu_add_opts(&opts_list_01);
     qemu_add_opts(&opts_list_02);
     qemu_add_opts(&opts_list_03);
+    qemu_add_opts(&opts_list_04);
 }
 
 static void test_find_unknown_opts(void)
@@ -402,17 +416,21 @@ static void test_qemu_opts_set(void)
     QemuOpts *opts;
     const char *opt;
 
-    list = qemu_find_opts("opts_list_01");
+    list = qemu_find_opts("opts_list_04");
     g_assert(list != NULL);
     g_assert(QTAILQ_EMPTY(&list->head));
-    g_assert_cmpstr(list->name, ==, "opts_list_01");
+    g_assert_cmpstr(list->name, ==, "opts_list_04");
 
     /* should not find anything at this point */
     opts = qemu_opts_find(list, NULL);
     g_assert(opts == NULL);
 
     /* implicitly create opts and set str3 value */
-    qemu_opts_set(list, NULL, "str3", "value", &error_abort);
+    qemu_opts_set(list, "str3", "first_value", &error_abort);
+    g_assert(!QTAILQ_EMPTY(&list->head));
+
+    /* set it again */
+    qemu_opts_set(list, "str3", "value", &error_abort);
     g_assert(!QTAILQ_EMPTY(&list->head));
 
     /* get the just created opts */
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 59be4f9d21..c88e159f18 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -665,15 +665,12 @@ void qemu_opts_loc_restore(QemuOpts *opts)
     loc_restore(&opts->loc);
 }
 
-bool qemu_opts_set(QemuOptsList *list, const char *id,
-                   const char *name, const char *value, Error **errp)
+bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp)
 {
     QemuOpts *opts;
 
-    opts = qemu_opts_create(list, id, 1, errp);
-    if (!opts) {
-        return false;
-    }
+    assert(list->merge_lists);
+    opts = qemu_opts_create(list, NULL, 0, &error_abort);
     return qemu_opt_set(opts, name, value, errp);
 }
 
-- 
2.26.2

Paolo Bonzini <pbonzini@redhat.com> writes:

> qemu_opts_set is used to create default network backends and to
> parse sugar options -kernel, -initrd, -append, -bios and -dtb.
> Switch the former to qemu_opts_parse, so that qemu_opts_set
> is now only used on merge-lists QemuOptsList (for which it makes
> the most sense indeed)... except in the testcase, which is
> changed to use a merge-list QemuOptsList.
>
> With this change we can remove the id parameter.  With the
> parameter always NULL, we know that qemu_opts_create cannot fail
> and can pass &error_abort to it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/option.h  |  3 +--
>  softmmu/vl.c           | 19 +++++++------------
>  tests/test-qemu-opts.c | 24 +++++++++++++++++++++---
>  util/qemu-option.c     |  9 +++------
>  4 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index ac69352e0e..f73e0dc7d9 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -119,8 +119,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>                             int fail_if_exists, Error **errp);
>  void qemu_opts_reset(QemuOptsList *list);
>  void qemu_opts_loc_restore(QemuOpts *opts);
> -bool qemu_opts_set(QemuOptsList *list, const char *id,
> -                   const char *name, const char *value, Error **errp);
> +bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp);

Long line.  Please break before Error.

>  const char *qemu_opts_id(QemuOpts *opts);
>  void qemu_opts_set_id(QemuOpts *opts, char *id);
>  void qemu_opts_del(QemuOpts *opts);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index a71164494e..65607fe55e 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3107,20 +3107,16 @@ void qemu_init(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_kernel:
> -                qemu_opts_set(qemu_find_opts("machine"), NULL, "kernel", optarg,
> -                              &error_abort);
> +                qemu_opts_set(qemu_find_opts("machine"), "kernel", optarg, &error_abort);
>                  break;
>              case QEMU_OPTION_initrd:
> -                qemu_opts_set(qemu_find_opts("machine"), NULL, "initrd", optarg,
> -                              &error_abort);
> +                qemu_opts_set(qemu_find_opts("machine"), "initrd", optarg, &error_abort);
>                  break;
>              case QEMU_OPTION_append:
> -                qemu_opts_set(qemu_find_opts("machine"), NULL, "append", optarg,
> -                              &error_abort);
> +                qemu_opts_set(qemu_find_opts("machine"), "append", optarg, &error_abort);
>                  break;
>              case QEMU_OPTION_dtb:
> -                qemu_opts_set(qemu_find_opts("machine"), NULL, "dtb", optarg,
> -                              &error_abort);
> +                qemu_opts_set(qemu_find_opts("machine"), "dtb", optarg, &error_abort);
>                  break;
>              case QEMU_OPTION_cdrom:
>                  drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
> @@ -3230,8 +3226,7 @@ void qemu_init(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_bios:
> -                qemu_opts_set(qemu_find_opts("machine"), NULL, "firmware", optarg,
> -                              &error_abort);
> +                qemu_opts_set(qemu_find_opts("machine"), "firmware", optarg, &error_abort);
>                  break;
>              case QEMU_OPTION_singlestep:
>                  singlestep = 1;

Long lines.  Please keep the line breaks.

> @@ -4258,9 +4253,9 @@ void qemu_init(int argc, char **argv, char **envp)
>  
>      if (default_net) {
>          QemuOptsList *net = qemu_find_opts("net");
> -        qemu_opts_set(net, NULL, "type", "nic", &error_abort);
> +        qemu_opts_parse(net, "nic", true, &error_abort);
>  #ifdef CONFIG_SLIRP
> -        qemu_opts_set(net, NULL, "type", "user", &error_abort);
> +        qemu_opts_parse(net, "user", true, &error_abort);
>  #endif
>      }
>  

Looks safe to me, but I don't quite get why you switch to
qemu_opts_parse().  The commit message explains it is "so that
qemu_opts_set is now only used on merge-lists QemuOptsList (for which it
makes the most sense indeed)..."  Is there anything wrong with using ot
on non-merge-lists QemuOptsList?

Am I missing something?

> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index 297ffe79dd..322b32871b 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -84,11 +84,25 @@ static QemuOptsList opts_list_03 = {
>      },
>  };
>  
> +static QemuOptsList opts_list_04 = {
> +    .name = "opts_list_04",
> +    .head = QTAILQ_HEAD_INITIALIZER(opts_list_04.head),
> +    .merge_lists = true,
> +    .desc = {
> +        {
> +            .name = "str3",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  static void register_opts(void)
>  {
>      qemu_add_opts(&opts_list_01);
>      qemu_add_opts(&opts_list_02);
>      qemu_add_opts(&opts_list_03);
> +    qemu_add_opts(&opts_list_04);
>  }
>  
>  static void test_find_unknown_opts(void)
> @@ -402,17 +416,21 @@ static void test_qemu_opts_set(void)
>      QemuOpts *opts;
>      const char *opt;
>  
> -    list = qemu_find_opts("opts_list_01");
> +    list = qemu_find_opts("opts_list_04");
>      g_assert(list != NULL);
>      g_assert(QTAILQ_EMPTY(&list->head));
> -    g_assert_cmpstr(list->name, ==, "opts_list_01");
> +    g_assert_cmpstr(list->name, ==, "opts_list_04");
>  
>      /* should not find anything at this point */
>      opts = qemu_opts_find(list, NULL);
>      g_assert(opts == NULL);
>  
>      /* implicitly create opts and set str3 value */
> -    qemu_opts_set(list, NULL, "str3", "value", &error_abort);
> +    qemu_opts_set(list, "str3", "first_value", &error_abort);


This part the commit message mentions.

> +    g_assert(!QTAILQ_EMPTY(&list->head));
> +
> +    /* set it again */
> +    qemu_opts_set(list, "str3", "value", &error_abort);
>      g_assert(!QTAILQ_EMPTY(&list->head));

This one not.

What are you trying to accomplish?

>  
>      /* get the just created opts */
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 59be4f9d21..c88e159f18 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -665,15 +665,12 @@ void qemu_opts_loc_restore(QemuOpts *opts)
>      loc_restore(&opts->loc);
>  }
>  
> -bool qemu_opts_set(QemuOptsList *list, const char *id,
> -                   const char *name, const char *value, Error **errp)
> +bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp)

Long line.  Please break before Error.

>  {
>      QemuOpts *opts;
>  
> -    opts = qemu_opts_create(list, id, 1, errp);
> -    if (!opts) {
> -        return false;
> -    }
> +    assert(list->merge_lists);
> +    opts = qemu_opts_create(list, NULL, 0, &error_abort);
>      return qemu_opt_set(opts, name, value, errp);
>  }

Yes, qemu_opts_create() can fail only if its second paramter is
non-null, or its thirs paramter is non-zero.

I can see quite a few such calls.  Could be simplified with a wrapper
that takes just the first parameter and can't fail.  Not now.

Just enough confusion on my part to withhold my R-by for now.

On 09/11/20 16:55, Markus Armbruster wrote:
>>           QemuOptsList *net = qemu_find_opts("net");
>> -        qemu_opts_set(net, NULL, "type", "nic", &error_abort);
>> +        qemu_opts_parse(net, "nic", true, &error_abort);
>>   #ifdef CONFIG_SLIRP
>> -        qemu_opts_set(net, NULL, "type", "user", &error_abort);
>> +        qemu_opts_parse(net, "user", true, &error_abort);
>>   #endif
>>       }
>>   
> Looks safe to me, but I don't quite get why you switch to
> qemu_opts_parse().  The commit message explains it is "so that
> qemu_opts_set is now only used on merge-lists QemuOptsList (for which it
> makes the most sense indeed)..."  Is there anything wrong with using ot
> on non-merge-lists QemuOptsList?

I would *expect* a function named qemu_opts_set to do two things:

1. setting an option in a merge-lists QemuOptsList, such as -kernel.

This is indeed what we mostly use qemu_opts_set for.


2. setting an option in a non-merge-lists QemuOptsList with non-NULL id, 
similar to -set.

QEMU does not use qemu_opts_set for the latter (see qemu_set_option) 
because it wants to use qemu_opts_find rather than qemu_opts_create.  In 
fact it wouldn't *work* to use qemu_opts_set for the latter because 
qemu_opts_set uses fail_if_exists==1. So:

    -> For non-merge-lists QemuOptsList and non-NULL id, it is
       debatable that qemu_opts_set fails if the (QemuOptsList, id)
       pair already exists


On the other hand, I would not *expect* qemu_opts_set to create a 
non-merge-lists QemuOpts with a single option; which it does, though. 
This leads us directly to:

    -> For non-merge-lists QemuOptsList and NULL id, qemu_opts_set
       hardly adds value over qemu_opts_parse.  It does skip some
       parsing and unescaping, but its two call sites don't really care.

So qemu_opts_set has warty behavior for non-merge-lists QemuOptsList if 
id is non-NULL, and it's mostly pointless if id is NULL.  My solution to 
keeping the API as simple as possible is to limit qemu_opts_set to 
merge-lists QemuOptsList.  For them, it's useful (we don't want 
comma-unescaping for -kernel) *and* has sane semantics.

>> +    g_assert(!QTAILQ_EMPTY(&list->head));
>> +
>> +    /* set it again */
>> +    qemu_opts_set(list, "str3", "value", &error_abort);
>>      g_assert(!QTAILQ_EMPTY(&list->head));
> 
> This one not.
> 
> What are you trying to accomplish?

Improve the testcase, though I should have mentioned it in the commit 
message.  Basically emulating "-kernel bc -kernel def".

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 09/11/20 16:55, Markus Armbruster wrote:
>>>           QemuOptsList *net = qemu_find_opts("net");
>>> -        qemu_opts_set(net, NULL, "type", "nic", &error_abort);
>>> +        qemu_opts_parse(net, "nic", true, &error_abort);
>>>   #ifdef CONFIG_SLIRP
>>> -        qemu_opts_set(net, NULL, "type", "user", &error_abort);
>>> +        qemu_opts_parse(net, "user", true, &error_abort);
>>>   #endif
>>>       }
>>>   
>> Looks safe to me, but I don't quite get why you switch to
>> qemu_opts_parse().  The commit message explains it is "so that
>> qemu_opts_set is now only used on merge-lists QemuOptsList (for which it
>> makes the most sense indeed)..."  Is there anything wrong with using ot
>> on non-merge-lists QemuOptsList?
>
> I would *expect* a function named qemu_opts_set to do two things:
>
> 1. setting an option in a merge-lists QemuOptsList, such as -kernel.
>
> This is indeed what we mostly use qemu_opts_set for.
>
>
> 2. setting an option in a non-merge-lists QemuOptsList with non-NULL
> id, similar to -set.
>
> QEMU does not use qemu_opts_set for the latter (see qemu_set_option)
> because it wants to use qemu_opts_find rather than qemu_opts_create.
> In fact it wouldn't *work* to use qemu_opts_set for the latter because 
> qemu_opts_set uses fail_if_exists==1. So:
>
>    -> For non-merge-lists QemuOptsList and non-NULL id, it is
>       debatable that qemu_opts_set fails if the (QemuOptsList, id)
>       pair already exists
>
>
> On the other hand, I would not *expect* qemu_opts_set to create a
> non-merge-lists QemuOpts with a single option; which it does, though. 
> This leads us directly to:
>
>    -> For non-merge-lists QemuOptsList and NULL id, qemu_opts_set
>       hardly adds value over qemu_opts_parse.  It does skip some
>       parsing and unescaping, but its two call sites don't really care.
>
> So qemu_opts_set has warty behavior for non-merge-lists QemuOptsList
> if id is non-NULL, and it's mostly pointless if id is NULL.  My
> solution to keeping the API as simple as possible is to limit
> qemu_opts_set to merge-lists QemuOptsList.  For them, it's useful (we
> don't want comma-unescaping for -kernel) *and* has sane semantics.

Okay, makes sense.

Do you think working (some of) this into commit message would be worth
your while?

>>> +    g_assert(!QTAILQ_EMPTY(&list->head));
>>> +
>>> +    /* set it again */
>>> +    qemu_opts_set(list, "str3", "value", &error_abort);
>>>      g_assert(!QTAILQ_EMPTY(&list->head));
>> This one not.
>> What are you trying to accomplish?
>
> Improve the testcase, though I should have mentioned it in the commit
> message.  Basically emulating "-kernel bc -kernel def".

Worth testing.  But the case "-kernel bc" is also worth testing.
test_qemu_opts_get() tests both:

    /* haven't set anything to str2 yet */
    opt = qemu_opt_get(opts, "str2");
    g_assert(opt == NULL);

    qemu_opt_set(opts, "str2", "value", &error_abort);

    /* now we have set str2, should know about it */
    opt = qemu_opt_get(opts, "str2");
    g_assert_cmpstr(opt, ==, "value");

    qemu_opt_set(opts, "str2", "value2", &error_abort);

    /* having reset the value, the returned should be the reset one */
    opt = qemu_opt_get(opts, "str2");
    g_assert_cmpstr(opt, ==, "value2");

I'm okay with not improving the test in this patch, or with strictly
extending coverage, preferably in a separate patch that goes before this
one.

On 09/11/20 19:52, Markus Armbruster wrote:
> Do you think working (some of) this into commit message would be worth
> your while?

Easy and does not require a respin, so yes.

>> Improve the testcase, though I should have mentioned it in the commit
>> message.  Basically emulating "-kernel bc -kernel def".
> Worth testing.  But the case "-kernel bc" is also worth testing.
> test_qemu_opts_get() tests both:
> 
>      /* haven't set anything to str2 yet */
>      opt = qemu_opt_get(opts, "str2");
>      g_assert(opt == NULL);
> 
>      qemu_opt_set(opts, "str2", "value", &error_abort);
> 
>      /* now we have set str2, should know about it */
>      opt = qemu_opt_get(opts, "str2");
>      g_assert_cmpstr(opt, ==, "value");
> 
>      qemu_opt_set(opts, "str2", "value2", &error_abort);
> 
>      /* having reset the value, the returned should be the reset one */
>      opt = qemu_opt_get(opts, "str2");
>      g_assert_cmpstr(opt, ==, "value2");

Note opt_set vs. opts_set though.

> I'm okay with not improving the test in this patch, or with strictly
> extending coverage, preferably in a separate patch that goes before this
> one.
> 

Ok, I'll just drop the testcase change for now.

Paolo