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

From: Anthony PERARD <anthony.perard@citrix.com>
To: Paul Durrant <pdurrant@amazon.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH-for-4.13 v5] Rationalize max_grant_frames and max_maptrack_frames handling
Date: Fri, 29 Nov 2019 12:46:07 +0000	[thread overview]
Message-ID: <20191129124607.GD1155@perard.uk.xensource.com> (raw)
In-Reply-To: <20191128165224.2959-1-pdurrant@amazon.com>

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?

>      
>      ("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?

Thanks,

-- 
Anthony PERARD

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