Markus Armbruster | 16 Aug 13:41 2012
Picon

[PATCH 0/2] Fix two buggy pc_sysfw error paths

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

Markus Armbruster | 16 Aug 13:41 2012
Picon

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

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 <at> 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
 <at>  <at>  -84,6 +84,11  <at>  <at>  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);
(Continue reading)

Luiz Capitulino | 16 Aug 15:30 2012
Picon

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

On Thu, 16 Aug 2012 13:41:12 +0200
Markus Armbruster <armbru <at> 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 <at> 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
>  <at>  <at>  -84,6 +84,11  <at>  <at>  static void pc_fw_add_pflash_drv(void)
>          bios_name = BIOS_FILENAME;
>      }
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +    if (!filename) {
(Continue reading)

Markus Armbruster | 16 Aug 15:50 2012
Picon

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

Luiz Capitulino <lcapitulino <at> redhat.com> writes:

> On Thu, 16 Aug 2012 13:41:12 +0200
> Markus Armbruster <armbru <at> 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 <at> 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
>>  <at>  <at>  -84,6 +84,11  <at>  <at>  static void pc_fw_add_pflash_drv(void)
>>          bios_name = BIOS_FILENAME;
>>      }
(Continue reading)

Kevin Wolf | 16 Aug 15:58 2012
Picon

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

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

Luiz Capitulino | 16 Aug 16:03 2012
Picon

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

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

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

Markus Armbruster | 16 Aug 16:32 2012
Picon

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

Luiz Capitulino <lcapitulino <at> redhat.com> writes:

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

Luiz Capitulino | 16 Aug 16:49 2012
Picon

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

On Thu, 16 Aug 2012 16:32:12 +0200
Markus Armbruster <armbru <at> redhat.com> wrote:

> Luiz Capitulino <lcapitulino <at> redhat.com> writes:
> 
> > On Thu, 16 Aug 2012 15:50:51 +0200
> > Markus Armbruster <armbru <at> redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino <at> redhat.com> writes:
> >> 
> >> > On Thu, 16 Aug 2012 13:41:12 +0200
> >> > Markus Armbruster <armbru <at> 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 <at> redhat.com>
> >> >> ---
> >> >>  hw/pc_sysfw.c | 5 +++++
> >> >>  1 file changed, 5 insertions(+)
(Continue reading)

Markus Armbruster | 16 Aug 17:12 2012
Picon

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

Luiz Capitulino <lcapitulino <at> redhat.com> writes:

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

Luiz Capitulino | 16 Aug 18:49 2012
Picon

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

On Thu, 16 Aug 2012 17:12:37 +0200
Markus Armbruster <armbru <at> redhat.com> wrote:

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

Luiz Capitulino | 16 Aug 19:58 2012
Picon

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

On Thu, 16 Aug 2012 13:49:15 -0300
Luiz Capitulino <lcapitulino <at> 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.

Jordan Justen | 16 Aug 19:10 2012
Picon

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

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

On Thu, Aug 16, 2012 at 4:41 AM, Markus Armbruster <armbru <at> 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 <at> 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
>  <at>  <at>  -84,6 +84,11  <at>  <at>  static void pc_fw_add_pflash_drv(void)
>          bios_name = BIOS_FILENAME;
>      }
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +    if (!filename) {
(Continue reading)

Markus Armbruster | 16 Aug 13:41 2012
Picon

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

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 <at> 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 fd22154..ebb6b25 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
 <at>  <at>  -103,7 +103,9  <at>  <at>  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.2


Gmane