From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55118) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8CFA-00041B-0P for qemu-devel@nongnu.org; Mon, 05 Jan 2015 13:19:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y8CF5-0007tx-Qv for qemu-devel@nongnu.org; Mon, 05 Jan 2015 13:19:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38406) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8CF5-0007tm-J1 for qemu-devel@nongnu.org; Mon, 05 Jan 2015 13:19:03 -0500 Message-ID: <54AAD58C.3000508@redhat.com> Date: Mon, 05 Jan 2015 19:18:52 +0100 From: Laszlo Ersek 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 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 Cc: Peter Maydell , Marcel Apfelbaum , Stefan Hajnoczi , qemu-devel , plucinski.mariusz@gmail.com, Stefan Hajnoczi , Jan Kiszka , Tiejun Chen 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 >> 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. 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