Luiz Capitulino <lcapitul...@redhat.com> writes: > 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.
Only if you don't mind all the non-error messages it finds, too. I converted more error messages to the error-reporting-infrastructure- du-jour than was enjoyable, and I can tell you that the restrictions that come with error_report() compared to anything-goes-fprintf() do make the job easier. >> >> 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 :) I have a better idea: stop reading qemu-devel. >> > 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. [citation needed] >> 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? You obviously don't drop error_report() when printing to monitor is no longer needed. You drop the code in error_report() that prints to the monitor. Anything else would be pointless churn. > Besides, this doesn't run in monitor context and all callers above this > function > use fprintf(). It's also a matter of consistency. Feel free to respin the patch, I don't feel possessive about it. >> > 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). >> [...]