Patchwork [qom-next] qom: make object cast assert if NULL object is passed as argument

login
register
mail settings
Submitter Igor Mammedov
Date May 30, 2012, 4:19 p.m.
Message ID <1338394747-17427-1-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/162026/
State New
Headers show

Comments

Igor Mammedov - May 30, 2012, 4:19 p.m.
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(-)
Andreas Färber - May 30, 2012, 4:24 p.m.
Am 30.05.2012 18:19, schrieb Igor Mammedov:
> 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>

Acked-by: Andreas Färber <afaerber@suse.de>

If nobody objects, I'll queue it on qom-next.

Andreas

> ---
>  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) {
Mitsyanko Igor - May 30, 2012, 5:05 p.m.
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).
Igor Mammedov - May 31, 2012, 8:11 a.m.
On 05/30/2012 07:05 PM, Igor Mitsyanko wrote:
> 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).
sort grep shows that no one is casting NULLs to anything, what's the point?
Markus Armbruster - May 31, 2012, 8:30 a.m.
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.
Mitsyanko Igor - May 31, 2012, 9:34 a.m.
On 05/31/2012 12:11 PM, Igor Mammedov wrote:
> On 05/30/2012 07:05 PM, Igor Mitsyanko wrote:
>> 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).
> sort grep shows that no one is casting NULLs to anything, what's the point?
>
No one passing NULL to OBJECT_CHECK() either, what I'm talking about is 
cases when we cast some variable foo to Object * with OBJECT(foo) when 
foo == NULL. But Markus thinks that its OK to do that, perhaps he's 
right, asserting will impose unnecessary restrictions on developers.
Igor Mammedov - May 31, 2012, 9:49 a.m.
On 05/31/2012 11:34 AM, Igor Mitsyanko wrote:
> On 05/31/2012 12:11 PM, Igor Mammedov wrote:
>> On 05/30/2012 07:05 PM, Igor Mitsyanko wrote:
>>> 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).
>> sort grep shows that no one is casting NULLs to anything, what's the point?
>>
> No one passing NULL to OBJECT_CHECK() either, what I'm talking about is cases when we cast some variable foo to Object * with OBJECT(foo) when foo ==
> NULL. But Markus thinks that its OK to do that, perhaps he's right, asserting will impose unnecessary restrictions on developers.

Ah I see, I've got confused, sorry.
Paolo Bonzini - May 31, 2012, 10:16 a.m.
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?

Paolo
Igor Mammedov - May 31, 2012, 11:17 a.m.
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

> Paolo
>
Andreas Färber - June 1, 2012, 9:52 a.m.
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
Andreas Färber - June 1, 2012, 9:57 a.m.
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
Markus Armbruster - June 1, 2012, 11:18 a.m.
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.
Andreas Färber - June 1, 2012, 1:04 p.m.
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
Markus Armbruster - June 4, 2012, 7:39 a.m.
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.

[...]
Igor Mammedov - June 4, 2012, 1:14 p.m.
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
>

Patch

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