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

[Luiz Capitulino]

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

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

A quick check shows that most calls are error reporting calls. Of course
that there lots of them, but that will happen whatever reporting function
we use.

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

This patch is fixing a function that's only used in command-line context,
I don't see why fprintf() shouldn't be a good fit. Your call about PROGNAME
is a valid one, but no function in the call chain prints it yet, so it's not
a big deal, specially if compared to the alternative (which is using
error_report()).

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

I was joking. Speaking frankly now, I feel very sorry to read that. It's a
relatively simple discussion about using fprintf() to report an error,
I don't see why it should turn out a bad thing like that.

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

In the current HMP design, lower-level functions that are used elsewhere
don't print errors directly to users. They propagate errors instead, and the
caller routes the error appropriately. Meaning that the caller could keep
propagating it up, or print it to the user, or pass it to QMP low-levels for
json parsing.

A function using error_report() will only do the "right thing" in the
command-line. Even in HMP it might get things wrong, as it may print something
unexpected. In QMP it's just plain wrong, as the error just gets ignored.

Today, error_report() is only used by legacy code that haven't been converted
to the qapi and the new error infrastructure. Converting to the new infra,
means propagating errors correctly, which error_report() does not do.

Now, pc_fw_add_pflash_drv() is completely out of all this. It's not used
in HMP context, nor in QMP context. It's used only once, in command-line
context. For this usage, fprintf() suffices and all related functions in the
call chain does just that.

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

It depends. If the function in question is used in HMP or QMP then that's
wrong, as we do have to replace error_report() usage by correct error
propagation.

Now, if the function is only used in command-line context, then maybe we
could do a simple renaming. But that does not mean we should go and use
error_report() everywhere (or worse, replace fprintf() usage by error_report()),
as that also causes pointless churn.