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 
> 
> 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 
> Signed-off-by: Denis V. Lunev 
> CC: Paolo Bonzini 
> CC: Kevin Wolf 
> CC: Max Reitz 
> ---
>  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 = >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 = >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:", _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 

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

[Denis V. Lunev]

From: Denis Plotnikov 

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 
Signed-off-by: Denis V. Lunev 
CC: Paolo Bonzini 
CC: Kevin Wolf 
CC: Max Reitz 
---
 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"
+
+#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 = >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 = >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:", _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:", )) {
+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, ':');
+