Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Feb 08, 2017 at 04:17:58PM +0000, George Dunlap wrote:
> On 08/02/17 16:11, Dario Faggioli wrote:
> > On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
> >> Callers to libxl_cpupool_create() can either request a specific pool
> >> id, or request that Xen do it for them.  But at the moment, the
> >> "automatic" selection is indicated by using a magic value, 0.  This
> >> is
> >> undesirable both because it doesn't obviously have meaning, but also
> >> because '0' is a valid cpupool (albeit one which at the moment can't
> >> be changed).
> >>
> >> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
> >> instead.  Still accept '0' as meaning "ANY" for backwards
> >> compatibility.
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> >>
> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> > 
> > With one remark.
> > 
> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >> --- a/tools/libxl/libxl.h
> >> +++ b/tools/libxl/libxl.h
> >> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
> >> uint32_t domid, char* uuid,
> >>  int libxl_tmem_freeable(libxl_ctx *ctx);
> >>  
> >>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
> >> +
> >> +/* 
> >> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
> >> + * free poolid for you.
> >> + */
> >> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
> >>
> > Do we want this to be here, or in libxl_types.idl.
> > 
> > Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
> > this. I appreciate that there's few point in making this an enum, as it
> > is only one value, and will most likely remain so, but still, I thought
> > I'd at least bring this up.
> > 
> > FWIW, my Reviewed-by stands both if it is kept as is, and if it is
> > moved to IDL.
> 
> Well there's things like:
> 
> #define LIBXL_PCI_FUNC_ALL (~0U)
> 
> #define LIBXL_TIMER_MODE_DEFAULT -1
> #define LIBXL_MEMKB_DEFAULT ~0ULL
> 
> #define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
> 
> #define LIBXL_MS_VM_GENID_LEN 16
> 
> #define LIBXL_SUSPEND_DEBUG 1
> #define LIBXL_SUSPEND_LIVE 2
> 
> Many of which seem similar in some ways.  Enums I think are meant to be
> exhaustive (as in, contain all possible options), not be special cases.
> 
> But I'm happy to defer to the tools maintainers.
> 

I don't really care if it is an enum or a macro.

There is an issue that is  more subtle than where it lives or what form
it is in.

You need to modify all the poolid fields in various structure to make it
as default.  Otherwise the whole json infrastructure would still use 0
as the default value.

And maybe a LIBXL_HAVE macro is needed, too.

Wei.

>  -George
> 
> > 
> > Regards,
> > Dario
> > 
> 

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