Re: [Qemu-ppc] [Qemu-devel] [PATCH 1/3] PPC: Clean up misuse of qdev_init() in kvm-openpic creation

[Markus Armbruster]

Scott Wood <address@hidden> writes:

> On Wed, 2015-02-18 at 15:43 +0100, Markus Armbruster wrote:
>> Scott, can you review?
>> 
>> Markus Armbruster <address@hidden> writes:
>> 
>> > We call ppce500_init_mpic_kvm() to create a "kvm-openpic".  If it
>> > fails, we call ppce500_init_mpic_qemu() to fall back to plain
>> > "openpic".
>> >
>> > ppce500_init_mpic_kvm() uses qdev_init().  qdev_init()'s error
>> > handling has an unwanted side effect: it calls qerror_report_err(),
>> > which prints to stderr.  Looks like an error, but isn't.
>> >
>> > In QMP context, it would stash the error in the monitor instead,
>> > making the QMP command fail.  Fortunately, it's only called from board
>> > initialization, never in QMP context.
>> >
>> > Clean up by cutting out the qdev_init() middle-man: set property
>> > "realized" directly.
>> >
>> > While there, improve the error message when we can't satisfy an
>> > explicit user request for "kvm-openpic", and exit(1) instead of
>> > abort().
>
> I'm OK with this if setting the realized property directly is considered
> good practice,

I'm following precedence set by new code, and I double-checked with
Andreas to make sure it's how he wants it done.

>                but if we're not supposed to call qdev_init() in cases
> where it could legitimately fail, why is it distinct from
> qdev_init_nofail()?

Fair question!

Both qdev_init() and qdev_init_nofail() are deprecated according to
qdev-core.h:

 * As an interim step, the #DeviceState:realized property is set by deprecated
 * functions qdev_init() and qdev_init_nofail().

qdev_init() needs to go away, because it fundamentally relies on
qerror_report() to get the errors out, and that one needs to go away,
because it doesn't coexist nicely with the newer Error API.

I'm working towards getting rid of both, and this patch is part of the
effort.

I'm not working towards getting rid of qdev_init_nofail().  As long as
board code has to realize devices, having a helper that exits when
realization fails makes sense.  It may make sense to rename it once
qdev_init() is gone.