Re: [Qemu-devel] [PATCH] vl.c: fix -usb option assertion failure in qemu_opt_get_bool_helper()

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/1/5 20:14, Marcel Apfelbaum wrote:
> On 01/05/2015 01:50 PM, Stefan Hajnoczi wrote:
>> On Mon, Jan 5, 2015 at 11:37 AM, Jan Kiszka <jan.kiszka@siemens.com>
>> wrote:
>>> On 2015-01-05 12:22, Stefan Hajnoczi wrote:
>>>> Commit 49d2e648e8087d154d8bf8b91f27c8e05e79d5a6 ("machine: remove
>>>> qemu_machine_opts global list") removed option descriptions from the
>>>> -machine QemuOptsList to avoid repeating MachineState's QOM properties.
>>>>
>>>> This change broke vl.c:usb_enabled() because qemu_opt_get_bool() cannot
>>>> be used on QemuOptsList without option descriptions since QemuOpts
>>>> doesn't know the type and therefore left an unparsed string value.
>>>>
>>>> This patch avoids calling qemu_opt_get_bool() to fix the assertion
>>>> failure:
>>>>
>>>>    $ qemu-system-x86_64 -usb
>>>>    qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type
>>>> == QEMU_OPT_BOOL' failed.
>>>>
>>>> Test the presence of -usb using qemu_opt_find() but use the
>>>> MachineState->usb field instead of qemu_opt_get_bool().
>>>>
>>>> Cc: Marcel Apfelbaum <marcel.a@redhat.com>
>>>> Cc: Tiejun Chen <tiejun.chen@intel.com>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>   vl.c | 7 +++++--
>>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/vl.c b/vl.c
>>>> index bea9656..6e8889c 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -999,8 +999,11 @@ static int parse_name(QemuOpts *opts, void
>>>> *opaque)
>>>>
>>>>   bool usb_enabled(bool default_usb)
>>>>   {
>>>> -    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>>>> -                             has_defaults && default_usb);
>>>> +    if (qemu_opt_find(qemu_get_machine_opts(), "usb")) {
>>>> +        return current_machine->usb;
>>>> +    } else {
>>>> +        return has_defaults && default_usb;
>>>> +    }
>>>>   }
>>>>
>>>>   #ifndef _WIN32
>>>>
>>>
>>> That still leaves the other boolean machine options broken. A generic
>>> fix would be good. Or revert the original commit until we have one.
>>
>> I think we should revert the original commit.
>>
>> qemu_option_get_*() callers need to be converted to query MachineState
>> fields instead of using QemuOpts functions.

Can this work out currently?

---
  util/qemu-option.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index a708241..933b885 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -317,7 +317,7 @@ const char *qemu_opt_get(QemuOpts *opts, const char 
*name)
      }

      opt = qemu_opt_find(opts, name);
-    if (!opt) {
+    if (!opt || !opt->desc)  {
          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, 
name);
          if (desc && desc->def_value_str) {
              return desc->def_value_str;
@@ -341,7 +341,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
      }

      opt = qemu_opt_find(opts, name);
-    if (!opt) {
+    if (!opt || !opt->desc)  {
          desc = find_desc_by_name(opts->list->desc, name);
          if (desc && desc->def_value_str) {
              str = g_strdup(desc->def_value_str);
@@ -377,7 +377,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, 
const char *name,
      }

      opt = qemu_opt_find(opts, name);
-    if (opt == NULL) {
+    if (!opt || !opt->desc)  {
          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, 
name);
          if (desc && desc->def_value_str) {
              parse_option_bool(name, desc->def_value_str, &ret, 
&error_abort);
@@ -413,7 +413,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts 
*opts, const char *name,
      }

      opt = qemu_opt_find(opts, name);
-    if (opt == NULL) {
+    if (!opt || !opt->desc)  {
          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, 
name);
          if (desc && desc->def_value_str) {
              parse_option_number(name, desc->def_value_str, &ret, 
&error_abort);
@@ -450,7 +450,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts 
*opts, const char *name,
      }

      opt = qemu_opt_find(opts, name);
-    if (opt == NULL) {
+    if (!opt || !opt->desc)  {
          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, 
name);
          if (desc && desc->def_value_str) {
              parse_option_size(name, desc->def_value_str, &ret, 
&error_abort);
-- 

Thanks
Tiejun

>>
>> This means we need to modify the way default values are processed.
>> MachineState fields should start with the default value and be
>> overridden by command-line options.  Right now it works differently:
>> we query the command-line option and fall back to the default if the
>> option wasn't set.  That approach doesn't work for MachineState fields
>> since C types (bool, int, etc) aren't tristate in the general case, so
>> they cannot indicate whether or not they were set.
>>
>> The benefit of doing this work is that we eliminate the QemuOpts layer
>> and work directly with QOM properties instead.
>>
>> Marcel: Do you want to do this or do you have another fix in mind?
> Hi, sorry for not getting to this earlier, I thought Paolo already fixed
> it, but I was wrong.
>
> I think this is the right direction, I'll am going to take care of it.
> (At least I'll try...)
>
> Thanks,
> Marcel
>>
>> Stefan
>>
>
>