Re: [Xen-devel] [PATCH 2/3] libxl: Change claim_mode from bool to int.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote:
> During the review it was noticed that it would be better if internally
> the claim_mode was held as an 'int' instead of a 'bool'. The reason
> is that during the startup of xl, one has call the libxl_defbool_setdefault.
> otherwise any usage of claim_mode would result in assert break.
> 
> The assert is due to the fact that using defbool without any set
> values (either true of false) will cause it hit an assertion.
> 
> If we use an 'int' we don't have to worry about it and by default
> the value of zero will suffice for checks whether the claim is
> enabled or disabled.

In <1366102243.8399.118.camel@xxxxxxxxxxxxxxxxxxxxxx> I mentioned that
it doesn't seem correct that this is a libxl_domain_build_info setting
at all, in that thread you said that claim was a host global setting not
a per domain one.

Perhaps we want to consider moving claim_mode from
libxl_domain_build_info to e.g. libxl_ctx now to save us some API churn
in the future? Oh, except libxl_ctx is opaque to the callers of libxl so
it's not much use and we're therefore back to having to add APIs to
expose these settings and all the faff.

OK, so I think I've convinced myself that this patch is OK for 4.3
(modulo the comment below on the libxl_create.c change) and we can have
a rethink about libxl default handling for 4.4. Keeping the libxl
interface for claim as a defbool in build_info will facilitate us
implementing this rethink since it effectively then becomes a per-domain
override of the host global setting, so it doesn't paint us into too
much of a corner.

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_create.c |    2 --
>  tools/libxl/xl.c           |    6 +++---
>  tools/libxl/xl.h           |    2 +-
>  tools/libxl/xl_cmdimpl.c   |    6 +++---
>  4 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index cb9c822..c116186 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -202,8 +202,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>      if (b_info->target_memkb == LIBXL_MEMKB_DEFAULT)
>          b_info->target_memkb = b_info->max_memkb;
>  
> -    libxl_defbool_setdefault(&b_info->claim_mode, false);

This line should stay, since it allows callers of libxl to not care
about claim if they don't want to (xl does, others may not).

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5259b2e..76ee33a 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -734,7 +734,7 @@ static void parse_config_data(const char *config_source,
>      if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
>          b_info->max_memkb = l * 1024;
>  
> -    b_info->claim_mode = claim_mode;
> +    libxl_defbool_set(&b_info->claim_mode, claim_mode);
>  
>      if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
>          buf = "destroy";
> @@ -4607,7 +4607,7 @@ static void output_physinfo(void)
>      /*
>       * Only if enabled (claim_mode=1) or there are outstanding claims.
>       */
> -    if (libxl_defbool_val(claim_mode) || info.outstanding_pages)
> +    if (claim_mode || info.outstanding_pages)
>          printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);

There wouldn't seem to be much harm in printing this unconditionally, it
would print 0 if claim mode was disabled, which is ok.

>  
>      if (!libxl_get_freecpus(ctx, &cpumap)) {
> @@ -5911,7 +5911,7 @@ int main_claims(int argc, char **argv)
>          /* No options */
>      }
>  
> -    if (!libxl_defbool_val(claim_mode))
> +    if (!claim_mode)
>          fprintf(stderr, "claim_mode not enabled (see man xl.conf).\n");

You don't exit here? Like above I think it would be actually OK for it
to just list the domains with claims of zero.

FWIW It occurs to me now that this could have just been "xl list
--claims/-c", but it's there now.

(those last three comments are a bit orthogonal to the actual patch...)

Ian.


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