On Thu, 16 Aug 2012 15:50:51 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes: > > > On Thu, 16 Aug 2012 13:41:12 +0200 > > Markus Armbruster <arm...@redhat.com> 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 <arm...@redhat.com> > >> --- > >> 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. > 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: o error_printf() prepend PROGNAME and calls fprintf() o die(): calls error_printf() and exit(1) > 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). > > > Also, maybe you could add the following patch to this series? > > > > http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg04686.html > > Can do in case I need to respin anyway. > > > Although I'm not sure it qualifies for hard-freeze... > > I didn't tag my series "for-1.2". I understand that fixes to > not-so-important stuff aren't welcome at this time even when they're > really simple. > > >> + exit(1); > >> + } > >> > >> opts = drive_add(IF_PFLASH, -1, filename, "readonly=on"); > >> >