Re: [PATCH] tools/libxc: Fix error checking for xc_get_{cpu, node}map_size() callers

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2310 bytes --]

On gio, 2013-12-12 at 21:05 +0000, Andrew Cooper wrote:
> On 12/12/2013 14:56, Dario Faggioli wrote:
> > Yep, I confirm that, after that changeset, neither
> > xc_get_max_{cpus,nodes}() nor xc_get_{cpu,node}map_size() return 0 as an
> > error anymore.
> 
> Zero might not be "the error condition" any more, but it is certainly an
> error from any of these functions (and possible as
> xc_get_max_{cpus,nodes}() is capable of returning 0 if Xen hands back -1
> for physinfo.max_{cpu,node}_id)
> 
Well, yes, but under what circumstances Xen would do such a thing? As
far as I can see, max_node_id is just 'MAX_NUMNODES-1'. max_cpu_id is
'nr_cpu_ids-1', nr_cpu_ids is '__read_mostly nr_cpu_ids = NR_CPUS'.

I may be wrong, but it looks to me that either both MAX_NUMNODES and
NR_CPUS (and nr_cpu_ids+1 too, if it changes) are > 0, or the system
would be experiencing way bigger issues than misdimensioning a bitmap.

What I mean is, if we are there checking, we at least have one node and
one cpu. In which case, either the call failed and returned <0, or it
succeeded, and returned >0.

What am I missing?

> xc_{cpu/node}map_alloc() must strictly still be "<= 0" checks to avoid
> the issue where calloc(1, 0) returns a non-NULL pointer.
> 
Here `man calloc' says, among other things: "The memory is set to zero.
If nmemb or size is 0, then calloc() returns either NULL, or a unique
pointer value that can later be successfully passed to free()."

Was it that what you were referring to?

> Currently, I am of the opinion that the patch is better as is, than
> changing some of the checks to being strictly "< 0"
> 
Given the first part of this reply (if I'm not mistaken in there) I'd
prefer the other way round. I.e., '< 0' whenever it makes sense and, if
it's an actual issue, '<= 0' in xc_{cpu/node}map_alloc(), perhaps with a
comment, saying that the '<=' is there to prevent calloc madness. :-)

That being said, I'm happy with whatever solution a tool maintainer
likes better.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel