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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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".

> 
> 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.

Thoughts?

 -George


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