From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mqu2U-0007BG-37 for qemu-devel@nongnu.org; Thu, 24 Sep 2009 15:31:38 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mqu2P-00079M-Dg for qemu-devel@nongnu.org; Thu, 24 Sep 2009 15:31:37 -0400 Received: from [199.232.76.173] (port=44388 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mqu2P-00079E-0n for qemu-devel@nongnu.org; Thu, 24 Sep 2009 15:31:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18640) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mqu2O-0004bf-Af for qemu-devel@nongnu.org; Thu, 24 Sep 2009 15:31:32 -0400 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8OJVVcJ011970 for ; Thu, 24 Sep 2009 15:31:31 -0400 Date: Thu, 24 Sep 2009 16:31:23 -0300 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH] Fix exit on 'pci_add' Monitor command Message-ID: <20090924163123.5eec5c90@doriath> In-Reply-To: <87iqf8ciup.fsf@pike.pond.sub.org> References: <20090924111601.1d3668d2@doriath> <87iqf8ciup.fsf@pike.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On Thu, 24 Sep 2009 20:12:30 +0200 Markus Armbruster wrote: > Luiz Capitulino writes: > > > If the user issues one of the following commands to the Monitor: > > > > pci_add pci_addr=auto nic model=None > > pci_add pci_addr=auto nic model=? > > > > QEMU will exit, because the function used to perform sanity > > checks (qemu_check_nic_model_list()) exits on error. > > Yes. I meant to fix this, but you beat me to the line. > > There might be more bugs like this one. Yeah. > > This function is used by the startup code, where it makes > > sense to exit on error, but in the Monitor it doesn't. > > > > Changing qemu_check_nic_model_list() to not exit on error > > is not possible though, as it's used by the board init > > code (the PC one), where all board specific code must have > > void return. > > > > The way I've chosen to fix this was to introduce a new function > > called pci_nic_supported(), which checks if the NIC is supported > > and returns true or false accordingly. > > > > The new function is used only by the Monitor, it performs the > > necessary check and returns an error in case the NIC is not > > supported, thus qemu_check_nic_model_list()'s exit is never trigged. > > > > The following should be observed: > > > > 1. Only the specified NIC is checked, the default one is assumed > > to be supported > > > > 2. The NIC query command (model=?) won't work with pci_add, the > > right way to do this with the Monitor is to add a new command > > > > Signed-off-by: Luiz Capitulino > > It's a minimal fix, and I trust it works. But is it the proper fix? Well, given the current situation I agree it's not perfect but I think it's good enough. > It works by checking the model before calling pci_nic_init() on behalf > of monitor cmmand "pci_add ... nic ...", so that when pci_nic_init() > checks the model again, it always succeeds, and thus never exits. > > My minor complaint is that the new model check pci_nic_supported() > duplicates the existing check in qemu_check_nic_model_list(). Yes, I don't like this either.. Although qemu_check_nic_model_list() also checks for the default model, this is a bonus check. :) > My major complaint is that I'd rather see the code cleaned up there. > It's perfectly fine for code that can run only during startup to > terminate the program on configuration error. Code to be used after > startup (used from monitor, in particular) must not do that. Instead, > it should return failure up the call chain, until we reach either > startup code or monitor code, where the policy how to handle the error > resides. What cleanup do you suggest? Note that it's not only about exit(), the function also has some fprintf()s. If a big refactor is needed to properly fix this, I guess we will have to live with the bug for a long time...