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

[Markus Armbruster]

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

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

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.

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

It isn't for calls that use the value without storing it in a variable
first.

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