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

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