Re: [Xen-devel] [PATCH 6 of 9] libxl: expose cpu topology as a single list of cpu->{node, core, socket} maps

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[Xen-devel] [PATCH 6 of 9] libxl: expose cpu topology as 
a single list of cpu->{node, core, socket} maps"):
> libxl: expose cpu topology as a single list of cpu->{node,core,socket} maps.
> 
> Rather than the previous tripple list which is more complicated to work with
> and harder for language bindings.

This is plausible.  But:

> +#if 0
>  static void libxl_cpuarray_rand_init(libxl_cpuarray *p)
>  {
>      int i;
> @@ -209,6 +210,7 @@ static void libxl_cpuarray_rand_init(lib
>              p->array[i] = r;
>      }
>  }
> +#endif

You haven't quite finished ?

> +    for (cpu = 0; cpu < nr; cpu++) {
...
> +        libxl_cputopology_dispose(&topology[cpu]);
>      }
>  
> -    libxl_topologyinfo_dispose(&topology);
> -
> +    free(topology);

This is quite ugly to have out here in the caller.  Perhaps we should
provide a helper for this, called libxl_cputopology_free or
something ?

> -    libxl_topologyinfo_dispose(&topology);
> +    for (cpu = 0; cpu < nr_cpus; cpu++)
> +        libxl_cputopology_dispose(&topology[cpu]);
> +    free(topology);

And here it is again, proving my point :-).

> +#define LIBXL_CPUTOPOLOGY_INVALID_ENTRY ~0
...
> +libxl_cputopology = Struct("cputopology", [
> +    ("core", uint32),
> +    ("socket", uint32),
> +    ("node", uint32),

You mean (~(uint32_t)0) I think.  The outer ( ) should be included too!

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel