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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

 -George

> 
> Regards,
> Dario
> 


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