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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, May 09, 2013 at 09:10:00AM +0100, Ian Campbell wrote:
> 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@zakaz.uk.xensource.com> 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@oracle.com>
> > ---
> >  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.

OK, making the change.
> 
> >  
> >      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.

Ian Jackson during the review suggested that it just print that warning
and continue on. And now that I am using I actually like the warning. It
reminds me that I forgot to enable or disable it.

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

I can certainly try to redo it. I think I tried it two weeks ago and ran
in the trouble of having to modify a bunch of extra print_* functions to
have the extra claim information. And also not being sure how to expose
it via the JSON or sxp. So I choose the less invasive method as it was
close to the feature freeze (or already past it).

I can certainly rework this for Xen 4.4 ? Or for Xen 4.3 if you think
that George would be OK with that.
> 
> (those last three comments are a bit orthogonal to the actual patch...)

> 
> Ian.
>