Re: [Qemu-devel] [PATCH] hw/qdev-monitor: report error for -device <not-a-device-type>

[Andreas Färber]

Am 28.11.2012 13:14, schrieb Luiz Capitulino:
> On Wed, 28 Nov 2012 13:02:26 +0100
> Paolo Bonzini <address@hidden> wrote:
> 
>> Il 28/11/2012 12:54, Luiz Capitulino ha scritto:
>>> On Wed, 28 Nov 2012 10:54:33 +0200
>>> Alon Levy <address@hidden> wrote:
>>>
>>>> Instead of aborting immediately after at DEVICE_CLASS(obj)
>>>>
>>>> Signed-off-by: Alon Levy <address@hidden>
>>>> ---
>>>>  hw/qdev-monitor.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>>>> index 479eecd..3b70cdb 100644
>>>> --- a/hw/qdev-monitor.c
>>>> +++ b/hw/qdev-monitor.c
>>>> @@ -426,6 +426,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>>>          return NULL;
>>>>      }
>>>>  
>>>> +    if (!object_class_dynamic_cast(obj, "device")) {
>>>> +        qerror_report(QERR_INVALID_PARAMETER_TYPE, "driver", "device 
>>>> type");
>>>> +        return NULL;
>>>> +    }
>>>
>>> Gives me the impression that something is wrong before this, but it's
>>> better to ask a QOM guy (CC'ing them).
>>
>> I would reuse the same error message as for "if (!obj)", and also use
>> TYPE_DEVICE instead of the hardcoded string, but the patch is ok.
> 
> It's a bit weird to me that you check for a condition and right next
> you also assert it (it's what DEVICE_CLASS() will do). But I'm not familiar
> with QOM, so I'll just trust you.

We could just as well do:

k = object_class_dynamic_cast(obj, TYPE_DEVICE);
if (k == NULL) {
    // do the error reporting
    return ...;
}

/* find bus */
...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg