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

[Greg Kurz]

On Thu, 17 Sep 2020 13:18:51 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> 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.
> 

I tend to agree.

> 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.
> 

Since I've open the can of worms, I'm volunteering to do that if
we have a consensus on the fact that "the only valid runtime
error os the case the property has not been set".

Cheers,

--
Greg

> Regards,
> Daniel