[Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Igor Mammedov]

in case if caller setting property doesn't care about error and
passes in NULL as errp argument but error occurs in property setter,
it is silently discarded leaving object in undefined state.

As result it leads to hard to find bugs, so if caller doesn't
care about error it must be sure that property exists and
accepts provided value, otherwise it's better to abort early
since error case couldn't be handled gracefully and find
invalid usecase early.

In addition multitude of property setters will be always
guarantied to have error object present and won't be required
to handle this condition individually.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qom/object.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index fc19cf6..2c0bb64 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -792,16 +792,25 @@ void object_property_get(Object *obj, Visitor *v, const char *name,
 void object_property_set(Object *obj, Visitor *v, const char *name,
                          Error **errp)
 {
-    ObjectProperty *prop = object_property_find(obj, name, errp);
-    if (prop == NULL) {
-        return;
+    Error *local_error = NULL;
+    ObjectProperty *prop = object_property_find(obj, name, &local_error);
+    if (local_error) {
+        goto out;
     }
 
     if (!prop->set) {
-        error_set(errp, QERR_PERMISSION_DENIED);
+        error_set(&local_error, QERR_PERMISSION_DENIED);
     } else {
-        prop->set(obj, v, prop->opaque, name, errp);
+        prop->set(obj, v, prop->opaque, name, &local_error);
     }
+out:
+    if (local_error) {
+        if (!errp) {
+            assert_no_error(local_error);
+        }
+        error_propagate(errp, local_error);
+    }
+
 }
 
 void object_property_set_str(Object *obj, const char *value,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

On 11/27/2013 06:24 PM, Igor Mammedov wrote:
> in case if caller setting property doesn't care about error and
> passes in NULL as errp argument but error occurs in property setter,
> it is silently discarded leaving object in undefined state.
> 
> As result it leads to hard to find bugs, so if caller doesn't
> care about error it must be sure that property exists and
> accepts provided value, otherwise it's better to abort early
> since error case couldn't be handled gracefully and find
> invalid usecase early.
> 
> In addition multitude of property setters will be always
> guarantied to have error object present and won't be required

s/guarantied/guaranteed/

> to handle this condition individually.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

> +out:
> +    if (local_error) {

This says local_error was set...

> +        if (!errp) {
> +            assert_no_error(local_error);

so this assert_no_error() is dead code in its current position.  To be
useful, you probably want:

if (!errp) {
    assert_no_error(local_error);
} else if (local_error) {
    error_propagate(errp, local_error);
}

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Igor Mammedov]

On Wed, 27 Nov 2013 21:58:18 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 11/27/2013 06:24 PM, Igor Mammedov wrote:
> > in case if caller setting property doesn't care about error and
> > passes in NULL as errp argument but error occurs in property setter,
> > it is silently discarded leaving object in undefined state.
> > 
> > As result it leads to hard to find bugs, so if caller doesn't
> > care about error it must be sure that property exists and
> > accepts provided value, otherwise it's better to abort early
> > since error case couldn't be handled gracefully and find
> > invalid usecase early.
> > 
> > In addition multitude of property setters will be always
> > guarantied to have error object present and won't be required
> 
> s/guarantied/guaranteed/
thanks, I'll fix it.

> 
> > to handle this condition individually.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> 
> > +out:
> > +    if (local_error) {
> 
> This says local_error was set...
> 
> > +        if (!errp) {
> > +            assert_no_error(local_error);
> 
> so this assert_no_error() is dead code in its current position.  To be
> useful, you probably want:
it is not, retested it again and it still fails when errp == NULL but
there is a error in local_error.

> 
> if (!errp) {
>     assert_no_error(local_error);
> } else if (local_error) {
>     error_propagate(errp, local_error);
> }
this's just another way to do the same thing.

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Peter Crosthwaite]

Hi,

On Thu, Nov 28, 2013 at 11:24 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> in case if caller setting property doesn't care about error and
> passes in NULL as errp argument but error occurs in property setter,
> it is silently discarded leaving object in undefined state.
>
> As result it leads to hard to find bugs, so if caller doesn't
> care about error it must be sure that property exists and
> accepts provided value, otherwise it's better to abort early
> since error case couldn't be handled gracefully and find
> invalid usecase early.
>
> In addition multitude of property setters will be always
> guarantied to have error object present and won't be required
> to handle this condition individually.
>

So there seems to be contention between different APIs and use cases
on what to do in the NULL case. Personally, I'm of the opinion that
NULL should be a silent don't care policy. But you are right in that
checking every API call at call site is cumbersome, and its better to
have some sort of collective mechanism to implement different policys.
I think this can be done caller side efficiently by having a special
Error * that can be passed in that tells the error API to assert or
error raise.

extern error *abort_on_err;

And update your call sites to do this:

object_property_set(Foo, bar, "baz", &abort_on_err);

Error_set and friends are then taught to recognise abort_on_err and
abort accordingly and theres no need for this form of change pattern -
its all solved in the error API.

You can also implement a range of automatic error handling policies e.g:

extern Error *report_err; /* report any errors to monitor/stdout */

To report an error without the assertion.

Regards,
Peter

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  qom/object.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index fc19cf6..2c0bb64 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -792,16 +792,25 @@ void object_property_get(Object *obj, Visitor *v, const char *name,
>  void object_property_set(Object *obj, Visitor *v, const char *name,
>                           Error **errp)
>  {
> -    ObjectProperty *prop = object_property_find(obj, name, errp);
> -    if (prop == NULL) {
> -        return;
> +    Error *local_error = NULL;
> +    ObjectProperty *prop = object_property_find(obj, name, &local_error);
> +    if (local_error) {
> +        goto out;
>      }
>
>      if (!prop->set) {
> -        error_set(errp, QERR_PERMISSION_DENIED);
> +        error_set(&local_error, QERR_PERMISSION_DENIED);
>      } else {
> -        prop->set(obj, v, prop->opaque, name, errp);
> +        prop->set(obj, v, prop->opaque, name, &local_error);
>      }
> +out:
> +    if (local_error) {
> +        if (!errp) {
> +            assert_no_error(local_error);
> +        }
> +        error_propagate(errp, local_error);
> +    }
> +
>  }
>
>  void object_property_set_str(Object *obj, const char *value,
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Markus Armbruster]

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> Hi,
>
> On Thu, Nov 28, 2013 at 11:24 AM, Igor Mammedov <imammedo@redhat.com> wrote:
>> in case if caller setting property doesn't care about error and
>> passes in NULL as errp argument but error occurs in property setter,
>> it is silently discarded leaving object in undefined state.
>>
>> As result it leads to hard to find bugs, so if caller doesn't
>> care about error it must be sure that property exists and
>> accepts provided value, otherwise it's better to abort early
>> since error case couldn't be handled gracefully and find
>> invalid usecase early.
>>
>> In addition multitude of property setters will be always
>> guarantied to have error object present and won't be required
>> to handle this condition individually.
>>
>
> So there seems to be contention between different APIs and use cases
> on what to do in the NULL case. Personally, I'm of the opinion that
> NULL should be a silent don't care policy. But you are right in that
> checking every API call at call site is cumbersome, and its better to
> have some sort of collective mechanism to implement different policys.
> I think this can be done caller side efficiently by having a special
> Error * that can be passed in that tells the error API to assert or
> error raise.
>
> extern error *abort_on_err;
>
> And update your call sites to do this:
>
> object_property_set(Foo, bar, "baz", &abort_on_err);
>
> Error_set and friends are then taught to recognise abort_on_err and
> abort accordingly and theres no need for this form of change pattern -
> its all solved in the error API.

The existing practice is to have a pair of functions, like this one:

    Foo *foo(int arg, Error **errp)
    {
    ...
    }

    Foo *foo_nofail(int arg)
    {
        Error *local_err = NULL;
        Foo *f = foo(arg, &local_err);
        assert_no_error(local_err);
        return f;
    }

The asserting function's name is intentionally longer.

Passing NULL is still temptingly short.  Code review has to catch abuse
of it.

Your proposal is more flexible.  If we adopt it, we may want to retire
the _nofail idiom.

> You can also implement a range of automatic error handling policies e.g:
>
> extern Error *report_err; /* report any errors to monitor/stdout */
>
> To report an error without the assertion.

monitor/stderr, you mean.

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Igor Mammedov]

On Thu, 28 Nov 2013 15:10:50 +1000
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> Hi,
> 
> On Thu, Nov 28, 2013 at 11:24 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> > in case if caller setting property doesn't care about error and
> > passes in NULL as errp argument but error occurs in property setter,
> > it is silently discarded leaving object in undefined state.
> >
> > As result it leads to hard to find bugs, so if caller doesn't
> > care about error it must be sure that property exists and
> > accepts provided value, otherwise it's better to abort early
> > since error case couldn't be handled gracefully and find
> > invalid usecase early.
> >
> > In addition multitude of property setters will be always
> > guarantied to have error object present and won't be required
> > to handle this condition individually.
> >
> 
> So there seems to be contention between different APIs and use cases
> on what to do in the NULL case. Personally, I'm of the opinion that
> NULL should be a silent don't care policy. But you are right in that
If caller doesn't care about setting property was successful, it perhaps
shouldn't call property setter at all or pass a dummy errp for special
case if he really doesn't care about error (I can't really come up with
use case though).

Otherwise there is chance that property setter fails and object won't be
in state it's supposed to be. If developer passes NULL for errp, that
should mean that he is sure call will succeed, if it fails/aborts
developer will find about error much earlier than when in field deployment
starts misbehaving in unpredicable way.

Doing it in object_property_set() also would allow
to remove a bunch of duplicate code like:
Error *err = NULL;
...
assert_no_error(err);

in device_post_init(), qdev_prop_set_...() and other places


> checking every API call at call site is cumbersome, and its better to
> have some sort of collective mechanism to implement different policys.
> I think this can be done caller side efficiently by having a special
> Error * that can be passed in that tells the error API to assert or
> error raise.
> 
> extern error *abort_on_err;
> 
> And update your call sites to do this:
> 
> object_property_set(Foo, bar, "baz", &abort_on_err);
that is just another way to put burden on caller, instead of doing it
in one place.

> 
> Error_set and friends are then taught to recognise abort_on_err and
> abort accordingly and theres no need for this form of change pattern -
> its all solved in the error API.
> 
> You can also implement a range of automatic error handling policies e.g:
> 
> extern Error *report_err; /* report any errors to monitor/stdout */
> 
> To report an error without the assertion.
> 
> Regards,
> Peter
> 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  qom/object.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index fc19cf6..2c0bb64 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -792,16 +792,25 @@ void object_property_get(Object *obj, Visitor *v, const char *name,
> >  void object_property_set(Object *obj, Visitor *v, const char *name,
> >                           Error **errp)
> >  {
> > -    ObjectProperty *prop = object_property_find(obj, name, errp);
> > -    if (prop == NULL) {
> > -        return;
> > +    Error *local_error = NULL;
> > +    ObjectProperty *prop = object_property_find(obj, name, &local_error);
> > +    if (local_error) {
> > +        goto out;
> >      }
> >
> >      if (!prop->set) {
> > -        error_set(errp, QERR_PERMISSION_DENIED);
> > +        error_set(&local_error, QERR_PERMISSION_DENIED);
> >      } else {
> > -        prop->set(obj, v, prop->opaque, name, errp);
> > +        prop->set(obj, v, prop->opaque, name, &local_error);
> >      }
> > +out:
> > +    if (local_error) {
> > +        if (!errp) {
> > +            assert_no_error(local_error);
> > +        }
> > +        error_propagate(errp, local_error);
> > +    }
> > +
> >  }
> >
> >  void object_property_set_str(Object *obj, const char *value,
> > --
> > 1.8.3.1
> >
> >

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Paolo Bonzini]

Il 28/11/2013 14:23, Igor Mammedov ha scritto:
> > object_property_set(Foo, bar, "baz", &abort_on_err);
> 
> that is just another way to put burden on caller, instead of doing it
> in one place.

It's also much more self-documenting.

The problem is that there is one very good case where you want the
silent-don't-care behavior: when you don't care about the exact error,
and you can figure out whether there was an error from the returned
value of the function.  This doesn't apply to object_property_set of
course, but it is the reason why NULL Error* has silent-don't-care behavior.

Now, let's look at the alternatives:

* keep silent don't care
  + consistent
  + predictable
  - not always handy

* only modify object_property_set
  + mostly does the right thing
  - inconsistent with other Error* functions
  - inconsistent with _nofail functions

* Peter's alternative
  + self-documenting
  + consistent
  + predictable

* make Error* mandatory for all void functions
  + consistent
  + almost predictable (because in C you can ignore return values)
  - not necessarily does the right thing (e.g. cleanup functions)
  - requires manual effort to abide to the policy

I vote for Peter's proposal, or for adding object_property_set_nofail.
No particular preference.

Another variant: modify object_property_set to add a g_warning.  I think
it's fine.  It reduces the inconsistency, and still simplifies debugging.

Paolo

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 28/11/2013 14:23, Igor Mammedov ha scritto:
>> > object_property_set(Foo, bar, "baz", &abort_on_err);
>> 
>> that is just another way to put burden on caller, instead of doing it
>> in one place.
>
> It's also much more self-documenting.
>
> The problem is that there is one very good case where you want the
> silent-don't-care behavior: when you don't care about the exact error,
> and you can figure out whether there was an error from the returned
> value of the function.  This doesn't apply to object_property_set of
> course, but it is the reason why NULL Error* has silent-don't-care behavior.
>
> Now, let's look at the alternatives:
>
> * keep silent don't care
>   + consistent
>   + predictable
>   - not always handy
>
> * only modify object_property_set
>   + mostly does the right thing
>   - inconsistent with other Error* functions
>   - inconsistent with _nofail functions
>
> * Peter's alternative
>   + self-documenting
>   + consistent
>   + predictable
>
> * make Error* mandatory for all void functions
>   + consistent
>   + almost predictable (because in C you can ignore return values)
>   - not necessarily does the right thing (e.g. cleanup functions)
>   - requires manual effort to abide to the policy
>
> I vote for Peter's proposal, or for adding object_property_set_nofail.
> No particular preference.
>
> Another variant: modify object_property_set to add a g_warning.  I think
> it's fine.  It reduces the inconsistency, and still simplifies debugging.

I like Peter's proposal, provided we use it to get rid of the _nofail
pattern.

Second preference is adding another _nofail wrapper.

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Peter Crosthwaite]

On Fri, Nov 29, 2013 at 1:03 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 28/11/2013 14:23, Igor Mammedov ha scritto:
>>> > object_property_set(Foo, bar, "baz", &abort_on_err);
>>>
>>> that is just another way to put burden on caller, instead of doing it
>>> in one place.
>>
>> It's also much more self-documenting.
>>
>> The problem is that there is one very good case where you want the
>> silent-don't-care behavior: when you don't care about the exact error,
>> and you can figure out whether there was an error from the returned
>> value of the function.  This doesn't apply to object_property_set of
>> course, but it is the reason why NULL Error* has silent-don't-care behavior.
>>
>> Now, let's look at the alternatives:
>>
>> * keep silent don't care
>>   + consistent
>>   + predictable
>>   - not always handy
>>
>> * only modify object_property_set
>>   + mostly does the right thing
>>   - inconsistent with other Error* functions
>>   - inconsistent with _nofail functions
>>
>> * Peter's alternative
>>   + self-documenting
>>   + consistent
>>   + predictable
>>
>> * make Error* mandatory for all void functions
>>   + consistent
>>   + almost predictable (because in C you can ignore return values)
>>   - not necessarily does the right thing (e.g. cleanup functions)
>>   - requires manual effort to abide to the policy
>>
>> I vote for Peter's proposal, or for adding object_property_set_nofail.
>> No particular preference.
>>
>> Another variant: modify object_property_set to add a g_warning.  I think
>> it's fine.  It reduces the inconsistency, and still simplifies debugging.
>
> I like Peter's proposal, provided we use it to get rid of the _nofail
> pattern.
>
> Second preference is adding another _nofail wrapper.
>

So this issue with _nofail is that if you are doing it properly, every
API needed both normal and _nofail version of every function. APIs
generally have no bussiness dictating failure policy so by extension,
normal and _nofail should exist for every API that accepts an Error *.
With my proposal, its fixed once, in one place and we can torch all
the _nofail boilerplate all over the tree as you have suggested.

These is another subtle advantage to my proposal, and that is that
assertions can happen at the point of failure in the offending API,
not after the fact in the caller. If the caller does an
assert_no_error, then the abort() happens after return from the API
call. This makes debugging awkward cause the stack frames into the API
call where the issue actually occured are lost. Whereas if error_set
does the abort() you will get all stack frames into the API call where
the issue occured when gdb traps the abort().

Regards,
Peter

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Markus Armbruster]

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> On Fri, Nov 29, 2013 at 1:03 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 28/11/2013 14:23, Igor Mammedov ha scritto:
>>>> > object_property_set(Foo, bar, "baz", &abort_on_err);
>>>>
>>>> that is just another way to put burden on caller, instead of doing it
>>>> in one place.
>>>
>>> It's also much more self-documenting.
>>>
>>> The problem is that there is one very good case where you want the
>>> silent-don't-care behavior: when you don't care about the exact error,
>>> and you can figure out whether there was an error from the returned
>>> value of the function.  This doesn't apply to object_property_set of
>>> course, but it is the reason why NULL Error* has silent-don't-care behavior.
>>>
>>> Now, let's look at the alternatives:
>>>
>>> * keep silent don't care
>>>   + consistent
>>>   + predictable
>>>   - not always handy
>>>
>>> * only modify object_property_set
>>>   + mostly does the right thing
>>>   - inconsistent with other Error* functions
>>>   - inconsistent with _nofail functions
>>>
>>> * Peter's alternative
>>>   + self-documenting
>>>   + consistent
>>>   + predictable
>>>
>>> * make Error* mandatory for all void functions
>>>   + consistent
>>>   + almost predictable (because in C you can ignore return values)
>>>   - not necessarily does the right thing (e.g. cleanup functions)
>>>   - requires manual effort to abide to the policy
>>>
>>> I vote for Peter's proposal, or for adding object_property_set_nofail.
>>> No particular preference.
>>>
>>> Another variant: modify object_property_set to add a g_warning.  I think
>>> it's fine.  It reduces the inconsistency, and still simplifies debugging.
>>
>> I like Peter's proposal, provided we use it to get rid of the _nofail
>> pattern.
>>
>> Second preference is adding another _nofail wrapper.
>>
>
> So this issue with _nofail is that if you are doing it properly, every
> API needed both normal and _nofail version of every function. APIs
> generally have no bussiness dictating failure policy so by extension,
> normal and _nofail should exist for every API that accepts an Error *.
> With my proposal, its fixed once, in one place and we can torch all
> the _nofail boilerplate all over the tree as you have suggested.
>
> These is another subtle advantage to my proposal, and that is that
> assertions can happen at the point of failure in the offending API,
> not after the fact in the caller. If the caller does an
> assert_no_error, then the abort() happens after return from the API
> call. This makes debugging awkward cause the stack frames into the API
> call where the issue actually occured are lost. Whereas if error_set
> does the abort() you will get all stack frames into the API call where
> the issue occured when gdb traps the abort().

To make further progress, we need a patch.  Care to cook one up?

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Peter Crosthwaite]

On Fri, Nov 29, 2013 at 5:56 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>
>> On Fri, Nov 29, 2013 at 1:03 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>
>>>> Il 28/11/2013 14:23, Igor Mammedov ha scritto:
>>>>> > object_property_set(Foo, bar, "baz", &abort_on_err);
>>>>>
>>>>> that is just another way to put burden on caller, instead of doing it
>>>>> in one place.
>>>>
>>>> It's also much more self-documenting.
>>>>
>>>> The problem is that there is one very good case where you want the
>>>> silent-don't-care behavior: when you don't care about the exact error,
>>>> and you can figure out whether there was an error from the returned
>>>> value of the function.  This doesn't apply to object_property_set of
>>>> course, but it is the reason why NULL Error* has silent-don't-care behavior.
>>>>
>>>> Now, let's look at the alternatives:
>>>>
>>>> * keep silent don't care
>>>>   + consistent
>>>>   + predictable
>>>>   - not always handy
>>>>
>>>> * only modify object_property_set
>>>>   + mostly does the right thing
>>>>   - inconsistent with other Error* functions
>>>>   - inconsistent with _nofail functions
>>>>
>>>> * Peter's alternative
>>>>   + self-documenting
>>>>   + consistent
>>>>   + predictable
>>>>
>>>> * make Error* mandatory for all void functions
>>>>   + consistent
>>>>   + almost predictable (because in C you can ignore return values)
>>>>   - not necessarily does the right thing (e.g. cleanup functions)
>>>>   - requires manual effort to abide to the policy
>>>>
>>>> I vote for Peter's proposal, or for adding object_property_set_nofail.
>>>> No particular preference.
>>>>
>>>> Another variant: modify object_property_set to add a g_warning.  I think
>>>> it's fine.  It reduces the inconsistency, and still simplifies debugging.
>>>
>>> I like Peter's proposal, provided we use it to get rid of the _nofail
>>> pattern.
>>>
>>> Second preference is adding another _nofail wrapper.
>>>
>>
>> So this issue with _nofail is that if you are doing it properly, every
>> API needed both normal and _nofail version of every function. APIs
>> generally have no bussiness dictating failure policy so by extension,
>> normal and _nofail should exist for every API that accepts an Error *.
>> With my proposal, its fixed once, in one place and we can torch all
>> the _nofail boilerplate all over the tree as you have suggested.
>>
>> These is another subtle advantage to my proposal, and that is that
>> assertions can happen at the point of failure in the offending API,
>> not after the fact in the caller. If the caller does an
>> assert_no_error, then the abort() happens after return from the API
>> call. This makes debugging awkward cause the stack frames into the API
>> call where the issue actually occured are lost. Whereas if error_set
>> does the abort() you will get all stack frames into the API call where
>> the issue occured when gdb traps the abort().
>
> To make further progress, we need a patch.  Care to cook one up?
>

Done.

Regards,
Peter

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Thu, 28 Nov 2013 15:10:50 +1000
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>
>> Hi,
>> 
>> On Thu, Nov 28, 2013 at 11:24 AM, Igor Mammedov <imammedo@redhat.com> wrote:
>> > in case if caller setting property doesn't care about error and
>> > passes in NULL as errp argument but error occurs in property setter,
>> > it is silently discarded leaving object in undefined state.
>> >
>> > As result it leads to hard to find bugs, so if caller doesn't
>> > care about error it must be sure that property exists and
>> > accepts provided value, otherwise it's better to abort early
>> > since error case couldn't be handled gracefully and find
>> > invalid usecase early.
>> >
>> > In addition multitude of property setters will be always
>> > guarantied to have error object present and won't be required
>> > to handle this condition individually.
>> >
>> 
>> So there seems to be contention between different APIs and use cases
>> on what to do in the NULL case. Personally, I'm of the opinion that
>> NULL should be a silent don't care policy. But you are right in that
> If caller doesn't care about setting property was successful, it perhaps
> shouldn't call property setter at all or pass a dummy errp for special
> case if he really doesn't care about error (I can't really come up with
> use case though).
>
> Otherwise there is chance that property setter fails and object won't be
> in state it's supposed to be. If developer passes NULL for errp, that
> should mean that he is sure call will succeed, if it fails/aborts
> developer will find about error much earlier than when in field deployment
> starts misbehaving in unpredicable way.

When you're sure the call can't fail, you really, really want an
assertion failure when it does.

There's a use for "I don't want an error object, thank you": when you
can detect failure in another way, say return value, and aren't
interested in details, and the notational overhead that comes with it.

This case is distinct from "I know this can't fail".

[...]

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Andreas Färber]

Am 28.11.2013 02:24, schrieb Igor Mammedov:
> in case if caller setting property doesn't care about error and
> passes in NULL as errp argument but error occurs in property setter,
> it is silently discarded leaving object in undefined state.
> 
> As result it leads to hard to find bugs, so if caller doesn't
> care about error it must be sure that property exists and
> accepts provided value, otherwise it's better to abort early
> since error case couldn't be handled gracefully and find
> invalid usecase early.
> 
> In addition multitude of property setters will be always
> guarantied to have error object present and won't be required
> to handle this condition individually.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  qom/object.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index fc19cf6..2c0bb64 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -792,16 +792,25 @@ void object_property_get(Object *obj, Visitor *v, const char *name,
>  void object_property_set(Object *obj, Visitor *v, const char *name,
>                           Error **errp)
>  {
> -    ObjectProperty *prop = object_property_find(obj, name, errp);
> -    if (prop == NULL) {
> -        return;
> +    Error *local_error = NULL;
> +    ObjectProperty *prop = object_property_find(obj, name, &local_error);
> +    if (local_error) {
> +        goto out;
>      }
>  
>      if (!prop->set) {
> -        error_set(errp, QERR_PERMISSION_DENIED);
> +        error_set(&local_error, QERR_PERMISSION_DENIED);
>      } else {
> -        prop->set(obj, v, prop->opaque, name, errp);
> +        prop->set(obj, v, prop->opaque, name, &local_error);
>      }
> +out:
> +    if (local_error) {
> +        if (!errp) {
> +            assert_no_error(local_error);
> +        }
> +        error_propagate(errp, local_error);
> +    }
> +
>  }
>  
>  void object_property_set_str(Object *obj, const char *value,

Aborting on NULL errp considered dangerous by me.

This function seems to work just fine with NULL errp, so your focus
seems to be on the callers.

Promoting *not* to abort has been one appeal of the new QOM-style APIs
to me, so making this implicitly assert feels like a step backwards.
The old qdev_prop_set_*() API, which most users are still using, does
assert, as discussed with PMM recently.

Also, why only for setting properties? Either all or none should behave
like this - and I guess none is going to be easier to achieve.
For instance, adding dynamic properties is a use case where in
instance_init I've seen NULL errp passed in (because instance_init API
cannot fail).

I will be more than happy to review and apply your patch (or contribute
further ones) going through (mis)uses of error_is_set().

Regards,
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: abort on error in property setter if caller passed errp == NULL

[Igor Mammedov]

On Thu, 28 Nov 2013 14:42:38 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 28.11.2013 02:24, schrieb Igor Mammedov:
> > in case if caller setting property doesn't care about error and
> > passes in NULL as errp argument but error occurs in property setter,
> > it is silently discarded leaving object in undefined state.
> > 
> > As result it leads to hard to find bugs, so if caller doesn't
> > care about error it must be sure that property exists and
> > accepts provided value, otherwise it's better to abort early
> > since error case couldn't be handled gracefully and find
> > invalid usecase early.
> > 
> > In addition multitude of property setters will be always
> > guarantied to have error object present and won't be required
> > to handle this condition individually.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  qom/object.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/qom/object.c b/qom/object.c
> > index fc19cf6..2c0bb64 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -792,16 +792,25 @@ void object_property_get(Object *obj, Visitor *v, const char *name,
> >  void object_property_set(Object *obj, Visitor *v, const char *name,
> >                           Error **errp)
> >  {
> > -    ObjectProperty *prop = object_property_find(obj, name, errp);
> > -    if (prop == NULL) {
> > -        return;
> > +    Error *local_error = NULL;
> > +    ObjectProperty *prop = object_property_find(obj, name, &local_error);
> > +    if (local_error) {
> > +        goto out;
> >      }
> >  
> >      if (!prop->set) {
> > -        error_set(errp, QERR_PERMISSION_DENIED);
> > +        error_set(&local_error, QERR_PERMISSION_DENIED);
> >      } else {
> > -        prop->set(obj, v, prop->opaque, name, errp);
> > +        prop->set(obj, v, prop->opaque, name, &local_error);
> >      }
> > +out:
> > +    if (local_error) {
> > +        if (!errp) {
> > +            assert_no_error(local_error);
> > +        }
> > +        error_propagate(errp, local_error);
> > +    }
> > +
> >  }
> >  
> >  void object_property_set_str(Object *obj, const char *value,
> 
> Aborting on NULL errp considered dangerous by me.
> 
> This function seems to work just fine with NULL errp, so your focus
> seems to be on the callers.
> 
> Promoting *not* to abort has been one appeal of the new QOM-style APIs
> to me, so making this implicitly assert feels like a step backwards.
> The old qdev_prop_set_*() API, which most users are still using, does
> assert, as discussed with PMM recently.
> 
> Also, why only for setting properties? Either all or none should behave
> like this - and I guess none is going to be easier to achieve.
> For instance, adding dynamic properties is a use case where in
> instance_init I've seen NULL errp passed in (because instance_init API
> cannot fail).
> 
> I will be more than happy to review and apply your patch (or contribute
> further ones) going through (mis)uses of error_is_set().
I've sent such one for target-i386/cpu.c see last patch in x86-properties.v10,
I've posted today.

> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Andreas Färber]

Am 28.11.2013 14:48, schrieb Igor Mammedov:
> On Thu, 28 Nov 2013 14:42:38 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> I will be more than happy to review and apply your patch (or contribute
>> further ones) going through (mis)uses of error_is_set().
> I've sent such one for target-i386/cpu.c see last patch in x86-properties.v10,
> I've posted today.

Yes, that's what I meant with "your patch". :-)

Andreas

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