On Thu, 17 Sep 2020 09:38:49 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> David Gibson <da...@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 <vsement...@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... >
