Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure

[Luiz Capitulino]

On Thu, 16 Aug 2012 16:32:12 +0200
Markus Armbruster <address@hidden> wrote:

> Luiz Capitulino <address@hidden> writes:
> 
> > On Thu, 16 Aug 2012 15:50:51 +0200
> > Markus Armbruster <address@hidden> wrote:
> >
> >> Luiz Capitulino <address@hidden> writes:
> >> 
> >> > On Thu, 16 Aug 2012 13:41:12 +0200
> >> > Markus Armbruster <address@hidden> 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 <address@hidden>
> >> >> ---
> >> >>  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).
> [...]
>