[PATCH v2 0/3] tools/xl: small cleanup of parsing code

[PATCH v2 0/3] tools/xl: small cleanup of parsing code

[Juergen Gross]

3 small patches cleaning up the parsing code in xl a little bit.

Changes in V2:
- added patch 2
- addressed comments

Juergen Gross (3):
  tools/xl: allow split_string_into_pair() to trim values
  tools/xl: make split_string_into_pair() more usable
  tools/xl: rework p9 config parsing

 tools/xl/xl_parse.c | 127 +++++++++++++++++++++-----------------------
 tools/xl/xl_parse.h |   4 +-
 2 files changed, 64 insertions(+), 67 deletions(-)

-- 
2.35.3

[PATCH v2 1/3] tools/xl: allow split_string_into_pair() to trim values

[Juergen Gross]

Most use cases of split_string_into_pair() are requiring the returned
strings to be white space trimmed.

In order to avoid the same code pattern multiple times, add a predicate
parameter to split_string_into_pair() which can be specified to call
trim() with that predicate for the string pair returned. Specifying
NULL for the predicate will avoid the call of trim().

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/xl/xl_parse.c | 42 +++++++++++++++++++-----------------------
 tools/xl/xl_parse.h |  4 ++--
 2 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e344d4fda3..0e1b6907fa 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -646,7 +646,7 @@ static void parse_vnuma_config(const XLU_Config *config,
              conf_count++) {
 
             if (xlu_cfg_value_type(conf_option) == XLU_STRING) {
-                char *buf, *option_untrimmed, *value_untrimmed;
+                char *buf;
                 char *option, *value;
                 unsigned long val;
 
@@ -654,15 +654,12 @@ static void parse_vnuma_config(const XLU_Config *config,
 
                 if (!buf) continue;
 
-                if (split_string_into_pair(buf, "=",
-                                           &option_untrimmed,
-                                           &value_untrimmed)) {
+                if (split_string_into_pair(buf, "=", &option, &value,
+                                           isspace)) {
                     fprintf(stderr, "xl: failed to split \"%s\" into pair\n",
                             buf);
                     exit(EXIT_FAILURE);
                 }
-                trim(isspace, option_untrimmed, &option);
-                trim(isspace, value_untrimmed, &value);
 
                 if (!strcmp("pnode", option)) {
                     val = parse_ulong(value);
@@ -715,8 +712,6 @@ static void parse_vnuma_config(const XLU_Config *config,
                 }
                 free(option);
                 free(value);
-                free(option_untrimmed);
-                free(value_untrimmed);
             }
         }
     }
@@ -838,7 +833,7 @@ int parse_vdispl_config(libxl_device_vdispl *vdispl, char *token)
 
             rc = split_string_into_pair(connectors[i], ":",
                                         &vdispl->connectors[i].unique_id,
-                                        &resolution);
+                                        &resolution, NULL);
 
             rc= sscanf(resolution, "%ux%u", &vdispl->connectors[i].width,
                        &vdispl->connectors[i].height);
@@ -2361,18 +2356,15 @@ void parse_config_data(const char *config_source,
             split_string_into_string_list(buf, ",", &pairs);
             len = libxl_string_list_length(&pairs);
             for (i = 0; i < len; i++) {
-                char *key, *key_untrimmed, *value, *value_untrimmed;
+                char *key, *value;
                 int rc;
-                rc = split_string_into_pair(pairs[i], "=",
-                                            &key_untrimmed,
-                                            &value_untrimmed);
+                rc = split_string_into_pair(pairs[i], "=", &key, &value,
+                                            isspace);
                 if (rc != 0) {
                     fprintf(stderr, "failed to parse channel configuration: %s",
                             pairs[i]);
                     exit(1);
                 }
-                trim(isspace, key_untrimmed, &key);
-                trim(isspace, value_untrimmed, &value);
 
                 if (!strcmp(key, "backend")) {
                     replace_string(&chn->backend_domname, value);
@@ -2395,9 +2387,7 @@ void parse_config_data(const char *config_source,
                                   " ignoring\n", key);
                 }
                 free(key);
-                free(key_untrimmed);
                 free(value);
-                free(value_untrimmed);
             }
             switch (chn->connection) {
             case LIBXL_CHANNEL_CONNECTION_UNKNOWN:
@@ -3021,10 +3011,8 @@ void trim(char_predicate_t predicate, const char *input, char **output)
     *output = result;
 }
 
-int split_string_into_pair(const char *str,
-                           const char *delim,
-                           char **a,
-                           char **b)
+int split_string_into_pair(const char *str, const char *delim,
+                           char **a, char **b, char_predicate_t predicate)
 {
     char *s, *p, *saveptr, *aa = NULL, *bb = NULL;
     int rc = 0;
@@ -3036,13 +3024,21 @@ int split_string_into_pair(const char *str,
         rc = ERROR_INVAL;
         goto out;
     }
-    aa = xstrdup(p);
+    if (predicate) {
+        trim(predicate, p, &aa);
+    } else {
+        aa = xstrdup(p);
+    }
     p = strtok_r(NULL, delim, &saveptr);
     if (p == NULL) {
         rc = ERROR_INVAL;
         goto out;
     }
-    bb = xstrdup(p);
+    if (predicate) {
+        trim(predicate, p, &bb);
+    } else {
+        bb = xstrdup(p);
+    }
 
     *a = aa;
     aa = NULL;
diff --git a/tools/xl/xl_parse.h b/tools/xl/xl_parse.h
index bab2861f8c..ab35c68545 100644
--- a/tools/xl/xl_parse.h
+++ b/tools/xl/xl_parse.h
@@ -45,14 +45,14 @@ int match_option_size(const char *prefix, size_t len,
 
 void split_string_into_string_list(const char *str, const char *delim,
                                    libxl_string_list *psl);
-int split_string_into_pair(const char *str, const char *delim,
-                           char **a, char **b);
 void replace_string(char **str, const char *val);
 
 /* NB: this follows the interface used by <ctype.h>. See 'man 3 ctype'
    and look for CTYPE in libxl_internal.h */
 typedef int (*char_predicate_t)(const int c);
 void trim(char_predicate_t predicate, const char *input, char **output);
+int split_string_into_pair(const char *str, const char *delim,
+                           char **a, char **b, char_predicate_t predicate);
 
 const char *get_action_on_shutdown_name(libxl_action_on_shutdown a);
 
-- 
2.35.3

[PATCH v2 2/3] tools/xl: make split_string_into_pair() more usable

[Juergen Gross]

Today split_string_into_pair() will not really do what its name is
suggesting: instead of splitting a string into a pair of strings using
a delimiter, it will return the first two strings of the initial string
by using the delimiter.

This is never what the callers want, so modify split_string_into_pair()
to split the string only at the first delimiter found, resulting in
something like "x=a=b" to be split into "x" and "a=b" when being called
with "=" as the delimiter. Today the returned strings would be "x" and
"a".

At the same time switch the delimiter from "const char *" (allowing
multiple delimiter characters) to "char" (a single character only), as
this makes the function more simple without breaking any use cases.

Suggested-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 tools/xl/xl_parse.c | 23 ++++++++++++-----------
 tools/xl/xl_parse.h |  4 ++--
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 0e1b6907fa..09cabd2732 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -654,7 +654,7 @@ static void parse_vnuma_config(const XLU_Config *config,
 
                 if (!buf) continue;
 
-                if (split_string_into_pair(buf, "=", &option, &value,
+                if (split_string_into_pair(buf, '=', &option, &value,
                                            isspace)) {
                     fprintf(stderr, "xl: failed to split \"%s\" into pair\n",
                             buf);
@@ -831,7 +831,7 @@ int parse_vdispl_config(libxl_device_vdispl *vdispl, char *token)
         {
             char *resolution;
 
-            rc = split_string_into_pair(connectors[i], ":",
+            rc = split_string_into_pair(connectors[i], ':',
                                         &vdispl->connectors[i].unique_id,
                                         &resolution, NULL);
 
@@ -2358,7 +2358,7 @@ void parse_config_data(const char *config_source,
             for (i = 0; i < len; i++) {
                 char *key, *value;
                 int rc;
-                rc = split_string_into_pair(pairs[i], "=", &key, &value,
+                rc = split_string_into_pair(pairs[i], '=', &key, &value,
                                             isspace);
                 if (rc != 0) {
                     fprintf(stderr, "failed to parse channel configuration: %s",
@@ -3011,26 +3011,27 @@ void trim(char_predicate_t predicate, const char *input, char **output)
     *output = result;
 }
 
-int split_string_into_pair(const char *str, const char *delim,
-                           char **a, char **b, char_predicate_t predicate)
+int split_string_into_pair(const char *str, char delim, char **a, char **b,
+                           char_predicate_t predicate)
 {
-    char *s, *p, *saveptr, *aa = NULL, *bb = NULL;
+    char *s, *p, *aa = NULL, *bb = NULL;
     int rc = 0;
 
     s = xstrdup(str);
 
-    p = strtok_r(s, delim, &saveptr);
+    p = strchr(s, delim);
     if (p == NULL) {
         rc = ERROR_INVAL;
         goto out;
     }
+    *p = 0;
     if (predicate) {
-        trim(predicate, p, &aa);
+        trim(predicate, s, &aa);
     } else {
-        aa = xstrdup(p);
+        aa = xstrdup(s);
     }
-    p = strtok_r(NULL, delim, &saveptr);
-    if (p == NULL) {
+    p++;
+    if (!*p) {
         rc = ERROR_INVAL;
         goto out;
     }
diff --git a/tools/xl/xl_parse.h b/tools/xl/xl_parse.h
index ab35c68545..fe0d586cdd 100644
--- a/tools/xl/xl_parse.h
+++ b/tools/xl/xl_parse.h
@@ -51,8 +51,8 @@ void replace_string(char **str, const char *val);
    and look for CTYPE in libxl_internal.h */
 typedef int (*char_predicate_t)(const int c);
 void trim(char_predicate_t predicate, const char *input, char **output);
-int split_string_into_pair(const char *str, const char *delim,
-                           char **a, char **b, char_predicate_t predicate);
+int split_string_into_pair(const char *str, char delim, char **a, char **b,
+                           char_predicate_t predicate);
 
 const char *get_action_on_shutdown_name(libxl_action_on_shutdown a);
 
-- 
2.35.3

Re: [PATCH v2 2/3] tools/xl: make split_string_into_pair() more usable

[Jason Andryuk]

On Wed, Mar 22, 2023 at 3:35 AM Juergen Gross <jgross@suse.com> wrote:
>
> Today split_string_into_pair() will not really do what its name is
> suggesting: instead of splitting a string into a pair of strings using
> a delimiter, it will return the first two strings of the initial string
> by using the delimiter.
>
> This is never what the callers want, so modify split_string_into_pair()
> to split the string only at the first delimiter found, resulting in
> something like "x=a=b" to be split into "x" and "a=b" when being called
> with "=" as the delimiter. Today the returned strings would be "x" and
> "a".
>
> At the same time switch the delimiter from "const char *" (allowing
> multiple delimiter characters) to "char" (a single character only), as
> this makes the function more simple without breaking any use cases.
>
> Suggested-by: Anthony PERARD <anthony.perard@citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Re: [PATCH v2 2/3] tools/xl: make split_string_into_pair() more usable

[Anthony PERARD]

On Wed, Mar 22, 2023 at 08:29:51AM -0400, Jason Andryuk wrote:
> On Wed, Mar 22, 2023 at 3:35 AM Juergen Gross <jgross@suse.com> wrote:
> >
> > Today split_string_into_pair() will not really do what its name is
> > suggesting: instead of splitting a string into a pair of strings using
> > a delimiter, it will return the first two strings of the initial string
> > by using the delimiter.
> >
> > This is never what the callers want, so modify split_string_into_pair()
> > to split the string only at the first delimiter found, resulting in
> > something like "x=a=b" to be split into "x" and "a=b" when being called
> > with "=" as the delimiter. Today the returned strings would be "x" and
> > "a".
> >
> > At the same time switch the delimiter from "const char *" (allowing
> > multiple delimiter characters) to "char" (a single character only), as
> > this makes the function more simple without breaking any use cases.
> >
> > Suggested-by: Anthony PERARD <anthony.perard@citrix.com>
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[PATCH v2 3/3] tools/xl: rework p9 config parsing

[Juergen Gross]

Rework the config parsing of a p9 device to use the
split_string_into_pair() function instead of open coding it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- add libxl_string_list_dispose() call (Jason Andryuk)
---
 tools/xl/xl_parse.c | 74 ++++++++++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 09cabd2732..1f6f47daf4 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2180,54 +2180,54 @@ void parse_config_data(const char *config_source,
 
     if (!xlu_cfg_get_list(config, "p9", &p9devs, 0, 0)) {
         libxl_device_p9 *p9;
-        char *security_model = NULL;
-        char *path = NULL;
-        char *tag = NULL;
-        char *backend = NULL;
-        char *p, *p2, *buf2;
 
         d_config->num_p9s = 0;
         d_config->p9s = NULL;
         while ((buf = xlu_cfg_get_listitem (p9devs, d_config->num_p9s)) != NULL) {
+            libxl_string_list pairs;
+            int len;
+
             p9 = ARRAY_EXTEND_INIT(d_config->p9s,
                                    d_config->num_p9s,
                                    libxl_device_p9_init);
             libxl_device_p9_init(p9);
 
-            buf2 = strdup(buf);
-            p = strtok(buf2, ",");
-            if(p) {
-               do {
-                  while(*p == ' ')
-                     ++p;
-                  if ((p2 = strchr(p, '=')) == NULL)
-                     break;
-                  *p2 = '\0';
-                  if (!strcmp(p, "security_model")) {
-                     security_model = strdup(p2 + 1);
-                  } else if(!strcmp(p, "path")) {
-                     path = strdup(p2 + 1);
-                  } else if(!strcmp(p, "tag")) {
-                     tag = strdup(p2 + 1);
-                  } else if(!strcmp(p, "backend")) {
-                     backend = strdup(p2 + 1);
-                  } else {
-                     fprintf(stderr, "Unknown string `%s' in 9pfs spec\n", p);
-                     exit(1);
-                  }
-               } while ((p = strtok(NULL, ",")) != NULL);
-            }
-            if (!path || !security_model || !tag) {
-               fprintf(stderr, "9pfs spec missing required field!\n");
-               exit(1);
+            split_string_into_string_list(buf, ",", &pairs);
+            len = libxl_string_list_length(&pairs);
+            for (i = 0; i < len; i++) {
+                char *key, *value;
+                int rc;
+
+                rc = split_string_into_pair(pairs[i], '=', &key, &value,
+                                            isspace);
+                if (rc != 0) {
+                    fprintf(stderr, "failed to parse 9pfs configuration: %s",
+                            pairs[i]);
+                    exit(1);
+                }
+
+                if (!strcmp(key, "security_model")) {
+                    replace_string(&p9->security_model, value);
+                } else if (!strcmp(key, "path")) {
+                    replace_string(&p9->path, value);
+                } else if (!strcmp(key, "tag")) {
+                    replace_string(&p9->tag, value);
+                } else if (!strcmp(key, "backend")) {
+                    replace_string(&p9->backend_domname, value);
+                } else {
+                    fprintf(stderr, "Unknown 9pfs parameter '%s'\n", key);
+                    exit(1);
+                }
+                free(key);
+                free(value);
             }
-            free(buf2);
 
-            replace_string(&p9->tag, tag);
-            replace_string(&p9->security_model, security_model);
-            replace_string(&p9->path, path);
-            if (backend)
-                    replace_string(&p9->backend_domname, backend);
+            libxl_string_list_dispose(&pairs);
+
+            if (!p9->path || !p9->security_model || !p9->tag) {
+                fprintf(stderr, "9pfs spec missing required field!\n");
+                exit(1);
+            }
         }
     }
 
-- 
2.35.3

Re: [PATCH v2 3/3] tools/xl: rework p9 config parsing

[Jason Andryuk]

On Wed, Mar 22, 2023 at 3:35 AM Juergen Gross <jgross@suse.com> wrote:
>
> Rework the config parsing of a p9 device to use the
> split_string_into_pair() function instead of open coding it.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Re: [PATCH v2 3/3] tools/xl: rework p9 config parsing

[Anthony PERARD]

On Wed, Mar 22, 2023 at 08:32:48AM -0400, Jason Andryuk wrote:
> On Wed, Mar 22, 2023 at 3:35 AM Juergen Gross <jgross@suse.com> wrote:
> >
> > Rework the config parsing of a p9 device to use the
> > split_string_into_pair() function instead of open coding it.
> >
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD