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 <address@hidden> wrote:

> Hi,
> 
> On Thu, Nov 28, 2013 at 11:24 AM, Igor Mammedov <address@hidden> 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 <address@hidden>
> > ---
> >  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
> >
> >