[Qemu-devel] [PATCH 0/2] pc_fw_add_pflash_drv() fixes

[Qemu-devel] [PATCH 0/2] pc_fw_add_pflash_drv() fixes

[Markus Armbruster]

Straightforward bug fixes, so they could qualify for 1.3.  But since
the bugs are fairly harmless, we might want to postpone to 1.4
regardless.  If we do, perhaps qemu-trivial could pick them up.

Markus Armbruster (2):
  pc_sysfw: Check for qemu_find_file() failure
  pc_sysfw: Plug memory leak on pc_fw_add_pflash_drv() error path

 hw/pc_sysfw.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
1.7.11.7

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

[Markus Armbruster]

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 <armbru@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 9d7c5f4..066c4fe 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));
+        exit(1);
+    }
 
     opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/2] pc_sysfw: Plug memory leak on pc_fw_add_pflash_drv() error path

[Markus Armbruster]

Harmless, because we the error inevitably leads to another, fatal one
in pc_system_flash_init(): PC system firmware (pflash) not available.
Fix it anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pc_sysfw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index 066c4fe..67fe87b 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
@@ -103,7 +103,9 @@ static void pc_fw_add_pflash_drv(void)
       return;
     }
 
-    drive_init(opts, machine->use_scsi);
+    if (!drive_init(opts, machine->use_scsi)) {
+        qemu_opts_del(opts);
+    }
 }
 
 static void pc_system_flash_init(MemoryRegion *rom_memory,
-- 
1.7.11.7

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

[Stefan Hajnoczi]

On Fri, Nov 23, 2012 at 07:12:17PM +0100, Markus Armbruster wrote:
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index 9d7c5f4..066c4fe 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));

qemu_find_file() does not document that errno is set when returning
NULL.  I can't find other callers to qemu_find_file() that use errno
either.

Please add a doc comment to qemu_find_file() that errno will be set on
NULL return, otherwise we can't rely on it in the caller.

Stefan

Re: [Qemu-devel] [PATCH 2/2] pc_sysfw: Plug memory leak on pc_fw_add_pflash_drv() error path

[Stefan Hajnoczi]

On Fri, Nov 23, 2012 at 07:12:18PM +0100, Markus Armbruster wrote:
> Harmless, because we the error inevitably leads to another, fatal one
> in pc_system_flash_init(): PC system firmware (pflash) not available.
> Fix it anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/pc_sysfw.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks, applied to the trivial patches tree:
https://github.com/stefanha/qemu/commits/trivial-patches

Stefan

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Fri, Nov 23, 2012 at 07:12:17PM +0100, Markus Armbruster wrote:
>> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
>> index 9d7c5f4..066c4fe 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));
>
> qemu_find_file() does not document that errno is set when returning
> NULL.  I can't find other callers to qemu_find_file() that use errno
> either.
>
> Please add a doc comment to qemu_find_file() that errno will be set on
> NULL return, otherwise we can't rely on it in the caller.

Good point, v2 coming.

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

[Markus Armbruster]

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 <armbru@redhat.com>
---
v2: Don't report errno, because that can produce misleading error
messages.  For instance, when "seabios/out/bios.bin" is unreadable, we
fall back to $data_dir/seabios/out/bios.bin, which doesn't exist, and
then report "seabios/out/bios.bin: No such file or directory".  No other
caller reports errno.

 hw/pc_sysfw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index 9d7c5f4..a161e7b 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
@@ -84,6 +84,10 @@ 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", bios_name);
+        exit(1);
+    }
 
     opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
 
-- 
1.7.11.7

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

[Stefan Hajnoczi]

On Wed, Dec 05, 2012 at 03:28:05PM +0100, Markus Armbruster 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 <armbru@redhat.com>
> ---
> v2: Don't report errno, because that can produce misleading error
> messages.  For instance, when "seabios/out/bios.bin" is unreadable, we
> fall back to $data_dir/seabios/out/bios.bin, which doesn't exist, and
> then report "seabios/out/bios.bin: No such file or directory".  No other
> caller reports errno.
> 
>  hw/pc_sysfw.c | 4 ++++
>  1 file changed, 4 insertions(+)

Thanks, applied to the trivial patches tree:
https://github.com/stefanha/qemu/commits/trivial-patches

Stefan

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

[Luiz Capitulino]

On Thu, 16 Aug 2012 13:49:15 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

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

I've talked with Markus about this on IRC and (correct me if I'm wrong Markus),
he says that we could kill the HMP stuff from error_report() once no caller
is depending on it. Looks like a plan.

I'd rather not add new error_report() calls where it's not strictly needed,
but no biggie.

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

[Jordan Justen]

Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

On Thu, Aug 16, 2012 at 4:41 AM, Markus Armbruster <armbru@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 <armbru@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));
> +        exit(1);
> +    }
>
>      opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
>
> --
> 1.7.11.2
>
>

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 <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 16 Aug 2012 16:32:12 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 16 Aug 2012 15:50:51 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Thu, 16 Aug 2012 13:41:12 +0200
> >> >> > Markus Armbruster <armbru@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 <armbru@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.

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.

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

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

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 <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 16 Aug 2012 15:50:51 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 16 Aug 2012 13:41:12 +0200
> >> > Markus Armbruster <armbru@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 <armbru@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.

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

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

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

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

[Luiz Capitulino]

On Thu, 16 Aug 2012 15:50:51 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 16 Aug 2012 13:41:12 +0200
> > Markus Armbruster <armbru@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 <armbru@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.

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

 o error_printf() prepend PROGNAME and calls fprintf()
 o die(): calls error_printf() and exit(1)

>  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).
> 
> > Also, maybe you could add the following patch to this series?
> >
> >  http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg04686.html
> 
> Can do in case I need to respin anyway.
> 
> > Although I'm not sure it qualifies for hard-freeze...
> 
> I didn't tag my series "for-1.2".  I understand that fixes to
> not-so-important stuff aren't welcome at this time even when they're
> really simple.
> 
> >> +        exit(1);
> >> +    }
> >>  
> >>      opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
> >>  
> 

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

[Kevin Wolf]

Am 16.08.2012 15:50, schrieb Markus Armbruster:
>> Although I'm not sure it qualifies for hard-freeze...
> 
> I didn't tag my series "for-1.2".  I understand that fixes to
> not-so-important stuff aren't welcome at this time even when they're
> really simple.

It's a clear bug fix, easy to understand and low risk, and even rc0
isn't out yet. I think this would still be fine for 1.2.

Kevin

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 16 Aug 2012 13:41:12 +0200
> Markus Armbruster <armbru@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 <armbru@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.  It also prepends the message with PROGNAME:, which is better
than literal "qemu:" when the executable isn't called qemu.  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).

> Also, maybe you could add the following patch to this series?
>
>  http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg04686.html

Can do in case I need to respin anyway.

> Although I'm not sure it qualifies for hard-freeze...

I didn't tag my series "for-1.2".  I understand that fixes to
not-so-important stuff aren't welcome at this time even when they're
really simple.

>> +        exit(1);
>> +    }
>>  
>>      opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
>>  

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

[Luiz Capitulino]

On Thu, 16 Aug 2012 13:41:12 +0200
Markus Armbruster <armbru@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 <armbru@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.

Also, maybe you could add the following patch to this series?

 http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg04686.html

Although I'm not sure it qualifies for hard-freeze...

> +        exit(1);
> +    }
>  
>      opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
>  

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

[Markus Armbruster]

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 <armbru@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));
+        exit(1);
+    }
 
     opts = drive_add(IF_PFLASH, -1, filename, "readonly=on");
 
-- 
1.7.11.2