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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

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