From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 03 of 10 v2] libxl, libxc: introduce libxl_get_numainfo() Date: Thu, 21 Jun 2012 12:00:14 +0200 Message-ID: <1340272814.4856.16.camel@Solace> References: <16222d05875389899968.1339779871@Solace> <1340269342.21872.20.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1194041570605301455==" Return-path: In-Reply-To: <1340269342.21872.20.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Andre Przywara , Stefano Stabellini , George Dunlap , Juergen Gross , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org --===============1194041570605301455== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-nR4dY8gfBHS4Qc8CBOA8" --=-nR4dY8gfBHS4Qc8CBOA8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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) > > +{ > [...] >=20 > The hypercall buffer stuff all looks good. >=20 > > + if (ret) > > + *nr =3D max_nodes; >=20 > You could put this before the fail: label. Not that it matters. >=20 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. >=20 > Don't you mean libxl_numinfo_list_free() ? >=20 > Also normally we put the comment before the prototype. >=20 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); > > } > > =20 > > +int libxl_get_max_nodes(libxl_ctx *ctx) > > +{ > > + return xc_get_max_nodes(ctx->xch); > > +} >=20 > 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. >=20 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); > > =20 > > /* XEN_SYSCTL_numainfo */ > > +#define INVALID_NUMAINFO_ID (~0U) >=20 > 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? >=20 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 --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-nR4dY8gfBHS4Qc8CBOA8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iEYEABECAAYFAk/i8K4ACgkQk4XaBE3IOsRyjgCgjTxRDM4Y7MyA5KoPDkDrZwDz eVgAn0LXquY5ny8k0AtoJdndnbq6q2bV =EOOP -----END PGP SIGNATURE----- --=-nR4dY8gfBHS4Qc8CBOA8-- --===============1194041570605301455== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============1194041570605301455==--