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

[Markus Armbruster <armbru@redhat.com>]

Greg Kurz <groug@kaod.org> writes:

> On Thu, 17 Sep 2020 09:38:49 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote:
>> >> On Tue, 15 Sep 2020 13:58:53 +0300
>> >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> >> 
>> >> > 14.09.2020 15:35, Greg Kurz wrote:
>> >> > > As recommended in "qapi/error.h", add a bool return value to
>> >> > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
>> >> > > of local_err in spapr_memory_plug().
>> >> > > 
>> >> > > Since object_property_get_uint() only returns 0 on failure, use
>> >> > > that as well.
>> >> > 
>> >> > Why are you sure? Can't the property be 0 itself?
>> >> > 
>> >> 
>> >> Hmmm... I've based this assumption on the header:
>> >> 
>> >>  * Returns: the value of the property, converted to an unsigned integer, or 0
>> >>  * an error occurs (including when the property value is not an integer).
>> >> 
>> >> but looking at the implementation, I don't see any check that
>> >> a property cannot be 0 indeed...
>> >
>> > Yeah, indeed I'm pretty sure it can.
>> 
>> Correct.
>> 
>> Corollary: you can't use to return value to check for failure, except
>> when you know the property cannot be zero (you commonly don't).
>> 
>> The function predates our (rather late) realization that returning a
>> recognizable error value in addition to setting an error leads to more
>> readable code.  Today, we'd perhaps do it the way you describe below.
>> 
>> >> It's a bit misleading to mention this in the header though. I
>> >> understand that the function should return something, which
>> >> should have a some explicitly assigned value to avoid compilers
>> >> or static analyzers to yell, but the written contract should be
>> >> that the value is _undefined_ IMHO.
>> >
>> > Hrm... I think the description could be clearer, but returning 0
>> > explicitly on the failure case has some benefits too.  If 0 is a
>> > reasonable default for when the property isn't present (which is a
>> > plausibly common case) then it means you can just get a value and
>> > ignore errors.
>> 
>> Matter of taste.
>> 
>> There's no shortage of _undefined_ in C...
>> 
>
> Yeah and each compiler has its take as how to handle that.
>
> FWIW see section 3.1 of this bachelor thesis on the topic:
>
> https://www.cs.ru.nl/bachelors-theses/2017/Matthias_Vogelaar___4372913___Defining_the_Undefined_in_C.pdf
>
>> >> In its present form, the only way to know if the property is
>> >> valid is to pass a non-NULL errp actually. I'd rather even see
>> >> that in the contract, and an assert() in the code.
>> >
>> > Maybe... see above.
>> 
>> If you think the contract could be improved, please post a patch.
>> 
>
> The contract of object_property_get_enum() which is the next function
> in object.h explicitly says that the result is undefined, even if
> the implementation returns 0. So I was thinking of doing the same
> for object_property_get_uint().

Let's survey actual behavior of the object_property_get*():

                           return value
    function            on success  on error
    o_p_get()           true        false
    o_p_get_str()       non-null    null
    o_p_get_link()      anything    null
    o_p_get_bool()      anything    false
    o_p_get_int()       anything    -1
    o_p_get_uint()      anything    0
    o_p_get_enum()      enum value  0 or -1

object_property_get() and object_property_get_str() have a distinct
error value.  Yes, a QAPI str cannot be null.

object_property_get_enum() has *two* error values, and one of them can
also occur as success value.  This is daft.  I'll send a patch to always
return -1 on error.  Bonus: distinct error value.

object_property_get_link(), _bool(), _int(), and _uint() don't have a
distinct error value.

>> What assertion do you have in mind?  If it's adding assert(errp) to
>> object_property_get_uint(), I'll object.  Functions should not place
>> additional restrictions on @errp arguments without a compelling reason.
>> 
>
> I had such an assertion in mind but if you think this restriction is
> abusive, I take your word :)
>
>> >> An alternative would be to convert it to have a bool return
>> >> value and get the actual uint result with a pointer argument.
>> >
>> > I don't think this is a good idea.  Returning success/failure as the
>> > return value is a good rule of thumb because it reduces the amount of
>> > checking of out-of-band information you need to do.  If you move to
>> > returning the actual value you're trying to get out of band in this
>> > sense, it kind of defeats that purpose.
>> >
>> > I think this one is a case where it is reasonable to make it required
>> > to explicitly check the error value.
>> 
>> If almost all calls assign the value to a variable, like
>> 
>>     val = object_property_get_uint(obj, name, &err);
>>     if (err) {
>>         error_propagate(errp, err)
>>         ... bail out ...
>>     }
>>     ... use val ...
>> 
>> then the alternative Greg proposed is easier on the eyes:
>> 
>>     if (!object_property_get_uint(obj, name, &val, errp)) {
>>         ... bail out ...
>>     }
>>     ... use val ...
>> 
>
> That's what I had in mind.
>
>> It isn't for calls that use the value without storing it in a variable
>> first.
>> 
>
> $ 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.
    
> Cheers,
>
> --
> Greg
>
>> >> > > Also call ERRP_GUARD() to be able to check the status of void
>> >> > > function pc_dimm_plug() with *errp.
>> >> 
>> >> I'm now hesitating to either check *errp for object_property_get_uint()
>> >> too or simply drop this patch...
>>