xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] tools/xl: small cleanup of parsing code
@ 2023-03-17 11:15 Juergen Gross
  2023-03-17 11:15 ` [PATCH 1/2] tools/xl: allow split_string_into_pair() to trim values Juergen Gross
  2023-03-17 11:15 ` [PATCH 2/2] tools/xl: rework p9 config parsing Juergen Gross
  0 siblings, 2 replies; 9+ messages in thread
From: Juergen Gross @ 2023-03-17 11:15 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Wei Liu, Anthony PERARD

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

Juergen Gross (2):
  tools/xl: allow split_string_into_pair() to trim values
  tools/xl: rework p9 config parsing

 tools/xl/xl_parse.c | 114 +++++++++++++++++++++-----------------------
 tools/xl/xl_parse.h |   4 +-
 2 files changed, 56 insertions(+), 62 deletions(-)

-- 
2.35.3



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] tools/xl: allow split_string_into_pair() to trim values
  2023-03-17 11:15 [PATCH 0/2] tools/xl: small cleanup of parsing code Juergen Gross
@ 2023-03-17 11:15 ` Juergen Gross
  2023-03-20 17:05   ` Jason Andryuk
  2023-03-17 11:15 ` [PATCH 2/2] tools/xl: rework p9 config parsing Juergen Gross
  1 sibling, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2023-03-17 11:15 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Wei Liu, Anthony PERARD

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>
---
 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 853e9f357a..2f9dfea05c 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);
@@ -2292,18 +2287,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);
@@ -2326,9 +2318,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:
@@ -2952,10 +2942,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;
@@ -2967,13 +2955,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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] tools/xl: rework p9 config parsing
  2023-03-17 11:15 [PATCH 0/2] tools/xl: small cleanup of parsing code Juergen Gross
  2023-03-17 11:15 ` [PATCH 1/2] tools/xl: allow split_string_into_pair() to trim values Juergen Gross
@ 2023-03-17 11:15 ` Juergen Gross
  2023-03-20 17:12   ` Jason Andryuk
  2023-03-21 10:44   ` Anthony PERARD
  1 sibling, 2 replies; 9+ messages in thread
From: Juergen Gross @ 2023-03-17 11:15 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Wei Liu, Anthony PERARD

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>
---
 tools/xl/xl_parse.c | 72 ++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 2f9dfea05c..715e14f95f 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2111,54 +2111,52 @@ 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);
+            if (!p9->path || !p9->security_model || !p9->tag) {
+                fprintf(stderr, "9pfs spec missing required field!\n");
+                exit(1);
+            }
         }
     }
 
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] tools/xl: allow split_string_into_pair() to trim values
  2023-03-17 11:15 ` [PATCH 1/2] tools/xl: allow split_string_into_pair() to trim values Juergen Gross
@ 2023-03-20 17:05   ` Jason Andryuk
  2023-03-21 10:24     ` Anthony PERARD
  0 siblings, 1 reply; 9+ messages in thread
From: Jason Andryuk @ 2023-03-20 17:05 UTC (permalink / raw)
  To: Juergen Gross; +Cc: xen-devel, Wei Liu, Anthony PERARD

On Fri, Mar 17, 2023 at 7:16 AM Juergen Gross <jgross@suse.com> wrote:
>
> 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>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] tools/xl: rework p9 config parsing
  2023-03-17 11:15 ` [PATCH 2/2] tools/xl: rework p9 config parsing Juergen Gross
@ 2023-03-20 17:12   ` Jason Andryuk
  2023-03-21  5:45     ` Juergen Gross
  2023-03-21 10:44   ` Anthony PERARD
  1 sibling, 1 reply; 9+ messages in thread
From: Jason Andryuk @ 2023-03-20 17:12 UTC (permalink / raw)
  To: Juergen Gross; +Cc: xen-devel, Wei Liu, Anthony PERARD

On Fri, Mar 17, 2023 at 7:16 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>
> ---
>  tools/xl/xl_parse.c | 72 ++++++++++++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 37 deletions(-)
>
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 2f9dfea05c..715e14f95f 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2111,54 +2111,52 @@ 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);

I think you need libxl_string_list_dispose(&pairs); somewhere around here?

The rest looks good.

Regards,
Jason


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] tools/xl: rework p9 config parsing
  2023-03-20 17:12   ` Jason Andryuk
@ 2023-03-21  5:45     ` Juergen Gross
  0 siblings, 0 replies; 9+ messages in thread
From: Juergen Gross @ 2023-03-21  5:45 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Wei Liu, Anthony PERARD


