[Xen-devel] [PATCH] libxlu: Handle += in config files

Anthony PERARD posted 1 patch 4 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190813144827.6318-1-anthony.perard@citrix.com
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(-)

[Xen-devel] [PATCH] libxlu: Handle += in config files

Posted by Anthony PERARD 4 days ago
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

Re: [Xen-devel] [PATCH] libxlu: Handle += in config files

Posted by Andrew Cooper 4 days ago
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

Re: [Xen-devel] [PATCH] libxlu: Handle += in config files

Posted by Anthony PERARD 4 days ago
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

Re: [Xen-devel] [PATCH] libxlu: Handle += in config files

Posted by Andrew Cooper 4 days ago
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

Re: [Xen-devel] [PATCH] libxlu: Handle += in config files

Posted by Anthony PERARD 4 days ago
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

Re: [Xen-devel] [PATCH] libxlu: Handle += in config files

Posted by Wei Liu 1 day ago
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

Re: [Xen-devel] [PATCH] libxlu: Handle += in config files

Posted by Anthony PERARD 1 day ago
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