[Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Amit Shah]

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

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

Re: [Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Amit Shah]

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

Re: [Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Amit Shah]

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

Re: [Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Amit Shah]

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

Re: [Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH] qdev: show failing device name instead of silently exiting

[Gerd Hoffmann]

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