Re: [Xen-devel] [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Feb 09, 2017 at 11:17:46AM +0000, George Dunlap wrote:
> On 09/02/17 10:35, Wei Liu wrote:
> > 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@xxxxxxxxxx>
> >>>>
> >>> Reviewed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> >>>
> >>> 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.
> 
> Hmm, at the moment there are only two structs that include poolid:
> cpupoolinfo, which is OUT-only (so the field should always be
> initialized) and domain_create_info, for which 0 is a much better
> default logically than "ANY".
> 

I don't follow here. Isn't 0 already "ANY"?

> > 
> > And maybe a LIBXL_HAVE macro is needed, too.
> 
> I thought about that, but figured people could just do #ifdef
> LIBXL_CPUPOOL_POOLID_ANY.  It seemed a bit strange to define a whole new
> macro just to see if another macro existed.
> 

I want to reserve the possibility to change that into an enum in the
future.

Wei.

> Thoughts?
> 
>  -George
> 

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