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

[Stefan Hajnoczi]

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 <address@hidden>
> >>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 <address@hidden>
> >>>>Cc: Tiejun Chen <address@hidden>
> >>>>Signed-off-by: Stefan Hajnoczi <address@hidden>
> >>>>---
> >>>>  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

pgpQG7qJQmdpM.pgp
Description: PGP signature