I've dropped the SPAM mentions from the subject this time :) On Tue, 15 Sep 2020 14:53:53 +0300 Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote:
> 15.09.2020 14:43, 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... > > > > 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. > I don't expect much people except David or maybe Markus to look at these patches actually. And anyway, it's up to David to merge them. But, yes, I agree patch 14 and 15 should go to the next round. Thanks for the review ! Cheers, -- Greg