Greg Kurz <gr...@kaod.org> writes:
> 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().
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...
>>
