Re: [Xen-devel] [PATCH-for-4.13 v5] Rationalize max_grant_frames and max_maptrack_frames handling

["Durrant, Paul" <pdurrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Sent: 29 November 2019 12:46
> To: Durrant, Paul <pdurrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Julien
> Grall <julien@xxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Marek Marczykowski-Górecki
> <marmarek@xxxxxxxxxxxxxxxxxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH-for-4.13 v5] Rationalize max_grant_frames and
> max_maptrack_frames handling
> 
> On Thu, Nov 28, 2019 at 04:52:24PM +0000, Paul Durrant wrote:
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 49b56fa1a3..a2a5d321c5 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -364,8 +364,8 @@
> >   */
> >  #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
> >
> > -#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32
> > -#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024
> > +#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
> > +#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
> >
> >  /*
> >   * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 0546d7865a..63e29bb2fb 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -511,8 +511,8 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
> >
> >      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
> >
> > -    ("max_grant_frames",    uint32, {'init_val':
> 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> > -    ("max_maptrack_frames", uint32, {'init_val':
> 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
> > +    ("max_grant_frames",    integer, {'init_val':
> 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
> > +    ("max_maptrack_frames", integer, {'init_val':
> 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
> 
> That's a change in the libxl API, could you add a LIBX_HAVE_* macro?
> 

Is it really, in practice?

> >
> >      ("device_model_version", libxl_device_model_version),
> >      ("device_model_stubdomain", libxl_defbool),
> > diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
> > index 72815d25dd..cafc632fc1 100644
> > --- a/tools/libxl/libxlu_cfg.c
> > +++ b/tools/libxl/libxlu_cfg.c
> > @@ -268,8 +268,9 @@ int xlu_cfg_replace_string(const XLU_Config *cfg,
> const char *n,
> >      return 0;
> >  }
> >
> > -int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
> > -                     long *value_r, int dont_warn) {
> > +int xlu_cfg_get_bounded_long(const XLU_Config *cfg, const char *n,
> > +                             long min, long max, long *value_r,
> > +                             int dont_warn) {
> >      long l;
> >      XLU_ConfigSetting *set;
> >      int e;
> > @@ -303,10 +304,31 @@ int xlu_cfg_get_long(const XLU_Config *cfg, const
> char *n,
> >                      cfg->config_source, set->lineno, n);
> >          return EINVAL;
> >      }
> > +    if (l < min) {
> > +        if (!dont_warn)
> > +            fprintf(cfg->report,
> > +                    "%s:%d: warning: value `%ld' is smaller than
> minimum bound '%ld'\n",
> > +                    cfg->config_source, set->lineno, l, min);
> > +        return EINVAL;
> > +    }
> > +    if (l > max) {
> > +        if (!dont_warn)
> > +            fprintf(cfg->report,
> > +                    "%s:%d: warning: value `%ld' is greater than
> maximum bound '%ld'\n",
> > +                    cfg->config_source, set->lineno, l, max);
> > +        return EINVAL;
> > +    }
> 
> I'm not sure what was the intention with the new function
> xlu_cfg_get_bounded_long(), but I don't think libxlu is the right place
> for it. That function is only going to make it harder for users to find
> mistakes in the config file. If `n' value is out of bound, it will only
> get ignored, and xl will keep going. I think xlu_cfg should only be a
> parser (and can check for syntax error).
> 
> Can you move that function to xl?
> 

I can, but why is this not considered useful in libxl? The call returns failure 
for an out-of-bounds check. If xl currently chooses to treat EINVAL as ENOENT 
then that's xl's bug to deal with.

  Paul

> Thanks,
> 
> --
> Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel