Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Daniel P . Berrangé]

On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote:
> Greg Kurz <groug@kaod.org> writes:
> 
> > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l
> > 60
> >
> > Manual inspecting the output of
> >
> > $ git grep -W object_property_get_uint -- :^{include,qom/object.c}
> > ...
> >
> > seems to be showing that most users simply ignore errors (ie. pass NULL
> > as 3rd argument). Then some users pass &error_abort and only a few
> > pass a &err or errp.
> >
> > Assuming that users know what they're doing, I guess my proposal
> > wouldn't bring much to the code base in the end... I'm not even
> > sure now that it's worth changing the contract.
> 
> We'd change
> 
>     val = object_property_get_uint(obj, name, &error_abort);
> 
> to
> 
>     object_property_get_uint(obj, name, &val, &error_abort);
> 
> which is not an improvement.
> 
> Most of the ones passing NULL should probably pass &error_abort
> instead.  Doesn't change the argument.

I wonder if we actually need to have an Error  parameter at all for
the getters. IIUC the only valid runtime error  is probably the case
where the property has not been set, and even then, I think properties
usually have a default value that would be returned.  All the other
errors look like programmer errors.

IOW, can we remove the Error parameter and have all the o_p_get*
method impls use error_abort.

In the rare case where a caller needs to handle a property not
being set, they can use object_property_find() as a test before
invoking the getter.

Of course requires someone motivated to audit all the case that
are not using NULL or error_abort and decide whether the attempt
at passing a local errp was actually justified or not.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|