From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mqso5-0003wT-IV for qemu-devel@nongnu.org; Thu, 24 Sep 2009 14:12:41 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mqso0-0003uv-U7 for qemu-devel@nongnu.org; Thu, 24 Sep 2009 14:12:40 -0400 Received: from [199.232.76.173] (port=41555 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mqso0-0003ur-FN for qemu-devel@nongnu.org; Thu, 24 Sep 2009 14:12:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41168) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mqsnz-00004u-EY for qemu-devel@nongnu.org; Thu, 24 Sep 2009 14:12:35 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8OICVPa010724 for ; Thu, 24 Sep 2009 14:12:31 -0400 Subject: Re: [Qemu-devel] [PATCH] Fix exit on 'pci_add' Monitor command References: <20090924111601.1d3668d2@doriath> From: Markus Armbruster Date: Thu, 24 Sep 2009 20:12:30 +0200 In-Reply-To: <20090924111601.1d3668d2@doriath> (Luiz Capitulino's message of "Thu\, 24 Sep 2009 11\:16\:01 -0300") Message-ID: <87iqf8ciup.fsf@pike.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: qemu-devel@nongnu.org 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. > 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? 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(). 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.