| Message ID | 1345117273-19526-2-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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"); >
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"); >>
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
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"); > >> >
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). [...]
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). > [...] >
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). >> [...]
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.
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 > >
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.
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");
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(+)