tools/libxl/libxlu_cfg.c | 100 +++++++++++++++++++++++++++++++++- tools/libxl/libxlu_cfg_i.h | 1 + tools/libxl/libxlu_cfg_l.l | 1 + tools/libxl/libxlu_cfg_y.y | 4 +- tools/libxl/libxlu_internal.h | 1 + tools/libxl/libxlutil.h | 5 ++ 6 files changed, 109 insertions(+), 3 deletions(-)
Handle += of both strings and lists.
If += is used for config options expected to be numbers, then a
warning is printed and the config option ignored (because xl ignores
config options with errors).
This is to be used for development purposes, where modifying config
option can be done on the `xl create' command line.
One could have a cmdline= in the cfg file, and specify cmdline+= on
the `xl create` command line without the need to write the whole
cmdline in `xl' command line but simply append to the one in the cfg file.
Or add an extra vif or disk by simply having "vif += [ '', ];" in the
`xl' cmdline.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
Commiter, the libxlu_cfg_?.[hc] files needs to be regenerated. (with make)
This is a different proposal to Andrew's patch:
<20190805144910.20223-1-andrew.cooper3@citrix.com>
[PATCH] tools/xl: Make extra= usable in combination with cmdline=
tools/libxl/libxlu_cfg.c | 100 +++++++++++++++++++++++++++++++++-
tools/libxl/libxlu_cfg_i.h | 1 +
tools/libxl/libxlu_cfg_l.l | 1 +
tools/libxl/libxlu_cfg_y.y | 4 +-
tools/libxl/libxlu_internal.h | 1 +
tools/libxl/libxlutil.h | 5 ++
6 files changed, 109 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index 5838f6885e..72815d25dd 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -276,6 +276,14 @@ int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
char *ep;
e= find_atom(cfg,n,&set,dont_warn); if (e) return e;
+ if (set->op == XLU_OP_ADDITION) {
+ if (!dont_warn)
+ fprintf(cfg->report,
+ "%s:%d: warning: can't use += with numbers"
+ " for parameter `%s'\n",
+ cfg->config_source, set->lineno, n);
+ return EINVAL;
+ }
errno= 0; l= strtol(set->value->u.string, &ep, 0);
e= errno;
if (errno) {
@@ -450,23 +458,111 @@ void xlu__cfg_list_append(CfgParseContext *ctx,
list->u.list.nvalues++;
}
+static int xlu__cfg_concat_vals(CfgParseContext *ctx,
+ XLU_ConfigValue *prev,
+ XLU_ConfigValue *to_add)
+{
+ int r;
+
+ if (prev->type != to_add->type) {
+ xlu__cfgl_lexicalerror(ctx,
+ "can't add [list] to \"string\" or vice versa");
+ return EINVAL;
+ }
+
+ switch (to_add->type) {
+ case XLU_STRING: {
+ char *new_string = NULL;
+
+ r = asprintf(&new_string, "%s%s", prev->u.string,
+ to_add->u.string);
+ if (r < 0) {
+ return errno;
+ }
+ free(to_add->u.string);
+ to_add->u.string = new_string;
+ return 0;
+ }
+ case XLU_LIST: {
+ XLU_ConfigList *const prev_list = &prev->u.list;
+ XLU_ConfigList *const cur_list = &to_add->u.list;
+ int nvalues;
+
+ if (prev->u.list.nvalues > INT_MAX - to_add->u.list.nvalues) {
+ return ERANGE;
+ }
+ nvalues = prev->u.list.nvalues + to_add->u.list.nvalues;
+
+ if (nvalues >= cur_list->avalues) {
+ XLU_ConfigValue **new_vals;
+ new_vals = realloc(cur_list->values,
+ nvalues * sizeof(*new_vals));
+ if (!new_vals) {
+ return ENOMEM;
+ }
+ cur_list->avalues = nvalues;
+ cur_list->values = new_vals;
+ }
+
+ /* make space for `prev' into `to_add' */
+ memmove(cur_list->values + prev_list->nvalues,
+ cur_list->values,
+ cur_list->nvalues * sizeof(XLU_ConfigValue *));
+ /* move values from `prev' to `to_add' as the list in `prev' will
+ * not be reachable by find(). */
+ memcpy(cur_list->values,
+ prev_list->values,
+ prev_list->nvalues * sizeof(XLU_ConfigValue *));
+ cur_list->nvalues = nvalues;
+ prev_list->nvalues = 0;
+ memset(prev_list->values, 0,
+ prev_list->nvalues * sizeof(XLU_ConfigValue *));
+ return 0;
+ }
+ default:
+ abort();
+ }
+ return -1;
+}
+
void xlu__cfg_set_store(CfgParseContext *ctx, char *name,
+ enum XLU_Operation op,
XLU_ConfigValue *val, int lineno) {
XLU_ConfigSetting *set;
+ int r;
- if (ctx->err) return;
+ if (ctx->err) goto out;
assert(name);
+
+ if (op == XLU_OP_ADDITION) {
+ /* If we have += concatenate with previous value with same name */
+ XLU_ConfigSetting *prev_set = find(ctx->cfg, name);
+ if (prev_set) {
+ r = xlu__cfg_concat_vals(ctx, prev_set->value, val);
+ if (r) {
+ ctx->err = r;
+ goto out;
+ }
+ }
+ }
+
set = malloc(sizeof(*set));
if (!set) {
ctx->err = errno;
- return;
+ goto out;
}
set->name= name;
set->value = val;
+ set->op = op;
set->lineno= lineno;
set->next= ctx->cfg->settings;
ctx->cfg->settings= set;
+ return;
+out:
+ assert(ctx->err);
+ free(name);
+ xlu__cfg_value_free(val);
}
char *xlu__cfgl_strdup(CfgParseContext *ctx, const char *src) {
diff --git a/tools/libxl/libxlu_cfg_i.h b/tools/libxl/libxlu_cfg_i.h
index 1b59b3312f..87b19df311 100644
--- a/tools/libxl/libxlu_cfg_i.h
+++ b/tools/libxl/libxlu_cfg_i.h
@@ -24,6 +24,7 @@
void xlu__cfg_set_free(XLU_ConfigSetting *set);
void xlu__cfg_set_store(CfgParseContext*, char *name,
+ enum XLU_Operation op,
XLU_ConfigValue *val, int lineno);
XLU_ConfigValue *xlu__cfg_string_mk(CfgParseContext *ctx,
char *atom, YYLTYPE *loc);
diff --git a/tools/libxl/libxlu_cfg_l.l b/tools/libxl/libxlu_cfg_l.l
index e0ea8cfcb3..390d6e2c2b 100644
--- a/tools/libxl/libxlu_cfg_l.l
+++ b/tools/libxl/libxlu_cfg_l.l
@@ -66,6 +66,7 @@ void xlu__cfg_yyset_column(int column_no, yyscan_t yyscanner);
, { GOT(','); }
\[ { GOT('['); }
\] { GOT(']'); }
+\+\= { GOT(OP_ADD); }
\= { GOT('='); }
\; { GOT(';'); }
diff --git a/tools/libxl/libxlu_cfg_y.y b/tools/libxl/libxlu_cfg_y.y
index a923f7672d..020fc63eb3 100644
--- a/tools/libxl/libxlu_cfg_y.y
+++ b/tools/libxl/libxlu_cfg_y.y
@@ -38,6 +38,7 @@
%token <string> IDENT STRING NUMBER NEWLINE
%type <string> atom
%destructor { free($$); } atom IDENT STRING NUMBER
+%token OP_ADD "+="
%type <value> value valuelist values
%destructor { xlu__cfg_value_free($$); } value valuelist values
@@ -54,7 +55,8 @@ stmt: assignment endstmt
| endstmt
| error NEWLINE
-assignment: IDENT '=' value { xlu__cfg_set_store(ctx,$1,$3,@3.first_line); }
+assignment: IDENT '=' value { xlu__cfg_set_store(ctx,$1,XLU_OP_ASSIGNMENT,$3,@3.first_line); }
+ | IDENT "+=" value { xlu__cfg_set_store(ctx,$1,XLU_OP_ADDITION,$3,@3.first_line); }
endstmt: NEWLINE
| ';'
diff --git a/tools/libxl/libxlu_internal.h b/tools/libxl/libxlu_internal.h
index 0acdde38f4..1f7559ecd9 100644
--- a/tools/libxl/libxlu_internal.h
+++ b/tools/libxl/libxlu_internal.h
@@ -53,6 +53,7 @@ typedef struct XLU_ConfigSetting { /* transparent */
struct XLU_ConfigSetting *next;
char *name;
XLU_ConfigValue *value;
+ enum XLU_Operation op;
int lineno;
} XLU_ConfigSetting;
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index e81b644c01..057cc25cb2 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -25,6 +25,11 @@ enum XLU_ConfigValueType {
XLU_LIST,
};
+enum XLU_Operation {
+ XLU_OP_ASSIGNMENT = 0,
+ XLU_OP_ADDITION,
+};
+
/* Unless otherwise stated, all functions return an errno value. */
typedef struct XLU_Config XLU_Config;
typedef struct XLU_ConfigList XLU_ConfigList;
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 13/08/2019 15:48, Anthony PERARD wrote: > Handle += of both strings and lists. > > If += is used for config options expected to be numbers, then a > warning is printed and the config option ignored (because xl ignores > config options with errors). > > This is to be used for development purposes, where modifying config > option can be done on the `xl create' command line. > > One could have a cmdline= in the cfg file, and specify cmdline+= on > the `xl create` command line without the need to write the whole > cmdline in `xl' command line but simply append to the one in the cfg file. > Or add an extra vif or disk by simply having "vif += [ '', ];" in the > `xl' cmdline. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Notes: > Commiter, the libxlu_cfg_?.[hc] files needs to be regenerated. (with make) > > This is a different proposal to Andrew's patch: > <20190805144910.20223-1-andrew.cooper3@citrix.com> > [PATCH] tools/xl: Make extra= usable in combination with cmdline= I gave this a spin, but got: [root@fusebot ~]# ./xlplus create shim.cfg ramdisk=\"/root/tests/selftest/test-hvm64-selftest\" cmdline+=\"dom0=pvh\ dom0-iommu=none\" Parsing config from shim.cfg shim.cfg:19: config parsing error near `+="dom0=pvh': lexical error warning: Config file looks like it contains Python code. warning: Arbitrary Python is no longer supported. warning: See http://wiki.xen.org/wiki/PythonInXlConfig Failed to parse config: Invalid argument I can't any combination of syntax which xl is happy with. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Aug 13, 2019 at 04:06:33PM +0100, Andrew Cooper wrote: > On 13/08/2019 15:48, Anthony PERARD wrote: > > Handle += of both strings and lists. > > > > If += is used for config options expected to be numbers, then a > > warning is printed and the config option ignored (because xl ignores > > config options with errors). > > > > This is to be used for development purposes, where modifying config > > option can be done on the `xl create' command line. > > > > One could have a cmdline= in the cfg file, and specify cmdline+= on > > the `xl create` command line without the need to write the whole > > cmdline in `xl' command line but simply append to the one in the cfg file. > > Or add an extra vif or disk by simply having "vif += [ '', ];" in the > > `xl' cmdline. > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > --- > > > > Notes: > > Commiter, the libxlu_cfg_?.[hc] files needs to be regenerated. (with make) > > > > This is a different proposal to Andrew's patch: > > <20190805144910.20223-1-andrew.cooper3@citrix.com> > > [PATCH] tools/xl: Make extra= usable in combination with cmdline= > > I gave this a spin, but got: > > [root@fusebot ~]# ./xlplus create shim.cfg > ramdisk=\"/root/tests/selftest/test-hvm64-selftest\" > cmdline+=\"dom0=pvh\ dom0-iommu=none\" > Parsing config from shim.cfg > shim.cfg:19: config parsing error near `+="dom0=pvh': lexical error > warning: Config file looks like it contains Python code. > warning: Arbitrary Python is no longer supported. > warning: See http://wiki.xen.org/wiki/PythonInXlConfig > Failed to parse config: Invalid argument Either older version of `flex' behave differently, or you don't have it installed on your system. `make' seems to only print a warning if `flex' is missing. Also, I've only done concatenation of string, += doesn't add a ' ' in between. So for cmdline, it would needs to be cmdline+=\"\ dom0=pvh\". Thanks, -- Anthony PERARD _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 13/08/2019 16:30, Anthony PERARD wrote: > On Tue, Aug 13, 2019 at 04:06:33PM +0100, Andrew Cooper wrote: >> On 13/08/2019 15:48, Anthony PERARD wrote: >>> Handle += of both strings and lists. >>> >>> If += is used for config options expected to be numbers, then a >>> warning is printed and the config option ignored (because xl ignores >>> config options with errors). >>> >>> This is to be used for development purposes, where modifying config >>> option can be done on the `xl create' command line. >>> >>> One could have a cmdline= in the cfg file, and specify cmdline+= on >>> the `xl create` command line without the need to write the whole >>> cmdline in `xl' command line but simply append to the one in the cfg file. >>> Or add an extra vif or disk by simply having "vif += [ '', ];" in the >>> `xl' cmdline. >>> >>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >>> --- >>> >>> Notes: >>> Commiter, the libxlu_cfg_?.[hc] files needs to be regenerated. (with make) >>> >>> This is a different proposal to Andrew's patch: >>> <20190805144910.20223-1-andrew.cooper3@citrix.com> >>> [PATCH] tools/xl: Make extra= usable in combination with cmdline= >> I gave this a spin, but got: >> >> [root@fusebot ~]# ./xlplus create shim.cfg >> ramdisk=\"/root/tests/selftest/test-hvm64-selftest\" >> cmdline+=\"dom0=pvh\ dom0-iommu=none\" >> Parsing config from shim.cfg >> shim.cfg:19: config parsing error near `+="dom0=pvh': lexical error >> warning: Config file looks like it contains Python code. >> warning: Arbitrary Python is no longer supported. >> warning: See http://wiki.xen.org/wiki/PythonInXlConfig >> Failed to parse config: Invalid argument > Either older version of `flex' behave differently, or you don't have it > installed on your system. `make' seems to only print a warning if > `flex' is missing. > > Also, I've only done concatenation of string, += doesn't add a ' ' in > between. So for cmdline, it would needs to be cmdline+=\"\ dom0=pvh\". Error between user and terminal. :) I'd sync'd xl and libxl.so, but not libxlu.so Ok, so that is working now. I think 'cmdline+=" dom0=pvh dom0-iommu=none"' is slightly less tortured syntax, but I guess there is no way that this isn't going to be horrible. As for the general mechanism, how usable is += for anything other than cmdline? Most strings in config files can't usefully be extended in this matter - if they need changing, they need changing wholesale. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Aug 13, 2019 at 04:47:23PM +0100, Andrew Cooper wrote: > Error between user and terminal. :) > > I'd sync'd xl and libxl.so, but not libxlu.so I actually made the same mistake first time I tried. > Ok, so that is working now. I think 'cmdline+=" dom0=pvh > dom0-iommu=none"' is slightly less tortured syntax, but I guess there is > no way that this isn't going to be horrible. > > As for the general mechanism, how usable is += for anything other than > cmdline? Most strings in config files can't usefully be extended in > this matter - if they need changing, they need changing wholesale. That's true, but one could imaging some maybe bad example like adding a suffix to the name of the guest: "name+='-ovmf';". Going through `man xl.cfg', maybe a good example other than cmdline could be "cpus+=',^1'" but maybe a space is fine here, or one could use a list instead. Other potential uses could be for "PATH", but in this case it would be better reset the setting rather that attempting to add a suffix to an existing one. I wonder if instead of doing += on all strings, we should instead have `xl' whitelist the few options where += would make sense. (and at that point, it would be easy to add a ' ' where is make sense, like "cmdline"s. But then, how to tell users that it can't do "name+='-new'"? because xlu would just print a warning, and xl would keep going with name="". Try "xl create memory+=42" ;-). -- Anthony PERARD _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Aug 13, 2019 at 05:42:15PM +0100, Anthony PERARD wrote: > On Tue, Aug 13, 2019 at 04:47:23PM +0100, Andrew Cooper wrote: > > Error between user and terminal. :) > > > > I'd sync'd xl and libxl.so, but not libxlu.so > > I actually made the same mistake first time I tried. > > > Ok, so that is working now. I think 'cmdline+=" dom0=pvh > > dom0-iommu=none"' is slightly less tortured syntax, but I guess there is > > no way that this isn't going to be horrible. > > > > As for the general mechanism, how usable is += for anything other than > > cmdline? Most strings in config files can't usefully be extended in > > this matter - if they need changing, they need changing wholesale. > > That's true, but one could imaging some maybe bad example like adding a > suffix to the name of the guest: "name+='-ovmf';". > Going through `man xl.cfg', maybe a good example other than cmdline > could be "cpus+=',^1'" but maybe a space is fine here, or one could use > a list instead. > Other potential uses could be for "PATH", but in this case it would be > better reset the setting rather that attempting to add a suffix to an > existing one. > > I wonder if instead of doing += on all strings, we should instead have > `xl' whitelist the few options where += would make sense. (and at that > point, it would be easy to add a ' ' where is make sense, like > "cmdline"s. But then, how to tell users that it can't do "name+='-new'"? > because xlu would just print a warning, and xl would keep going with > name="". Try "xl create memory+=42" ;-). Not sure I follow. Can you limit this in xl? Isn't += handled in libxlu already? Wei. > > -- > Anthony PERARD _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, Aug 16, 2019 at 12:47:07PM +0000, Wei Liu wrote: > On Tue, Aug 13, 2019 at 05:42:15PM +0100, Anthony PERARD wrote: > > On Tue, Aug 13, 2019 at 04:47:23PM +0100, Andrew Cooper wrote: > > > Error between user and terminal. :) > > > > > > I'd sync'd xl and libxl.so, but not libxlu.so > > > > I actually made the same mistake first time I tried. > > > > > Ok, so that is working now. I think 'cmdline+=" dom0=pvh > > > dom0-iommu=none"' is slightly less tortured syntax, but I guess there is > > > no way that this isn't going to be horrible. > > > > > > As for the general mechanism, how usable is += for anything other than > > > cmdline? Most strings in config files can't usefully be extended in > > > this matter - if they need changing, they need changing wholesale. > > > > That's true, but one could imaging some maybe bad example like adding a > > suffix to the name of the guest: "name+='-ovmf';". > > Going through `man xl.cfg', maybe a good example other than cmdline > > could be "cpus+=',^1'" but maybe a space is fine here, or one could use > > a list instead. > > Other potential uses could be for "PATH", but in this case it would be > > better reset the setting rather that attempting to add a suffix to an > > existing one. > > > > I wonder if instead of doing += on all strings, we should instead have > > `xl' whitelist the few options where += would make sense. (and at that > > point, it would be easy to add a ' ' where is make sense, like > > "cmdline"s. But then, how to tell users that it can't do "name+='-new'"? > > because xlu would just print a warning, and xl would keep going with > > name="". Try "xl create memory+=42" ;-). > > Not sure I follow. Can you limit this in xl? With a patch to xl, yes ;-). What could be done is to add a new API in libxlu. The original function get_string() could return an error if += was used on a specific setting. A new function get_appended_string() (working title), could return return a string with the += computation done. That way, xl can choose which setting are allowed to have += used (and it could even use how to do it, i.e. add spaces between strings or not). > Isn't += handled in libxlu already? It's not, this patch adds the capability into libxlu. Cheers, -- Anthony PERARD _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Anthony PERARD writes ("Re: [PATCH] libxlu: Handle += in config files"): > I wonder if instead of doing += on all strings, we should instead have > `xl' whitelist the few options where += would make sense. (and at that > point, it would be easy to add a ' ' where is make sense, like > "cmdline"s. But then, how to tell users that it can't do "name+='-new'"? > because xlu would just print a warning, and xl would keep going with > name="". Try "xl create memory+=42" ;-). Do we really need to gold-plate it like this ? If someone tries to append to a string when it doesn't make sense the software will still do what they ought to have expected. And it doesn't seem like a likely kind of error. As for the original patch, Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Ian Jackson writes ("Re: [PATCH] libxlu: Handle += in config files"): > Anthony PERARD writes ("Re: [PATCH] libxlu: Handle += in config files"): > > I wonder if instead of doing += on all strings, we should instead have > > `xl' whitelist the few options where += would make sense. (and at that > > point, it would be easy to add a ' ' where is make sense, like > > "cmdline"s. But then, how to tell users that it can't do "name+='-new'"? > > because xlu would just print a warning, and xl would keep going with > > name="". Try "xl create memory+=42" ;-). > > Do we really need to gold-plate it like this ? If someone tries to > append to a string when it doesn't make sense the software will still > do what they ought to have expected. And it doesn't seem like a > likely kind of error. > > As for the original patch, > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> I reread the thread and I think there were no blocking objections. So I have pushed it. Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.