Re: [Xen-devel] [PATCH 03 of 10 v2] libxl, libxc: introduce libxl_get_numainfo()

[Dario Faggioli <raistlin@xxxxxxxx>]

On Thu, 2012-06-21 at 10:02 +0100, Ian Campbell wrote:
> > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> > --- a/tools/libxc/xenctrl.h
> > +++ b/tools/libxc/xenctrl.h
> ...
> > +libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
> > +{
> [...]
> 
> The hypercall buffer stuff all looks good.
> 
> > +    if (ret)
> > +        *nr = max_nodes;
> 
> You could put this before the fail: label. Not that it matters.
> 
Ok.

> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> ...
> >  #define LIBXL_CPUTOPOLOGY_INVALID_ENTRY (~(uint32_t)0)
> >  libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nr);
> >  void libxl_cputopology_list_free(libxl_cputopology *, int nr);
> > +#define LIBXL_NUMAINFO_INVALID_ENTRY (~(uint32_t)0)
> > +libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr);
> > +  /* On success, a list of nr libxl_numainfo elements is returned.
> > +   * That is from malloc, thus it is up to the caller to invoke
> > +   * libxl_cpupoolinfo_list_free() on it.
> 
> Don't you mean libxl_numinfo_list_free() ?
> 
> Also normally we put the comment before the prototype.
> 
Yes, I did, and will fix it. For the comment, again, I'll move that
up... It's just you can find so much different "examples" in those
files... :-O

> > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > --- a/tools/libxl/libxl_utils.c
> > +++ b/tools/libxl/libxl_utils.c
> > @@ -537,6 +537,11 @@ int libxl_get_max_cpus(libxl_ctx *ctx)
> >      return xc_get_max_cpus(ctx->xch);
> >  }
> >  
> > +int libxl_get_max_nodes(libxl_ctx *ctx)
> > +{
> > +    return xc_get_max_nodes(ctx->xch);
> > +}
> 
> Is this needed externally to libxl or do we expect all callers to use
> libxl_get_numainfo? I suppose there is no harm in exporting this either
> way.
> 
I'm not sure. What I did is to replicate what happens for
libxl_get_max_cpus(), but I really don't know whether or not they both
make any sense outside libxl. It does not look that bad to me that we
offer our users a chance to figure out how many cpus and/or nodes they
have, without needing to call the proper libxl_get_*info(), which is
quite a bit more of a burden. FWIW, I'd leave both of them public.


> > diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -484,6 +484,7 @@ typedef struct xen_sysctl_topologyinfo x
> >  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_topologyinfo_t);
> >  
> >  /* XEN_SYSCTL_numainfo */
> > +#define INVALID_NUMAINFO_ID (~0U)
> 
> It feels like there ought to be hunks in the hypervisor which either use
> this symbol instead of a hardcoded ~0U or which remove the internal
> definition in favour of this one?
> 
Again, -topologyinfo machinery does exactly this, so I really think we
either fix/change or leave as they are both of them (which of course I
can do, just tell me if that is what you want).

Thanks and Regards,
Dario

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

Attachment: signature.asc
Description: This is a digitally signed message part

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