Re: [Qemu-devel] [PATCH qom-next] qom: make object cast assert if NULL object is passed as argument

[Igor Mammedov]

On 06/01/2012 03:04 PM, Andreas Färber wrote:
> Am 01.06.2012 13:18, schrieb Markus Armbruster:
> > Andreas Färber<address@hidden>  writes:
> >
> > > Am 31.05.2012 13:17, schrieb Igor Mammedov:
> > > > On 05/31/2012 12:16 PM, Paolo Bonzini wrote:
> > > > > Il 31/05/2012 10:30, Markus Armbruster ha scritto:
> > > > > > > > Makes much sense, but maybe it should be done in OBJECT() cast? Assert
> > > > > > > > when we do OBJECT(NULL).
> > > > > > In my opinion, OBJECT(p) where p is a null pointer is perfectly valid
> > > > > > and should yield a null pointer.
> > > > >
> > > > > Perhaps object_dynamic_cast and object_dynamic_cast_assert should do the
> > > > > same?
> > > >
> > > > or better object_dynamic_cast should return NULL if obj is NULL,
> > > > after all it's expected that it may return NULL
> > >
> > > That's what I was suggesting: I think that we should define "NULL is not
> > > of type TYPE_FOO" and thus have the ..._is_... functions return false,
> > > and have the ..._cast_assert assert.
> >
> > Is it?
>
> See http://www.mail-archive.com/address@hidden/msg113922.html
>
> > Igor: object_dynamic_cast should return NULL if obj is NULL,
> >
> > You: have the ..._cast_assert assert [on null argument, I presume]
> >
> > Doesn't sound like the same suggestion to me :)
>
> I'll let you to your opinion. :) However, my opinion is that
> object_dynamic_cast_assert() should assert (its name should be program),
> not segfault, and that
> object_dynamic_cast()/object_is_type()/type_is_ancestor() should not
> assert but return false / NULL. So as to the effects and usability that
> pretty much aligns with Igor M., no?
If we decide that object_dynamic_cast() should not assert but rather return NULL
the this block in it will be incorrect in to places:

    if (object_is_type(obj, type_interface)) {
        assert(!obj->interfaces);  <== could be replaced with return NULL
        obj = INTERFACE(obj)->obj; <== calls OBJECT_CHECK() -> 
object_dynamic_cast_assert ()
    ...
    [snip]

maybe there should be INTERFACE_CHECK and INTERFACE macros calling ..._assert 
and non assert variants respectively?

> > If I understood you correctly: what do such assertions buy us other than
> > silliness like
> >
> >      p ? some_cast(p) : NULL
> >
> > ?
>
> Nack. The point is that currently deployed MY_TYPE(x) should assert
> (because nobody expects it to return NULL) and he who does want to
> handle NULL can use object_dynamic_cast(p). There's no real change to
> what we have except that an error case that was unhandled now is handled.
>
> > > So I still think this patch is correct. It could be accompanied by
> > > further patches adding error handling in the remaining functions.
> >
> > I'm not convinced.
>
> Shed any light?
>
> Andreas

--
-----
 Igor