Message ID | 1353686178-27520-2-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Am 23.11.2012 16:56, schrieb Paolo Bonzini: > Trying to cast a NULL value will cause a crash. Returning > NULL is also sensible, and it is also what the type-unsafe > DO_UPCAST macro does. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> I believe we had a lengthy discussion of where to place the NULL checks... seems that was never followed up with a v2 then. In practice however most of NULL assertions stem from misuses of the cast macros, i.e. using them before qdev_create() or so, which we should not hide because they lead to segfaults later. > --- > qom/object.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/qom/object.c b/qom/object.c > index d7092b0..2e18c9a 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -417,7 +417,7 @@ void object_delete(Object *obj) > > Object *object_dynamic_cast(Object *obj, const char *typename) > { > - if (object_class_dynamic_cast(object_get_class(obj), typename)) { > + if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) { > return obj; > } > This is followed by return NULL; Ack on this part. > @@ -430,7 +430,7 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename) > > inst = object_dynamic_cast(obj, typename); > > - if (!inst) { > + if (!inst && obj) { > fprintf(stderr, "Object %p is not an instance of type %s\n", > obj, typename); > abort(); This is followed by return inst; Since this function clearly has assert in the name I don't think this is right. I would expect %p to print 0x0 and the function to abort. Regards, Andreas
Il 23/11/2012 17:16, Andreas Färber ha scritto: >> > @@ -430,7 +430,7 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename) >> > >> > inst = object_dynamic_cast(obj, typename); >> > >> > - if (!inst) { >> > + if (!inst && obj) { >> > fprintf(stderr, "Object %p is not an instance of type %s\n", >> > obj, typename); >> > abort(); > This is followed by return inst; > > Since this function clearly has assert in the name I don't think this is > right. I would expect %p to print 0x0 and the function to abort. I think it's okay to segfault in this case. Otherwise you need to replace this simple cast+check pair: SCSIDevice *s = SCSI_DEVICE(some->long.expressio[n]); if (!s) { return; } with something that is more complex: DeviceState *d = some->long.expressio[n]; SCSIDevice *s; if (!d) { return; } s = SCSI_DEVICE(d); Paolo
Am 23.11.2012 17:25, schrieb Paolo Bonzini: > Il 23/11/2012 17:16, Andreas Färber ha scritto: >>>> @@ -430,7 +430,7 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename) >>>> >>>> inst = object_dynamic_cast(obj, typename); >>>> >>>> - if (!inst) { >>>> + if (!inst && obj) { >>>> fprintf(stderr, "Object %p is not an instance of type %s\n", >>>> obj, typename); >>>> abort(); >> This is followed by return inst; >> >> Since this function clearly has assert in the name I don't think this is >> right. I would expect %p to print 0x0 and the function to abort. > > I think it's okay to segfault in this case. > > Otherwise you need to replace this simple cast+check pair: > > SCSIDevice *s = SCSI_DEVICE(some->long.expressio[n]); > if (!s) { > return; > } > > with something that is more complex: > > DeviceState *d = some->long.expressio[n]; > SCSIDevice *s; > > if (!d) { > return; > } > s = SCSI_DEVICE(d); Right now our use of those macros guarantees that the result is never NULL, so there are no such checks! That expectation is broken by your patch - I'm guessing without reviewing all derived macros. If having SCSI_BUS() return NULL was your intent you would not have needed to switch to object_dynamic_cast in your second patch. :) But let's leave it for Anthony to decide how he wants his macros to work. ;) Andreas
Il 23/11/2012 17:30, Andreas Färber ha scritto: > If having SCSI_BUS() return NULL was your intent you would not have > needed to switch to object_dynamic_cast in your second patch. :) In the second patch I needed object_dynamic_cast in case the bus was not a SCSI bus at all. For example: $ qemu-system-x86_64 -S -monitor stdio -device piix3-usb-uhci (qemu) drive_add 4 if=scsi Object 0x7f2dcdcfe380 is not an instance of type SCSI Aborted Paolo
diff --git a/qom/object.c b/qom/object.c index d7092b0..2e18c9a 100644 --- a/qom/object.c +++ b/qom/object.c @@ -417,7 +417,7 @@ void object_delete(Object *obj) Object *object_dynamic_cast(Object *obj, const char *typename) { - if (object_class_dynamic_cast(object_get_class(obj), typename)) { + if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) { return obj; } @@ -430,7 +430,7 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename) inst = object_dynamic_cast(obj, typename); - if (!inst) { + if (!inst && obj) { fprintf(stderr, "Object %p is not an instance of type %s\n", obj, typename); abort();
Trying to cast a NULL value will cause a crash. Returning NULL is also sensible, and it is also what the type-unsafe DO_UPCAST macro does. Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- qom/object.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)