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

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

[Andreas Färber]

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.

So I still think this patch is correct. It could be accompanied by
further patches adding error handling in the remaining functions.

Andreas

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

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

[Andreas Färber]

Am 31.05.2012 10:30, schrieb Markus Armbruster:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
> 
>> On 05/30/2012 08:19 PM, Igor Mammedov wrote:
>>> without assert it will crash at following point:
>>>     OBJECT_CHECK(type, obj, name) \
>>>      ((type *)object_dynamic_cast_assert(OBJECT(obj), (name)))
>>>        =>   object_dynamic_cast(obj, typename)
>>>          =>   object_is_type(obj, target_type)
>>>            =>   type_is_ancestor(obj->class->type, target_type);
>>>                                   ^^^
>>> so abort earlier and print nice message instead of SIGSEGV
>>>
>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>> ---
>>>   qom/object.c |    2 ++
>>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 00bb3b0..444e2fc 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -481,6 +481,8 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename)
>>>   {
>>>       Object *inst;
>>>
>>> +    g_assert(obj != NULL);
>>> +
>>>       inst = object_dynamic_cast(obj, typename);
>>>
>>>       if (!inst) {
>> 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.

Paolo, today OBJECT(p) is a pure C cast. I wonder if that was due to
TYPE_OBJECT being NULL at the time. Should we reconsider that on
qom-next with your patches to turn TYPE_OBJECT into a regular type?

Andreas

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

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

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> 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?

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 :)

If I understood you correctly: what do such assertions buy us other than
silliness like

    p ? some_cast(p) : NULL

?

> 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.

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

[Andreas Färber]

Am 01.06.2012 13:18, schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> 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/qemu-devel@nongnu.org/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 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

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

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

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 01.06.2012 13:18, schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> 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/qemu-devel@nongnu.org/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

My question isn't about a difference of opinions between us two.  It's
about Igor writing "X should do Y", and you replying "Yes, that's what I
was suggesting, X should do !Y".  There's a misunderstanding there, and
it could well be mine.  So I ask.

[...]

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<afaerber@suse.de>  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/qemu-devel@nongnu.org/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