From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33758) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8K1U-0002fb-UF for qemu-devel@nongnu.org; Mon, 05 Jan 2015 21:37:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y8K1R-0003bR-L8 for qemu-devel@nongnu.org; Mon, 05 Jan 2015 21:37:32 -0500 Received: from mga11.intel.com ([192.55.52.93]:38994) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8K1R-0003bN-7n for qemu-devel@nongnu.org; Mon, 05 Jan 2015 21:37:29 -0500 Message-ID: <54AB4A65.2020503@intel.com> Date: Tue, 06 Jan 2015 10:37:25 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1420456961-19631-1-git-send-email-stefanha@redhat.com> <54AA7791.9060705@siemens.com> <54AA801A.8050605@redhat.com> In-Reply-To: <54AA801A.8050605@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed 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: Marcel Apfelbaum , Stefan Hajnoczi , Jan Kiszka Cc: Peter Maydell , Marcel Apfelbaum , qemu-devel , plucinski.mariusz@gmail.com, Stefan Hajnoczi 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? --- 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 >> > >