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

[Markus Armbruster]

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 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.

>  o error_printf() prepend PROGNAME and calls fprintf()

Rename error_report() to error_printf() and you're done.  It even does
the right thing in human monitor code.  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.

>  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).
[...]