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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

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

>From the notes I took when I last hacked a trail through this jungle...

Failure modes of

    object_property_get()
        @name not found
        @name permission, i.e. no ->get
        ->get() fails (how?)

    object_property_get_{str,bool,int,uint}()
        object_property_get_qobject()
            object_property_get() with qobject output visitor
                which see
        prop value qobject not a string / bool / int / uint

    object_property_get_enum()
        @name not found
        @typename doesn't match
        object_property_get() with string output visitor
            which see
        qapi_enum_parse()
            prop value not a value of enum @typename

    object_property_get_link()
        object_property_get_str()
            which see
        prop value does not resolve

I think most of these failures are obviously programming errors most of
the time.

Many callers treat failures as programming errors by passing
&error_abort.

Many callers ignore errors by passing NULL.  I believe most of them
should really pass &error_abort instead.  Fixing them is tedious,
because you have to check what would happen on error.  If the answer is
"chaos", fix to pass &error_abort.  But the answer can also be "works as
intended", or "beats me".

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

If we fix the callers that should pass &error_abort to do so, we'll see
what remains, and whether we can drop the parameter.

> 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 dislike "if (X() is going to fail) do something else; else X()".

I guess it could be okay for the narrow case of "property does not
exist".

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

We got one!  Thanks, Greg :)