[-- Attachment #1.1.1: Type: text/plain, Size: 4104 bytes --]

On 20.03.23 18:12, Jason Andryuk wrote:
> On Fri, Mar 17, 2023 at 7:16 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>
>> ---
>>   tools/xl/xl_parse.c | 72 ++++++++++++++++++++++-----------------------
>>   1 file changed, 35 insertions(+), 37 deletions(-)
>>
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 2f9dfea05c..715e14f95f 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -2111,54 +2111,52 @@ 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);
> 
> I think you need libxl_string_list_dispose(&pairs); somewhere around here?

Ah yes, thanks for noticing.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] tools/xl: allow split_string_into_pair() to trim values
  2023-03-20 17:05   ` Jason Andryuk
@ 2023-03-21 10:24     ` Anthony PERARD
  0 siblings, 0 replies; 9+ messages in thread
From: Anthony PERARD @ 2023-03-21 10:24 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: Juergen Gross, xen-devel, Wei Liu

On Mon, Mar 20, 2023 at 01:05:13PM -0400, Jason Andryuk wrote:
> On Fri, Mar 17, 2023 at 7:16 AM Juergen Gross <jgross@suse.com> wrote:
> >
> > 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>

Thanks,

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] tools/xl: rework p9 config parsing
  2023-03-17 11:15 ` [PATCH 2/2] tools/xl: rework p9 config parsing Juergen Gross
  2023-03-20 17:12   ` Jason Andryuk
@ 2023-03-21 10:44   ` Anthony PERARD
  2023-03-21 11:17     ` Juergen Gross
  1 sibling, 1 reply; 9+ messages in thread
From: Anthony PERARD @ 2023-03-21 10:44 UTC (permalink / raw)
  To: Juergen Gross; +Cc: xen-devel, Wei Liu

On Fri, Mar 17, 2023 at 12:15:46PM +0100, Juergen Gross wrote:
> Rework the config parsing of a p9 device to use the
> split_string_into_pair() function instead of open coding it.

But that wasn't an open codded version of split_string_into_pair(). Now
if a value contains a '=', everything after it is ignored.

split_string_into_pair() would split the string "foo=bar=void" into just
"foo" and "bar".

As the man page doesn't say that VALUE can't contains '=', this patch
looks like a regression.

I start to think that split_string_into_pair() is broken. I've notice
the same issue when reviewing the "smbios" addition, and did proposed to
"open code" split_string_into_pair(). But maybe that function needs
fixing instead.

Thanks,

-- 
Anthony PERARD


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] tools/xl: rework p9 config parsing
  2023-03-21 10:44   ` Anthony PERARD
@ 2023-03-21 11:17     ` Juergen Gross
  0 siblings, 0 replies; 9+ messages in thread
From: Juergen Gross @ 2023-03-21 11:17 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu


[-- Attachment #1.1.1: Type: text/plain, Size: 891 bytes --]

On 21.03.23 11:44, Anthony PERARD wrote:
> On Fri, Mar 17, 2023 at 12:15:46PM +0100, Juergen Gross wrote:
>> Rework the config parsing of a p9 device to use the
>> split_string_into_pair() function instead of open coding it.
> 
> But that wasn't an open codded version of split_string_into_pair(). Now
> if a value contains a '=', everything after it is ignored.
> 
> split_string_into_pair() would split the string "foo=bar=void" into just
> "foo" and "bar".
> 
> As the man page doesn't say that VALUE can't contains '=', this patch
> looks like a regression.
> 
> I start to think that split_string_into_pair() is broken. I've notice
> the same issue when reviewing the "smbios" addition, and did proposed to
> "open code" split_string_into_pair(). But maybe that function needs
> fixing instead.

Yes, I'll add a patch fixing split_string_into_pair().


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-03-21 11:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-17 11:15 [PATCH 0/2] tools/xl: small cleanup of parsing code Juergen Gross
2023-03-17 11:15 ` [PATCH 1/2] tools/xl: allow split_string_into_pair() to trim values Juergen Gross
2023-03-20 17:05   ` Jason Andryuk
2023-03-21 10:24     ` Anthony PERARD
2023-03-17 11:15 ` [PATCH 2/2] tools/xl: rework p9 config parsing Juergen Gross
2023-03-20 17:12   ` Jason Andryuk
2023-03-21  5:45     ` Juergen Gross
2023-03-21 10:44   ` Anthony PERARD
2023-03-21 11:17     ` Juergen Gross

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).