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

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

[Stefan Hajnoczi]

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
-- 
2.1.0

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

[Jan Kiszka]

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.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

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

[Stefan Hajnoczi]

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.

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?

Stefan

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

[Marcel Apfelbaum]

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.
>
> 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
>

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

[Laszlo Ersek]

On 01/05/15 13: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.

I agree. That's what I thought should be done after I reported

  http://thread.gmane.org/gmane.comp.emulators.qemu/312139

and (significantly later!) attempted to look into it.

Converting all callers looked like a gargantuan task though, so I hid,
ultimately.

>> 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.

That's my fault, apologies -- Paolo posted a quick patch which I applied
and "tested" -- it made the crash go away, which was the only thing I
cared about at that point:

  http://thread.gmane.org/gmane.comp.emulators.qemu/312139/focus=312174

However, as Paolo said in the thread almost immediately after,

  http://thread.gmane.org/gmane.comp.emulators.qemu/312139/focus=312174

the -usb option itself stopped working (which I had not tested at all).

You saw my rash Tested-by, but (apparently) missed Paolo's followup.

> I think this is the right direction, I'll am going to take care of it.
> (At least I'll try...)

Thanks, and sorry about the confusion.

Laszlo

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

[Chen, Tiejun]

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
>>
>
>

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

[Shannon Zhao]

On 2015/1/6 10:37, Chen, Tiejun wrote:
> 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?
> 
Hi Tiejun,

I apply your following patch and it works. At least it doesn't crash.

Thanks,
Shannon

> ---
>  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);

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

[Chen, Tiejun]

On 2015/1/6 14:20, Shannon Zhao wrote:
> On 2015/1/6 10:37, Chen, Tiejun wrote:
>> 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?
>>
> Hi Tiejun,
>
> I apply your following patch and it works. At least it doesn't crash.
>

Thanks for your test.

Tiejun

> Thanks,
> Shannon
>
>> ---
>>   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);
>
>
>
>

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

[Marcel Apfelbaum]

On 01/06/2015 11:01 AM, Chen, Tiejun wrote:
> On 2015/1/6 14:20, Shannon Zhao wrote:
>> On 2015/1/6 10:37, Chen, Tiejun wrote:
>>> 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?
>>>
>> Hi Tiejun,
>>
>> I apply your following patch and it works. At least it doesn't crash.
>>
>
> Thanks for your test.
Hi,
I just posted a fix on the mailing list.
https://www.mail-archive.com/qemu-devel@nongnu.org/msg272607.html

Please check if it works for you,
Thanks,
Marcel

>
> Tiejun
>
>> Thanks,
>> Shannon
>>
>>> ---
>>>   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);
>>
>>
>>
>>
>

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]

On Tue, Jan 06, 2015 at 10:37:25AM +0800, Chen, Tiejun wrote:
> 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)  {

No, this is incorrect because now it just assigns the default value and
ignores the value from the command-line!

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2058 bytes --]

On 2015-01-05 12:37, Jan Kiszka 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.

Just pulled current master, and at least the iommu machine option is
still triggering an abort.

What is the status of fixing these fallouts? Was anything else addressed
by now, just this one forgotten?

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Marcel Apfelbaum]

On 01/31/2015 11:23 AM, Jan Kiszka wrote:
> On 2015-01-05 12:37, Jan Kiszka 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.
>
> Just pulled current master, and at least the iommu machine option is
> still triggering an abort.
>
> What is the status of fixing these fallouts? Was anything else addressed
> by now, just this one forgotten?
No, is not forgotten. I waited to see that the approach to fix this issue
is accepted by the reviewers.

Thanks for the reminder, patches will be submitted soon.
Marcel


>
> Jan
>
>

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

[Christian Borntraeger]

Am 02.02.2015 um 11:02 schrieb Marcel Apfelbaum:
> On 01/31/2015 11:23 AM, Jan Kiszka wrote:
>> On 2015-01-05 12:37, Jan Kiszka 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.
>>
>> Just pulled current master, and at least the iommu machine option is
>> still triggering an abort.
>>
>> What is the status of fixing these fallouts? Was anything else addressed
>> by now, just this one forgotten?
> No, is not forgotten. I waited to see that the approach to fix this issue
> is accepted by the reviewers.
> 
> Thanks for the reminder, patches will be submitted soon.
> Marcel

Anything to look at a branch or something? Adding new machine bool options
is currently pretty hard - as long as it doesnt work ;-)

Christian

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

[Marcel Apfelbaum]

On 02/04/2015 04:18 PM, Christian Borntraeger wrote:
> Am 02.02.2015 um 11:02 schrieb Marcel Apfelbaum:
>> On 01/31/2015 11:23 AM, Jan Kiszka wrote:
>>> On 2015-01-05 12:37, Jan Kiszka 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.
>>>
>>> Just pulled current master, and at least the iommu machine option is
>>> still triggering an abort.
>>>
>>> What is the status of fixing these fallouts? Was anything else addressed
>>> by now, just this one forgotten?
>> No, is not forgotten. I waited to see that the approach to fix this issue
>> is accepted by the reviewers.
>>
>> Thanks for the reminder, patches will be submitted soon.
>> Marcel
>
> Anything to look at a branch or something? Adding new machine bool options
> is currently pretty hard - as long as it doesnt work ;-)
It will be ready today or tomorrow :)

Thanks,
Marcel
>
> Christian
>