[1/2] nbd: change option parsing scheme
diff mbox

Message ID 1475659988-20500-2-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Oct. 5, 2016, 9:33 a.m. UTC
From: Denis Plotnikov <dplotnikov@virtuozzo.com>

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 <dplotnikov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 124 insertions(+), 19 deletions(-)

Comments

Paolo Bonzini Oct. 5, 2016, 9:57 a.m. UTC | #1
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 <dplotnikov@virtuozzo.com>
> 
> 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 <dplotnikov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  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);
>  }
>

Patch
diff mbox

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"
+
+#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;
+}
+
+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;
+}
+
+
 static int nbd_parse_uri(const char *filename, QDict *options)
 {
     URI *uri;
@@ -79,18 +121,18 @@  static int nbd_parse_uri(const char *filename, QDict *options)
     }
 
     qp = query_params_parse(uri->query);
-    if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
-        ret = -EINVAL;
-        goto out;
-    }
 
     if (is_unix) {
         /* nbd+unix:///export?socket=path */
-        if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
+        if (uri->server || uri->port) {
+            ret = -EINVAL;
+            goto out;
+        }
+
+        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;
+        }
     }
 
 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++;
+        }
         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';
+        }
+
         InetSocketAddress *addr = NULL;
 
         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 = {
+            .name = "file_opts",
+            .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));
+        }
+
+        qemu_opts_del(opts);
+    }
+
 out:
     g_free(file);
 }