| Message ID | 1354092873-11954-1-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 28 Nov 2012 10:54:33 +0200 Alon Levy <alevy@redhat.com> wrote: > Instead of aborting immediately after at DEVICE_CLASS(obj) > > Signed-off-by: Alon Levy <alevy@redhat.com> > --- > 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). How do you reproduce it btw? > + > k = DEVICE_CLASS(obj); > > /* find bus */
Alon Levy <alevy@redhat.com> writes:
> Instead of aborting immediately after at DEVICE_CLASS(obj)
Reproducer?
Il 28/11/2012 12:54, Luiz Capitulino ha scritto: > On Wed, 28 Nov 2012 10:54:33 +0200 > Alon Levy <alevy@redhat.com> wrote: > >> Instead of aborting immediately after at DEVICE_CLASS(obj) >> >> Signed-off-by: Alon Levy <alevy@redhat.com> >> --- >> 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. > How do you reproduce it btw? $ x86_64-softmmu/qemu-system-x86_64 -device SCSI Object 0x7f56872248b0 is not an instance of type device Aborted However, we also need an object_class_is_abstract method to fix this similar failure: $ x86_64-softmmu/qemu-system-x86_64 -device pci-device ** ERROR:/home/pbonzini/work/upstream/qemu/qom/object.c:306:object_initialize_with_type: assertion failed: (type->abstract == false) Aborted All in all, it makes sense to me that we delay this post-1.3. Paolo >> + >> k = DEVICE_CLASS(obj); >> >> /* find bus */ >
On Wed, 28 Nov 2012 13:02:26 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 28/11/2012 12:54, Luiz Capitulino ha scritto: > > On Wed, 28 Nov 2012 10:54:33 +0200 > > Alon Levy <alevy@redhat.com> wrote: > > > >> Instead of aborting immediately after at DEVICE_CLASS(obj) > >> > >> Signed-off-by: Alon Levy <alevy@redhat.com> > >> --- > >> 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. If the patch is good, why should we wait post-1.3 giving that it fixes a real bug?
Am 28.11.2012 12:54, schrieb Luiz Capitulino: > On Wed, 28 Nov 2012 10:54:33 +0200 > Alon Levy <alevy@redhat.com> wrote: > >> Instead of aborting immediately after at DEVICE_CLASS(obj) >> >> Signed-off-by: Alon Levy <alevy@redhat.com> >> --- >> 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")) { "device" should be TYPE_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). What is definitely wrong here is the naming: "obj" is used for ObjectClass rather than Object. Therefore the use of object_class_dynamic_cast() is correct. I don't see an earlier place to error out. > > How do you reproduce it btw? My guess is that trying to instantiate a non-device type needs to be caught before using the mentioned asserting DEVICE_CLASS() macro below. I.e. something like -device x86_64-cpu (non-abstract non-device type currently) on qemu-system-x86_64, or easier -device container. Regards, Andreas > >> + >> k = DEVICE_CLASS(obj); >> >> /* find bus */ >
> On Wed, 28 Nov 2012 10:54:33 +0200 > Alon Levy <alevy@redhat.com> wrote: > > > Instead of aborting immediately after at DEVICE_CLASS(obj) > > > > Signed-off-by: Alon Levy <alevy@redhat.com> > > --- > > 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). > > How do you reproduce it btw? -device virtio-serial-bus which is the incorrect way of saying -device virtio-serial-pci > > > + > > k = DEVICE_CLASS(obj); > > > > /* find bus */ > >
> Il 28/11/2012 12:54, Luiz Capitulino ha scritto: > > On Wed, 28 Nov 2012 10:54:33 +0200 > > Alon Levy <alevy@redhat.com> wrote: > > > >> Instead of aborting immediately after at DEVICE_CLASS(obj) > >> > >> Signed-off-by: Alon Levy <alevy@redhat.com> > >> --- > >> 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. > > > How do you reproduce it btw? > > $ x86_64-softmmu/qemu-system-x86_64 -device SCSI > Object 0x7f56872248b0 is not an instance of type device > Aborted > > However, we also need an object_class_is_abstract method to fix this > similar failure: > > $ x86_64-softmmu/qemu-system-x86_64 -device pci-device > ** > ERROR:/home/pbonzini/work/upstream/qemu/qom/object.c:306:object_initialize_with_type: > assertion failed: (type->abstract == false) > Aborted > > All in all, it makes sense to me that we delay this post-1.3. I agree, I'll keep it until then. > > Paolo > > >> + > >> k = DEVICE_CLASS(obj); > >> > >> /* find bus */ > > > > >
Am 28.11.2012 13:14, schrieb Luiz Capitulino: > On Wed, 28 Nov 2012 13:02:26 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Il 28/11/2012 12:54, Luiz Capitulino ha scritto: >>> On Wed, 28 Nov 2012 10:54:33 +0200 >>> Alon Levy <alevy@redhat.com> wrote: >>> >>>> Instead of aborting immediately after at DEVICE_CLASS(obj) >>>> >>>> Signed-off-by: Alon Levy <alevy@redhat.com> >>>> --- >>>> 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
On Wed, 28 Nov 2012 13:19:36 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 28.11.2012 13:14, schrieb Luiz Capitulino: > > On Wed, 28 Nov 2012 13:02:26 +0100 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> Il 28/11/2012 12:54, Luiz Capitulino ha scritto: > >>> On Wed, 28 Nov 2012 10:54:33 +0200 > >>> Alon Levy <alevy@redhat.com> wrote: > >>> > >>>> Instead of aborting immediately after at DEVICE_CLASS(obj) > >>>> > >>>> Signed-off-by: Alon Levy <alevy@redhat.com> > >>>> --- > >>>> 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 ...; > } Yeah, was reaching this solution after some code reading :) > > /* find bus */ > ... > > Andreas >
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; + } + k = DEVICE_CLASS(obj); /* find bus */
Instead of aborting immediately after at DEVICE_CLASS(obj) Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qdev-monitor.c | 5 +++++ 1 file changed, 5 insertions(+)