qdev: show failing device name instead of silently exiting

Submitted by Amit Shah on Sept. 23, 2009, 11:14 a.m.

Details

Message ID 1253704471-30740-1-git-send-email-amit.shah@redhat.com
State Superseded
Headers show

Commit Message

Amit Shah Sept. 23, 2009, 11:14 a.m.
If initializing a device fails, show the name of the device
and then exit

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/qdev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Comments

Markus Armbruster Sept. 23, 2009, 8:53 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> If initializing a device fails, show the name of the device
> and then exit

"and then exit" is misleading, as you don't add an exit.

> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/qdev.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 1d79db0..62a6fc7 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -203,6 +203,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          return NULL;
>      }
>      if (qdev_init(qdev) != 0) {
> +        qemu_error("Error initializing device %s\n", driver);
>          qdev_free(qdev);
>          return NULL;
>      }
Amit Shah Sept. 24, 2009, 3:59 a.m.
Hey Markus,

On (Wed) Sep 23 2009 [22:53:27], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > If initializing a device fails, show the name of the device
> > and then exit
> 
> "and then exit" is misleading, as you don't add an exit.

The 'return NULL' does that -- the calling function exits.
(doesn't the subject line clarify that?)

> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/qdev.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 1d79db0..62a6fc7 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -203,6 +203,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >          return NULL;
> >      }
> >      if (qdev_init(qdev) != 0) {
> > +        qemu_error("Error initializing device %s\n", driver);
> >          qdev_free(qdev);
> >          return NULL;
> >      }

		Amit
Markus Armbruster Sept. 24, 2009, 1:30 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> Hey Markus,
>
> On (Wed) Sep 23 2009 [22:53:27], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> 
>> > If initializing a device fails, show the name of the device
>> > and then exit
>> 
>> "and then exit" is misleading, as you don't add an exit.
>
> The 'return NULL' does that -- the calling function exits.
> (doesn't the subject line clarify that?)

You could say "show the name of the device before exiting".

The message as you worded it made me look for an exit where there wasn't
one before.
Amit Shah Sept. 24, 2009, 1:31 p.m.
On (Thu) Sep 24 2009 [15:30:27], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Hey Markus,
> >
> > On (Wed) Sep 23 2009 [22:53:27], Markus Armbruster wrote:
> >> Amit Shah <amit.shah@redhat.com> writes:
> >> 
> >> > If initializing a device fails, show the name of the device
> >> > and then exit
> >> 
> >> "and then exit" is misleading, as you don't add an exit.
> >
> > The 'return NULL' does that -- the calling function exits.
> > (doesn't the subject line clarify that?)
> 
> You could say "show the name of the device before exiting".

That's what the subject says! :-)

> The message as you worded it made me look for an exit where there wasn't
> one before.

I'll respin it nevertheless.

		Amit
Markus Armbruster Sept. 24, 2009, 2:22 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> On (Thu) Sep 24 2009 [15:30:27], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> 
>> > Hey Markus,
>> >
>> > On (Wed) Sep 23 2009 [22:53:27], Markus Armbruster wrote:
>> >> Amit Shah <amit.shah@redhat.com> writes:
>> >> 
>> >> > If initializing a device fails, show the name of the device
>> >> > and then exit
>> >> 
>> >> "and then exit" is misleading, as you don't add an exit.
>> >
>> > The 'return NULL' does that -- the calling function exits.
>> > (doesn't the subject line clarify that?)
>> 
>> You could say "show the name of the device before exiting".
>
> That's what the subject says! :-)
>
>> The message as you worded it made me look for an exit where there wasn't
>> one before.
>
> I'll respin it nevertheless.
>
> 		Amit

Thanks, and hope you don't mind me being such a stickler for clear
commit messages.
Amit Shah Sept. 24, 2009, 3:36 p.m.
On (Thu) Sep 24 2009 [16:22:34], Markus Armbruster wrote:
> 
> Thanks, and hope you don't mind me being such a stickler for clear
> commit messages.

As long as you widen your net -- the commit logs aren't all too good to
read :-)

		Amit
Markus Armbruster Sept. 24, 2009, 5:50 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> On (Thu) Sep 24 2009 [16:22:34], Markus Armbruster wrote:
>> 
>> Thanks, and hope you don't mind me being such a stickler for clear
>> commit messages.
>
> As long as you widen your net -- the commit logs aren't all too good to
> read :-)

It's an all too common complaint in my reviews.  It stood out in your
case not because the message was particularly bad, only because I
couldn't find anything else to complain about :)
Gerd Hoffmann Sept. 25, 2009, 8:48 a.m.
On 09/24/09 05:59, Amit Shah wrote:
> Hey Markus,
>
> On (Wed) Sep 23 2009 [22:53:27], Markus Armbruster wrote:
>> Amit Shah<amit.shah@redhat.com>  writes:
>>
>>> If initializing a device fails, show the name of the device
>>> and then exit
>>
>> "and then exit" is misleading, as you don't add an exit.
>
> The 'return NULL' does that -- the calling function exits.
> (doesn't the subject line clarify that?)

I have patches in the queue which will wind up this via device_add 
monitor command.  Caller will *not* exit then ;)

cheers,
   Gerd

Patch hide | download patch | download mbox

diff --git a/hw/qdev.c b/hw/qdev.c
index 1d79db0..62a6fc7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -203,6 +203,7 @@  DeviceState *qdev_device_add(QemuOpts *opts)
         return NULL;
     }
     if (qdev_init(qdev) != 0) {
+        qemu_error("Error initializing device %s\n", driver);
         qdev_free(qdev);
         return NULL;
     }