Patchwork qdev: show failing device name instead of silently exiting

login
register
mail settings
Submitter Amit Shah
Date Sept. 23, 2009, 11:14 a.m.
Message ID <1253704471-30740-1-git-send-email-amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/34158/
State Superseded
Headers show

Comments

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

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;
     }