[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [SPAM] Re: [PATCH 14/15] spapr: Simplify error handling in spapr_mem

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [SPAM] Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()
Date: Tue, 15 Sep 2020 14:53:53 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0

15.09.2020 14:43, 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...

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.

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.

An alternative would be to convert it to have a bool return
value and get the actual uint result with a pointer argument.

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

.. and the following. If no more reviewers come to look at it, you can just 
merge first 13 patches, not resending the whole series for last two patches, 
which may be moved to round 3.

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]