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

[Greg Kurz]

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

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

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