[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failur
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure |
Date: |
Thu, 16 Aug 2012 17:12:37 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
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).
>> [...]
- [Qemu-devel] [PATCH 0/2] Fix two buggy pc_sysfw error paths, Markus Armbruster, 2012/08/16
- [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Markus Armbruster, 2012/08/16
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Luiz Capitulino, 2012/08/16
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Markus Armbruster, 2012/08/16
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Kevin Wolf, 2012/08/16
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Luiz Capitulino, 2012/08/16
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Markus Armbruster, 2012/08/16
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Luiz Capitulino, 2012/08/16
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Luiz Capitulino, 2012/08/16
- Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Luiz Capitulino, 2012/08/16
Re: [Qemu-devel] [PATCH 1/2] pc_sysfw: Check for qemu_find_file() failure, Jordan Justen, 2012/08/16
[Qemu-devel] [PATCH 2/2] pc_sysfw: Plug memory leak on pc_fw_add_pflash_drv() error path, Markus Armbruster, 2012/08/16