| Message ID | 1338394747-17427-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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) {
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).
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?
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.
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.
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.
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
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 >
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
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
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.
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
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. [...]
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 >
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) {
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(-)