From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58892) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8NVr-0001UX-GE for qemu-devel@nongnu.org; Tue, 06 Jan 2015 01:21:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y8NVn-0005LL-1m for qemu-devel@nongnu.org; Tue, 06 Jan 2015 01:21:07 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:6968) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8NVm-0005K7-1V for qemu-devel@nongnu.org; Tue, 06 Jan 2015 01:21:02 -0500 Message-ID: <54AB7EB4.2070604@huawei.com> Date: Tue, 6 Jan 2015 14:20:36 +0800 From: Shannon Zhao MIME-Version: 1.0 References: <1420456961-19631-1-git-send-email-stefanha@redhat.com> <54AA7791.9060705@siemens.com> <54AA801A.8050605@redhat.com> <54AB4A65.2020503@intel.com> In-Reply-To: <54AB4A65.2020503@intel.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vl.c: fix -usb option assertion failure in qemu_opt_get_bool_helper() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Chen, Tiejun" , Marcel Apfelbaum , Stefan Hajnoczi , Jan Kiszka Cc: Peter Maydell , Hangaohuai , Marcel Apfelbaum , qemu-devel , plucinski.mariusz@gmail.com, Stefan Hajnoczi 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 >>> 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 >>>>> Cc: Tiejun Chen >>>>> Signed-off-by: Stefan Hajnoczi >>>>> --- >>>>> 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);