From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38495) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T21Nb-0003M7-70 for qemu-devel@nongnu.org; Thu, 16 Aug 2012 10:49:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T21NQ-0006hH-3S for qemu-devel@nongnu.org; Thu, 16 Aug 2012 10:48:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35699) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T21NP-0006hC-Ri for qemu-devel@nongnu.org; Thu, 16 Aug 2012 10:48:48 -0400 Date: Thu, 16 Aug 2012 11:49:27 -0300 From: Luiz Capitulino Message-ID: <20120816114927.6f2951de@doriath.home> In-Reply-To: <874no2c3gz.fsf@blackfin.pond.sub.org> References: <1345117273-19526-1-git-send-email-armbru@redhat.com> <1345117273-19526-2-git-send-email-armbru@redhat.com> <20120816103057.675f433b@doriath.home> <87txw3djyc.fsf@blackfin.pond.sub.org> <20120816110359.7a88e552@doriath.home> <874no2c3gz.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: jordan.l.justen@intel.com, qemu-devel@nongnu.org, anthony@codemonkey.ws On Thu, 16 Aug 2012 16:32:12 +0200 Markus Armbruster wrote: > Luiz Capitulino writes: > > > On Thu, 16 Aug 2012 15:50:51 +0200 > > Markus Armbruster wrote: > > > >> Luiz Capitulino writes: > >> > >> > On Thu, 16 Aug 2012 13:41:12 +0200 > >> > Markus Armbruster wrote: > >> > > >> >> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily > >> >> creates a drive without a medium. > >> >> > >> >> When pc_system_flash_init() asks for its size, bdrv_getlength() fails > >> >> with -ENOMEDIUM, which isn't checked either. It fails relatively > >> >> cleanly only because -ENOMEDIUM isn't a multiple of 4096: > >> >> > >> >> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant > >> >> qemu: PC system firmware (pflash) must be a multiple of 0x1000 > >> >> [Exit 1 ] > >> >> > >> >> Fix by handling the qemu_find_file() failure. > >> >> > >> >> Signed-off-by: Markus Armbruster > >> >> --- > >> >> hw/pc_sysfw.c | 5 +++++ > >> >> 1 file changed, 5 insertions(+) > >> >> > >> >> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c > >> >> index b45f0ac..fd22154 100644 > >> >> --- a/hw/pc_sysfw.c > >> >> +++ b/hw/pc_sysfw.c > >> >> @@ -84,6 +84,11 @@ static void pc_fw_add_pflash_drv(void) > >> >> bios_name = BIOS_FILENAME; > >> >> } > >> >> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > >> >> + if (!filename) { > >> >> + error_report("Can't open BIOS image %s: %s", > >> >> + bios_name, strerror(errno)); > >> > > >> > Why not use plain fprintf()? This is called from machine init time, I > >> > don't think this is ever called in monitor context. > >> > >> error_report() makes the fact that's an error message obvious and > >> greppable. > > > > fprintf(stderr, ...) too. > > Nope. We print other things to stderr, too, not just errors. > error_report() is always an error, and always formatted the right way, > as a single line. It's still greppable. > >> It also prepends the message with PROGNAME:, which is better > >> than literal "qemu:" when the executable isn't called qemu. > > > > We can't spread error_report() usage just because of that. I mean, we're not > > going to replace every usage of fprintf(stderr, ...) with error_report() just > > because of that, right? > > > > For this fix, I suggest calling fprintf() plus abort(), which is what is > > done by the caller and several functions in the call chain. > > > > For the long term, I suggest adding: > > In the long term, we're all dead. Let's stop coding then :) > > o error_printf() prepend PROGNAME and calls fprintf() > > Rename error_report() to error_printf() and you're done. It's not a matter of naming. error_report() doesn't fit in the picture today where random code doesn't print to the monitor. It's really deprecated. > It even does > the right thing in human monitor code. Only from a legacy perspective. > Most of the human monitor code > runs silently on top of QMP nowadays, so the right thing isn't needed > there. It can easily be dropped as soon as no other human monitor code > exists anymore. That's my point, why are we going to add a function just to drop it afterwards? Besides, this doesn't run in monitor context and all callers above this function use fprintf(). It's also a matter of consistency. > > > o die(): calls error_printf() and exit(1) > > When your infrastructure is committed, and the old one is gone, I'll use > it. > > >> It would > >> even point to -bios nicely if we bothered to preserve that information > >> (we lose it by storing the option argument as naked char * without the > >> location). > [...] >