Re: [Qemu-block] [PATCH 1/2] nbd: change option parsing scheme

[Paolo Bonzini]

I reviewed this patch before noticing that the overall idea is not what
Kevin suggested, so I'm sending it out anyway.  Further comments from
Kevin and Max might come since I am not familiar with the current
conventions on parsing block device options.

On 05/10/2016 11:33, Denis V. Lunev wrote:
> From: Denis Plotnikov <address@hidden>
> 
> This is a preparatory commit to make code more generic. We are going to
> add more options in the next patch.
> 
> Signed-off-by: Denis Plotnikov <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Max Reitz <address@hidden>
> ---
>  block/nbd.c | 143 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 124 insertions(+), 19 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 6bc06d6..3b133ed 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -38,7 +38,9 @@
>  #include "qapi/qmp/qstring.h"
>  #include "qemu/cutils.h"
>  
> -#define EN_OPTSTR ":exportname="
> +#define EN_OPTSTR "exportname"

Please replace the #define with the "exportname" string; same for
ZI_OPTSTR in patch 2.

> +#define PATH_PARAM      (1u << 0)
>  
>  typedef struct BDRVNBDState {
>      NbdClientSession client;
> @@ -47,6 +49,46 @@ typedef struct BDRVNBDState {
>      char *path, *host, *port, *export, *tlscredsid;
>  } BDRVNBDState;
>  
> +/*
> + * helpers for dealing with option parsing
> + * to ease futher params adding and managing
> + */
> +
> +/*
> + *  @param_flags - bit flags defining a set of param names to be parsed
> + */
> +static bool parse_query_params(QueryParams *qp, QDict *options,
> +                               unsigned int param_flags)
> +{
> +    int i;
> +    for (i = 0; i < qp->n; i++) {
> +        QueryParam *param = &qp->p[i];
> +
> +        if ((PATH_PARAM & param_flags) &&
> +            strcmp(param->name, "socket") == 0) {
> +            qdict_put(options, "path", qstring_from_str(param->value));
> +            continue;
> +        }
> +
> +    }
> +    return true;
> +}

Please remove the param_flags argument.  In patch 2 you do:

        if (find_prohibited_params(qp, PATH_PARAM)) {
            ret = -EINVAL;
            goto out;
        }
        if (!parse_query_params(qp, options, ZERO_INIT_PARAM)) {
            ret = -EINVAL;
            goto out;
        }

Because you've filtered out the socket parameter, it's okay if
parse_query_params parses it always.

Also, please change nbd_parse_uri to return errors via Error**.  Then
parse_query_params can do the same, and we get better error messages.

> +
> +static bool find_prohibited_params(QueryParams *qp, unsigned int param_flags)
> +{
> +    int i;
> +    for (i = 0; i < qp->n; i++) {
> +        QueryParam *param = &qp->p[i];
> +
> +        if ((PATH_PARAM & param_flags) &&
> +            strcmp(param->name, "socket") == 0) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}

Please change this function to something like

static QueryParam *find_query_param(QueryParams *qp, const char *name)

> +        if (!parse_query_params(qp, options, PATH_PARAM)) {
>              ret = -EINVAL;
>              goto out;
>          }
> -        qdict_put(options, "path", qstring_from_str(qp->p[0].value));
>      } else {
>          QString *host;
>          /* nbd[+tcp]://host[:port]/export */
> @@ -113,6 +155,11 @@ static int nbd_parse_uri(const char *filename, QDict 
> *options)
>              qdict_put(options, "port", qstring_from_str(port_str));
>              g_free(port_str);
>          }
> +
> +        if (find_prohibited_params(qp, PATH_PARAM)) {
> +            ret = -EINVAL;
> +            goto out;
> +        }

As mentioned above, using Error** will let you return a better error
message, such as "The 'socket' parameter is only valid for NBD over Unix
domain sockets".

>      }
>  
>  out:
> @@ -127,7 +174,7 @@ static void nbd_parse_filename(const char *filename, 
> QDict *options,
>                                 Error **errp)
>  {
>      char *file;
> -    char *export_name;
> +    char *opt_str;
>      const char *host_spec;
>      const char *unixpath;
>  
> @@ -150,17 +197,6 @@ static void nbd_parse_filename(const char *filename, 
> QDict *options,
>  
>      file = g_strdup(filename);
>  
> -    export_name = strstr(file, EN_OPTSTR);
> -    if (export_name) {
> -        if (export_name[strlen(EN_OPTSTR)] == 0) {
> -            goto out;
> -        }
> -        export_name[0] = 0; /* truncate 'file' */
> -        export_name += strlen(EN_OPTSTR);
> -
> -        qdict_put(options, "export", qstring_from_str(export_name));
> -    }
> -
>      /* extract the host_spec - fail if it's not nbd:... */
>      if (!strstart(file, "nbd:", &host_spec)) {
>          error_setg(errp, "File name string for NBD must start with 'nbd:'");
> @@ -173,8 +209,40 @@ static void nbd_parse_filename(const char *filename, 
> QDict *options,
>  
>      /* are we a UNIX or TCP socket? */
>      if (strstart(host_spec, "unix:", &unixpath)) {
> +        opt_str = (char *) unixpath;
> +
> +        /* do we have any options? */
> +        /* unixpath could be unix: or unix:something:options */
> +        opt_str = strchr(opt_str, ':');
> +
> +        /* if we have any options then "divide" */
> +        /* the path and the options by replacing the last colon with "\0" */
> +        if (opt_str != NULL) {
> +            /* truncate 'unixpath' replacing the last ":" */
> +            char *colon_pos = opt_str;
> +            colon_pos[0] = '\0';
> +            opt_str++;

Just this:

if (opt_str != NULL) {
    *opt_str++ = 0;
}

> +        }
>          qdict_put(options, "path", qstring_from_str(unixpath));
>      } else {
> +        /* host_spec could be ip:port or ip:port:options */
> +        int i;
> +        opt_str = (char *)host_spec;
> +        for (i = 0; i < 2; i++) {
> +            opt_str = strchr(opt_str, ':');
> +            if (opt_str == NULL) {
> +                break;
> +            }
> +            opt_str++;
> +        }
> +
> +        /* the same idea with dividing as above */
> +        if (opt_str != NULL) {
> +            /* truncate 'host_name' replacing the last ":" */
> +            char *second_colon_pos = opt_str - 1;
> +            second_colon_pos[0] = '\0';
> +        }

Again, simpler:

opt_str = strchr((char *)host_spec, ':');
if (opt_str != NULL) {
    opt_str = strchr(opt_str + 1, ':');
    if (opt_str != NULL) {
        *opt_str++ = 0;
    }
}

>          InetSocketAddress *addr = NULL;

Please avoid declarations in the middle of statements.

>  
>          addr = inet_parse(host_spec, errp);
> @@ -187,6 +255,43 @@ static void nbd_parse_filename(const char *filename, 
> QDict *options,
>          qapi_free_InetSocketAddress(addr);
>      }
>  
> +    /* opt_str == NULL means no options given */
> +    if (opt_str != NULL) {
> +        static QemuOptsList file_opts = {

Please put this declaration outside the function, and call it nbd_opts.

> +            .name = "file_opts",

.name = "nbd",

> +            .head = QTAILQ_HEAD_INITIALIZER(file_opts.head),
> +            .desc = {
> +                {
> +                    .name = EN_OPTSTR,
> +                    .type = QEMU_OPT_STRING,
> +                    .help = "Name of the NBD export to open",
> +                },
> +            },
> +        };
> +
> +        QemuOpts *opts = qemu_opts_create(&file_opts, NULL, 0, errp);
> +        if (opts == NULL) {
> +            error_setg(errp, "Can't parse file options");
> +            goto out;
> +        }
> +
> +        Error *local_err = NULL;
> +        qemu_opts_do_parse(opts, opt_str, NULL, &local_err);
> +        if (local_err) {
> +            error_setg(errp, "Can't parse file options");
> +            qemu_opts_del(opts);
> +            goto out;
> +        }
> +
> +        const char *value;
> +        value = qemu_opt_get(opts, EN_OPTSTR);
> +        if (value) {
> +            qdict_put(options, "export", qstring_from_str(value));

"export" should be changed to "exportname" in the QDict.  This would
probably simplify the code but I don't know the exact function to use.

> +        }
> +
> +        qemu_opts_del(opts);
> +    }
> +
>  out:
>      g_free(file);
>  }
>