On Thu, 16 Aug 2012 16:32:12 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes: > > > 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. > > 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). > [...] >