[PATCH 00/13] RFC: use upcoming GUri for URI handling

[PATCH 00/13] RFC: use upcoming GUri for URI handling

[Marc-André Lureau]

Hi,

After years trying to add a glib API to handle URI, GLib 2.65.1 will finally
have one. As an exercice, I checked if the API fits qemu needs, and it seems to
be fine. It should be about as verbose as the current libxml based URI parser,
but the main benefit is that we will get rid of fairly complex URI
copied code in our tree.

The first few patches are code improvements mainly around g_auto, then the
patches to convert URI code over GUri. Obviously, it will take years before this
new API reaches old-stable distros. We may want to have a copy version of GUri,
instead of the current libxml copy as a fallback. Or we may want to keep both
current code and new GUri-based code side-by-side. I am more in favour of the
second approach, given that GUri is fresh, and may have subtle parsing
differences that better being spotted and fixed from unstable/newer distros
first. Maintaining the two side-by-side for some while shouldn't be a big
burdden, as they have a lot of similarities, and the code around it is pretty
stable.

thanks

Marc-André Lureau (13):
  uri: add g_auto macros for URI & QueryParams
  block/nbd: auto-ify URI parsing variables
  block/vxhs: auto-ify URI parsing variables
  block/sheepdog: auto-ify URI parsing variables
  block/ssh: auto-ify URI parsing variables
  block/nfs: auto-ify URI parsing variables
  block/gluster: auto-ify URI parsing variables
  build-sys: add HAVE_GLIB_GURI
  nbd: add GUri-based URI parsing version
  sheepdog: add GUri-based URI parsing
  nfs: add GUri-based URI parsing
  gluster: add GUri-based URI parsing
  ssh: add GUri-based URI parsing

 configure          |   7 +++
 include/qemu/uri.h |   3 +
 block/gluster.c    | 102 +++++++++++++++++++-----------
 block/nbd.c        | 109 +++++++++++++++++++++-----------
 block/nfs.c        | 126 ++++++++++++++++++++++---------------
 block/sheepdog.c   | 153 +++++++++++++++++++++++++++------------------
 block/ssh.c        |  94 +++++++++++++++++++---------
 block/vxhs.c       |  10 +--
 util/Makefile.objs |   2 +-
 9 files changed, 383 insertions(+), 223 deletions(-)

-- 
2.27.0.221.ga08a83db2b

[PATCH 01/13] uri: add g_auto macros for URI & QueryParams

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/uri.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qemu/uri.h b/include/qemu/uri.h
index d201c61260d..b246a59449b 100644
--- a/include/qemu/uri.h
+++ b/include/qemu/uri.h
@@ -105,6 +105,9 @@ struct QueryParams *query_params_new (int init_alloc);
 extern QueryParams *query_params_parse (const char *query);
 extern void query_params_free (QueryParams *ps);
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(URI, uri_free)
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QueryParams, query_params_free)
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.27.0.221.ga08a83db2b

[PATCH 02/13] block/nbd: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/nbd.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index eed160c5cda..faadcab442b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1516,10 +1516,9 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
 
 static int nbd_parse_uri(const char *filename, QDict *options)
 {
-    URI *uri;
+    g_autoptr(URI) uri = NULL;
+    g_autoptr(QueryParams) qp = NULL;
     const char *p;
-    QueryParams *qp = NULL;
-    int ret = 0;
     bool is_unix;
 
     uri = uri_parse(filename);
@@ -1535,8 +1534,7 @@ static int nbd_parse_uri(const char *filename, QDict *options)
     } else if (!g_strcmp0(uri->scheme, "nbd+unix")) {
         is_unix = true;
     } else {
-        ret = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
     p = uri->path ? uri->path : "";
@@ -1549,26 +1547,23 @@ 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;
+        return -EINVAL;
     }
 
     if (is_unix) {
         /* nbd+unix:///export?socket=path */
         if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
-            ret = -EINVAL;
-            goto out;
+            return -EINVAL;
         }
         qdict_put_str(options, "server.type", "unix");
         qdict_put_str(options, "server.path", qp->p[0].value);
     } else {
         QString *host;
-        char *port_str;
+        g_autofree char *port_str = NULL;
 
         /* nbd[+tcp]://host[:port]/export */
         if (!uri->server) {
-            ret = -EINVAL;
-            goto out;
+            return -EINVAL;
         }
 
         /* strip braces from literal IPv6 address */
@@ -1584,15 +1579,9 @@ static int nbd_parse_uri(const char *filename, QDict *options)
 
         port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
         qdict_put_str(options, "server.port", port_str);
-        g_free(port_str);
     }
 
-out:
-    if (qp) {
-        query_params_free(qp);
-    }
-    uri_free(uri);
-    return ret;
+    return 0;
 }
 
 static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
-- 
2.27.0.221.ga08a83db2b

[PATCH 03/13] block/vxhs: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/vxhs.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/block/vxhs.c b/block/vxhs.c
index d79fc97df66..5d61cfb7548 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -174,14 +174,12 @@ static QemuOptsList runtime_tcp_opts = {
  */
 static int vxhs_parse_uri(const char *filename, QDict *options)
 {
-    URI *uri = NULL;
-    char *port;
-    int ret = 0;
+    g_autoptr(URI) uri = NULL;
+    g_autofree char *port = NULL;
 
     trace_vxhs_parse_uri_filename(filename);
     uri = uri_parse(filename);
     if (!uri || !uri->server || !uri->path) {
-        uri_free(uri);
         return -EINVAL;
     }
 
@@ -190,15 +188,13 @@ static int vxhs_parse_uri(const char *filename, QDict *options)
     if (uri->port) {
         port = g_strdup_printf("%d", uri->port);
         qdict_put_str(options, VXHS_OPT_SERVER ".port", port);
-        g_free(port);
     }
 
     qdict_put_str(options, "vdisk-id", uri->path);
 
     trace_vxhs_parse_uri_hostinfo(uri->server, uri->port);
-    uri_free(uri);
 
-    return ret;
+    return 0;
 }
 
 static void vxhs_parse_filename(const char *filename, QDict *options,
-- 
2.27.0.221.ga08a83db2b

[PATCH 04/13] block/sheepdog: auto-ify URI parsing variables

[Marc-André Lureau]

Since we are going to introduce URI parsing alternative, I changed the
way SheepdogConfig takes care of host/path & URI/QueryParams lifetimes.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/sheepdog.c | 72 ++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 42 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 27a30d17f4c..3403adfc2cd 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -986,39 +986,33 @@ static bool sd_parse_snapid_or_tag(const char *str,
 }
 
 typedef struct {
-    const char *path;           /* non-null iff transport is tcp */
-    const char *host;           /* valid when transport is tcp */
+    char *path;                 /* non-null iff transport is tcp */
+    char *host;                 /* valid when transport is tcp */
     int port;                   /* valid when transport is tcp */
     char vdi[SD_MAX_VDI_LEN];
     char tag[SD_MAX_VDI_TAG_LEN];
     uint32_t snap_id;
-    /* Remainder is only for sd_config_done() */
-    URI *uri;
-    QueryParams *qp;
 } SheepdogConfig;
 
 static void sd_config_done(SheepdogConfig *cfg)
 {
-    if (cfg->qp) {
-        query_params_free(cfg->qp);
-    }
-    uri_free(cfg->uri);
+    g_clear_pointer(&cfg->host, g_free);
+    g_clear_pointer(&cfg->path, g_free);
 }
 
 static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
                          Error **errp)
 {
-    Error *err = NULL;
-    QueryParams *qp = NULL;
+    g_autoptr(QueryParams) qp = NULL;
+    g_autoptr(URI) uri = NULL;
     bool is_unix;
-    URI *uri;
 
     memset(cfg, 0, sizeof(*cfg));
 
-    cfg->uri = uri = uri_parse(filename);
+    uri = uri_parse(filename);
     if (!uri) {
-        error_setg(&err, "invalid URI '%s'", filename);
-        goto out;
+        error_setg(errp, "invalid URI '%s'", filename);
+        return;
     }
 
     /* transport */
@@ -1029,48 +1023,48 @@ static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
     } else if (!g_strcmp0(uri->scheme, "sheepdog+unix")) {
         is_unix = true;
     } else {
-        error_setg(&err, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
+        error_setg(errp, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
                    " or 'sheepdog+unix'");
-        goto out;
+        return;
     }
 
     if (uri->path == NULL || !strcmp(uri->path, "/")) {
-        error_setg(&err, "missing file path in URI");
-        goto out;
+        error_setg(errp, "missing file path in URI");
+        return;
     }
     if (g_strlcpy(cfg->vdi, uri->path + 1, SD_MAX_VDI_LEN)
         >= SD_MAX_VDI_LEN) {
-        error_setg(&err, "VDI name is too long");
-        goto out;
+        error_setg(errp, "VDI name is too long");
+        return;
     }
 
-    cfg->qp = qp = query_params_parse(uri->query);
+    qp = query_params_parse(uri->query);
 
     if (is_unix) {
         /* sheepdog+unix:///vdiname?socket=path */
         if (uri->server || uri->port) {
-            error_setg(&err, "URI scheme %s doesn't accept a server address",
+            error_setg(errp, "URI scheme %s doesn't accept a server address",
                        uri->scheme);
-            goto out;
+            return;
         }
         if (!qp->n) {
-            error_setg(&err,
+            error_setg(errp,
                        "URI scheme %s requires query parameter 'socket'",
                        uri->scheme);
-            goto out;
+            return;
         }
         if (qp->n != 1 || strcmp(qp->p[0].name, "socket")) {
-            error_setg(&err, "unexpected query parameters");
-            goto out;
+            error_setg(errp, "unexpected query parameters");
+            return;
         }
-        cfg->path = qp->p[0].value;
+        cfg->path = g_strdup(qp->p[0].value);
     } else {
         /* sheepdog[+tcp]://[host:port]/vdiname */
         if (qp->n) {
-            error_setg(&err, "unexpected query parameters");
-            goto out;
+            error_setg(errp, "unexpected query parameters");
+            return;
         }
-        cfg->host = uri->server;
+        cfg->host = g_strdup(uri->server);
         cfg->port = uri->port;
     }
 
@@ -1078,19 +1072,13 @@ static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
     if (uri->fragment) {
         if (!sd_parse_snapid_or_tag(uri->fragment,
                                     &cfg->snap_id, cfg->tag)) {
-            error_setg(&err, "'%s' is not a valid snapshot ID",
+            error_setg(errp, "'%s' is not a valid snapshot ID",
                        uri->fragment);
-            goto out;
+            return;
         }
     } else {
         cfg->snap_id = CURRENT_VDI_ID; /* search current vdi */
     }
-
-out:
-    if (err) {
-        error_propagate(errp, err);
-        sd_config_done(cfg);
-    }
 }
 
 /*
@@ -1184,7 +1172,7 @@ static void sd_parse_filename(const char *filename, QDict *options,
     }
     if (err) {
         error_propagate(errp, err);
-        return;
+        goto end;
     }
 
     if (cfg.path) {
@@ -1203,7 +1191,7 @@ static void sd_parse_filename(const char *filename, QDict *options,
         snprintf(buf, sizeof(buf), "%d", cfg.snap_id);
         qdict_set_default_str(options, "snap-id", buf);
     }
-
+end:
     sd_config_done(&cfg);
 }
 
-- 
2.27.0.221.ga08a83db2b

[PATCH 05/13] block/ssh: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/ssh.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 098dbe03c15..c8f6ad79e3c 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -180,9 +180,9 @@ static void sftp_error_trace(BDRVSSHState *s, const char *op)
 
 static int parse_uri(const char *filename, QDict *options, Error **errp)
 {
-    URI *uri = NULL;
-    QueryParams *qp;
-    char *port_str;
+    g_autoptr(URI) uri = NULL;
+    g_autoptr(QueryParams) qp = NULL;
+    g_autofree char *port_str = NULL;
     int i;
 
     uri = uri_parse(filename);
@@ -192,23 +192,23 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
 
     if (g_strcmp0(uri->scheme, "ssh") != 0) {
         error_setg(errp, "URI scheme must be 'ssh'");
-        goto err;
+        return -EINVAL;
     }
 
     if (!uri->server || strcmp(uri->server, "") == 0) {
         error_setg(errp, "missing hostname in URI");
-        goto err;
+        return -EINVAL;
     }
 
     if (!uri->path || strcmp(uri->path, "") == 0) {
         error_setg(errp, "missing remote path in URI");
-        goto err;
+        return -EINVAL;
     }
 
     qp = query_params_parse(uri->query);
     if (!qp) {
         error_setg(errp, "could not parse query parameters");
-        goto err;
+        return -EINVAL;
     }
 
     if(uri->user && strcmp(uri->user, "") != 0) {
@@ -219,7 +219,6 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
 
     port_str = g_strdup_printf("%d", uri->port ?: 22);
     qdict_put_str(options, "server.port", port_str);
-    g_free(port_str);
 
     qdict_put_str(options, "path", uri->path);
 
@@ -232,15 +231,7 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
         }
     }
 
-    query_params_free(qp);
-    uri_free(uri);
     return 0;
-
- err:
-    if (uri) {
-      uri_free(uri);
-    }
-    return -EINVAL;
 }
 
 static bool ssh_has_filename_options_conflict(QDict *options, Error **errp)
-- 
2.27.0.221.ga08a83db2b

Re: [PATCH 05/13] block/ssh: auto-ify URI parsing variables

[Richard W.M. Jones]

On Thu, Jul 09, 2020 at 11:42:26PM +0400, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  block/ssh.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 098dbe03c15..c8f6ad79e3c 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -180,9 +180,9 @@ static void sftp_error_trace(BDRVSSHState *s, const char *op)
>  
>  static int parse_uri(const char *filename, QDict *options, Error **errp)
>  {
> -    URI *uri = NULL;
> -    QueryParams *qp;
> -    char *port_str;
> +    g_autoptr(URI) uri = NULL;
> +    g_autoptr(QueryParams) qp = NULL;
> +    g_autofree char *port_str = NULL;
>      int i;
>  
>      uri = uri_parse(filename);
> @@ -192,23 +192,23 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
>  
>      if (g_strcmp0(uri->scheme, "ssh") != 0) {
>          error_setg(errp, "URI scheme must be 'ssh'");
> -        goto err;
> +        return -EINVAL;
>      }
>  
>      if (!uri->server || strcmp(uri->server, "") == 0) {
>          error_setg(errp, "missing hostname in URI");
> -        goto err;
> +        return -EINVAL;
>      }
>  
>      if (!uri->path || strcmp(uri->path, "") == 0) {
>          error_setg(errp, "missing remote path in URI");
> -        goto err;
> +        return -EINVAL;
>      }
>  
>      qp = query_params_parse(uri->query);
>      if (!qp) {
>          error_setg(errp, "could not parse query parameters");
> -        goto err;
> +        return -EINVAL;
>      }
>  
>      if(uri->user && strcmp(uri->user, "") != 0) {
> @@ -219,7 +219,6 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
>  
>      port_str = g_strdup_printf("%d", uri->port ?: 22);
>      qdict_put_str(options, "server.port", port_str);
> -    g_free(port_str);
>  
>      qdict_put_str(options, "path", uri->path);
>  
> @@ -232,15 +231,7 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
>          }
>      }
>  
> -    query_params_free(qp);
> -    uri_free(uri);
>      return 0;
> -
> - err:
> -    if (uri) {
> -      uri_free(uri);
> -    }
> -    return -EINVAL;
>  }
>  

I had to look up the definition of g_autoptr, and it seems fine
since there's a corresponding URI macro added in the first commit.

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

[PATCH 06/13] block/nfs: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/nfs.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index b1718d125a4..93d719551d2 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -77,34 +77,34 @@ typedef struct NFSRPC {
 
 static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
 {
-    URI *uri = NULL;
-    QueryParams *qp = NULL;
-    int ret = -EINVAL, i;
+    g_autoptr(URI) uri = NULL;
+    g_autoptr(QueryParams) qp = NULL;
+    int i;
 
     uri = uri_parse(filename);
     if (!uri) {
         error_setg(errp, "Invalid URI specified");
-        goto out;
+        return -EINVAL;
     }
     if (g_strcmp0(uri->scheme, "nfs") != 0) {
         error_setg(errp, "URI scheme must be 'nfs'");
-        goto out;
+        return -EINVAL;
     }
 
     if (!uri->server) {
         error_setg(errp, "missing hostname in URI");
-        goto out;
+        return -EINVAL;
     }
 
     if (!uri->path) {
         error_setg(errp, "missing file path in URI");
-        goto out;
+        return -EINVAL;
     }
 
     qp = query_params_parse(uri->query);
     if (!qp) {
         error_setg(errp, "could not parse query parameters");
-        goto out;
+        return -EINVAL;
     }
 
     qdict_put_str(options, "server.host", uri->server);
@@ -116,12 +116,12 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
         if (!qp->p[i].value) {
             error_setg(errp, "Value for NFS parameter expected: %s",
                        qp->p[i].name);
-            goto out;
+            return -EINVAL;
         }
         if (parse_uint_full(qp->p[i].value, &val, 0)) {
             error_setg(errp, "Illegal value for NFS parameter: %s",
                        qp->p[i].name);
-            goto out;
+            return -EINVAL;
         }
         if (!strcmp(qp->p[i].name, "uid")) {
             qdict_put_str(options, "user", qp->p[i].value);
@@ -138,18 +138,10 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
         } else {
             error_setg(errp, "Unknown NFS parameter name: %s",
                        qp->p[i].name);
-            goto out;
+            return -EINVAL;
         }
     }
-    ret = 0;
-out:
-    if (qp) {
-        query_params_free(qp);
-    }
-    if (uri) {
-        uri_free(uri);
-    }
-    return ret;
+    return 0;
 }
 
 static bool nfs_has_filename_options_conflict(QDict *options, Error **errp)
-- 
2.27.0.221.ga08a83db2b

[PATCH 07/13] block/gluster: auto-ify URI parsing variables

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/gluster.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 31233cac696..c06eca1c12f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -349,10 +349,10 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
                                   const char *filename)
 {
     SocketAddress *gsconf;
-    URI *uri;
-    QueryParams *qp = NULL;
+    g_autoptr(URI) uri = NULL;
+    g_autoptr(QueryParams) qp = NULL;
     bool is_unix = false;
-    int ret = 0;
+    int ret;
 
     uri = uri_parse(filename);
     if (!uri) {
@@ -374,29 +374,25 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
         gsconf->type = SOCKET_ADDRESS_TYPE_INET;
         warn_report("rdma feature is not supported, falling back to tcp");
     } else {
-        ret = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
     ret = parse_volume_options(gconf, uri->path);
     if (ret < 0) {
-        goto out;
+        return ret;
     }
 
     qp = query_params_parse(uri->query);
     if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
-        ret = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
     if (is_unix) {
         if (uri->server || uri->port) {
-            ret = -EINVAL;
-            goto out;
+            return -EINVAL;
         }
         if (strcmp(qp->p[0].name, "socket")) {
-            ret = -EINVAL;
-            goto out;
+            return -EINVAL;
         }
         gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
     } else {
@@ -408,12 +404,7 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
         }
     }
 
-out:
-    if (qp) {
-        query_params_free(qp);
-    }
-    uri_free(uri);
-    return ret;
+    return 0;
 }
 
 static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
-- 
2.27.0.221.ga08a83db2b

[PATCH 08/13] build-sys: add HAVE_GLIB_GURI

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/configure b/configure
index ee6c3c6792a..cd2fc120aed 100755
--- a/configure
+++ b/configure
@@ -3924,6 +3924,10 @@ if $pkg_config --atleast-version=$glib_req_ver gio-unix-2.0; then
     gio_libs="$gio_libs $($pkg_config --libs gio-unix-2.0)"
 fi
 
+if $pkg_config --atleast-version=2.65.0 glib-2.0; then
+    glib_guri=yes
+fi
+
 # Sanity check that the current size_t matches the
 # size that glib thinks it should be. This catches
 # problems on multi-arch where people try to build
@@ -7377,6 +7381,9 @@ if test "$gio" = "yes" ; then
     echo "GIO_LIBS=$gio_libs" >> $config_host_mak
     echo "GDBUS_CODEGEN=$gdbus_codegen" >> $config_host_mak
 fi
+if test "$glib_guri" = "yes" ; then
+    echo "HAVE_GLIB_GURI=y" >> $config_host_mak
+fi
 echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak
 if test "$gnutls" = "yes" ; then
   echo "CONFIG_GNUTLS=y" >> $config_host_mak
-- 
2.27.0.221.ga08a83db2b

[PATCH 09/13] nbd: add GUri-based URI parsing version

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/nbd.c        | 86 +++++++++++++++++++++++++++++++++++-----------
 util/Makefile.objs |  2 +-
 2 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index faadcab442b..fdc4a53a98f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -31,7 +31,10 @@
 #include "qemu/osdep.h"
 
 #include "trace.h"
+#ifndef HAVE_GLIB_GURI
 #include "qemu/uri.h"
+#endif
+#include "qemu/error-report.h"
 #include "qemu/option.h"
 #include "qemu/cutils.h"
 #include "qemu/main-loop.h"
@@ -1513,71 +1516,112 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
 /*
  * Parse nbd_open options
  */
-
 static int nbd_parse_uri(const char *filename, QDict *options)
 {
+    const char *p, *scheme, *server, *socket = NULL;
+    int port;
+    bool is_unix;
+
+#ifdef HAVE_GLIB_GURI
+    g_autoptr(GUri) uri = NULL;
+    g_autoptr(GHashTable) params = NULL;
+    g_autoptr(GError) err = NULL;
+
+    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
+    if (!uri) {
+        error_report("Failed to parse NBD URI: %s", err->message);
+        return -EINVAL;
+    }
+
+    p = g_uri_get_path(uri);
+    scheme = g_uri_get_scheme(uri);
+    server = g_uri_get_host(uri);
+    port = g_uri_get_port(uri);
+#else
     g_autoptr(URI) uri = NULL;
     g_autoptr(QueryParams) qp = NULL;
-    const char *p;
-    bool is_unix;
 
     uri = uri_parse(filename);
     if (!uri) {
         return -EINVAL;
     }
 
+    p = uri->path ? uri->path : "";
+    scheme = uri->scheme;
+    server = uri->server;
+    port = uri->port;
+#endif
     /* transport */
-    if (!g_strcmp0(uri->scheme, "nbd")) {
+    if (!g_strcmp0(scheme, "nbd")) {
         is_unix = false;
-    } else if (!g_strcmp0(uri->scheme, "nbd+tcp")) {
+    } else if (!g_strcmp0(scheme, "nbd+tcp")) {
         is_unix = false;
-    } else if (!g_strcmp0(uri->scheme, "nbd+unix")) {
+    } else if (!g_strcmp0(scheme, "nbd+unix")) {
         is_unix = true;
     } else {
         return -EINVAL;
     }
-
-    p = uri->path ? uri->path : "";
-    if (p[0] == '/') {
-        p++;
+#ifdef HAVE_GLIB_GURI
+    params = g_uri_parse_params(g_uri_get_query(uri), -1,
+                                "&;", G_URI_PARAMS_NONE, &err);
+    if (err) {
+        error_report("Failed to parse NBD URI query: %s", err->message);
+        return -EINVAL;
     }
-    if (p[0]) {
-        qdict_put_str(options, "export", p);
+    if ((is_unix && g_hash_table_size(params) != 1) ||
+        (!is_unix && g_hash_table_size(params) != 0)) {
+        return -EINVAL;
     }
-
+    if (is_unix) {
+        socket = g_hash_table_lookup(params, "socket");
+    }
+#else
     qp = query_params_parse(uri->query);
     if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
         return -EINVAL;
     }
+    if (is_unix) {
+        if (!g_str_equal(qp->p[0].name, "socket")) {
+            return -EINVAL;
+        }
+        socket = qp->p[0].value;
+    }
+#endif
+    if (p[0] == '/') {
+        p++;
+    }
+    if (p[0]) {
+        qdict_put_str(options, "export", p);
+    }
 
     if (is_unix) {
         /* nbd+unix:///export?socket=path */
-        if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
+        if (server || port > 0) {
             return -EINVAL;
         }
         qdict_put_str(options, "server.type", "unix");
-        qdict_put_str(options, "server.path", qp->p[0].value);
+        qdict_put_str(options, "server.path", socket);
     } else {
         QString *host;
         g_autofree char *port_str = NULL;
 
         /* nbd[+tcp]://host[:port]/export */
-        if (!uri->server) {
+        if (!server) {
             return -EINVAL;
         }
 
         /* strip braces from literal IPv6 address */
-        if (uri->server[0] == '[') {
-            host = qstring_from_substr(uri->server, 1,
-                                       strlen(uri->server) - 1);
+        if (server[0] == '[') {
+            host = qstring_from_substr(server, 1,
+                                       strlen(server) - 1);
         } else {
-            host = qstring_from_str(uri->server);
+            host = qstring_from_str(server);
         }
 
         qdict_put_str(options, "server.type", "inet");
         qdict_put(options, "server.host", host);
 
-        port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
+        port_str = g_strdup_printf("%d", port > 0 ? port : NBD_DEFAULT_PORT);
         qdict_put_str(options, "server.port", port_str);
     }
 
diff --git a/util/Makefile.objs b/util/Makefile.objs
index cc5e37177af..5a162178ae9 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -73,7 +73,7 @@ util-obj-y += qemu-timer.o
 util-obj-y += thread-pool.o
 util-obj-y += throttle.o
 util-obj-y += timed-average.o
-util-obj-y += uri.o
+util-obj-$(call lnot,$(HAVE_GLIB_GURI)) += uri.o
 
 util-obj-$(CONFIG_LINUX) += vfio-helpers.o
 util-obj-$(CONFIG_INOTIFY1) += filemonitor-inotify.o
-- 
2.27.0.221.ga08a83db2b

Re: [PATCH 09/13] nbd: add GUri-based URI parsing version

[Daniel P. Berrangé]

On Thu, Jul 09, 2020 at 11:42:30PM +0400, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  block/nbd.c        | 86 +++++++++++++++++++++++++++++++++++-----------
>  util/Makefile.objs |  2 +-
>  2 files changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index faadcab442b..fdc4a53a98f 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -31,7 +31,10 @@
>  #include "qemu/osdep.h"
>  
>  #include "trace.h"
> +#ifndef HAVE_GLIB_GURI
>  #include "qemu/uri.h"
> +#endif
> +#include "qemu/error-report.h"
>  #include "qemu/option.h"
>  #include "qemu/cutils.h"
>  #include "qemu/main-loop.h"
> @@ -1513,71 +1516,112 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
>  /*
>   * Parse nbd_open options
>   */
> -
>  static int nbd_parse_uri(const char *filename, QDict *options)
>  {
> +    const char *p, *scheme, *server, *socket = NULL;
> +    int port;
> +    bool is_unix;
> +
> +#ifdef HAVE_GLIB_GURI
> +    g_autoptr(GUri) uri = NULL;
> +    g_autoptr(GHashTable) params = NULL;
> +    g_autoptr(GError) err = NULL;
> +
> +    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
> +    if (!uri) {
> +        error_report("Failed to parse NBD URI: %s", err->message);
> +        return -EINVAL;
> +    }
> +
> +    p = g_uri_get_path(uri);
> +    scheme = g_uri_get_scheme(uri);
> +    server = g_uri_get_host(uri);
> +    port = g_uri_get_port(uri);

I would have expected this code to fail to compile as we're setting
GLIB_VERSION_MAX_ALLOWED == GLIB_VERSION_2_48  and GUri is tagged
as newer than that.

In any case, having this conditonal code in all callers is definitely
not a desirable approach. If we want to use it, then I think we need to
pull a copy of GUri into QEMU and expose it via glib-compat.h, so that
callers can use it unconditionally.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH 10/13] sheepdog: add GUri-based URI parsing

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/sheepdog.c | 99 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 28 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 3403adfc2cd..3f3f5b7dba9 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1003,24 +1003,48 @@ static void sd_config_done(SheepdogConfig *cfg)
 static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
                          Error **errp)
 {
-    g_autoptr(QueryParams) qp = NULL;
-    g_autoptr(URI) uri = NULL;
+    const char *scheme, *server, *path, *fragment, *socket = NULL;
+    int port;
     bool is_unix;
 
-    memset(cfg, 0, sizeof(*cfg));
+#ifdef HAVE_GLIB_GURI
+    g_autoptr(GUri) uri = NULL;
+    g_autoptr(GHashTable) params = NULL;
+    g_autoptr(GError) err = NULL;
+
+    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
+    if (!uri) {
+        error_report("Failed to parse sheepdog URI: %s", err->message);
+        return;
+    }
+    scheme = g_uri_get_scheme(uri);
+    server = g_uri_get_host(uri);
+    port = g_uri_get_port(uri);
+    path = g_uri_get_path(uri);
+    fragment = g_uri_get_fragment(uri);
+#else
+    g_autoptr(QueryParams) qp = NULL;
+    g_autoptr(URI) uri = NULL;
 
     uri = uri_parse(filename);
     if (!uri) {
         error_setg(errp, "invalid URI '%s'", filename);
         return;
     }
+    scheme = uri->scheme;
+    server = uri->server;
+    port = uri->port;
+    path = uri->path;
+    fragment = uri->fragment;
+#endif
+    memset(cfg, 0, sizeof(*cfg));
 
     /* transport */
-    if (!g_strcmp0(uri->scheme, "sheepdog")) {
+    if (!g_strcmp0(scheme, "sheepdog")) {
         is_unix = false;
-    } else if (!g_strcmp0(uri->scheme, "sheepdog+tcp")) {
+    } else if (!g_strcmp0(scheme, "sheepdog+tcp")) {
         is_unix = false;
-    } else if (!g_strcmp0(uri->scheme, "sheepdog+unix")) {
+    } else if (!g_strcmp0(scheme, "sheepdog+unix")) {
         is_unix = true;
     } else {
         error_setg(errp, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
@@ -1028,52 +1052,71 @@ static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
         return;
     }
 
-    if (uri->path == NULL || !strcmp(uri->path, "/")) {
+#ifdef HAVE_GLIB_GURI
+    params = g_uri_parse_params(g_uri_get_query(uri), -1,
+                                "&;", G_URI_PARAMS_NONE, &err);
+    if (err) {
+        error_report("Failed to parse sheepdog URI query: %s", err->message);
+        return;
+    }
+    if ((is_unix && g_hash_table_size(params) != 1) ||
+        (!is_unix && g_hash_table_size(params) != 0)) {
+        error_setg(errp, "unexpected query parameters");
+        return;
+    }
+    if (is_unix) {
+        socket = g_hash_table_lookup(params, "socket");
+    }
+#else
+    qp = query_params_parse(uri->query);
+    if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
+        error_setg(errp, "unexpected query parameters");
+        return;
+    }
+    if (is_unix) {
+        if (!g_str_equal(qp->p[0].name, "socket")) {
+            error_setg(errp, "unexpected query parameters");
+            return;
+        }
+        socket = qp->p[0].value;
+    }
+#endif
+    if (path == NULL || !strcmp(path, "/")) {
         error_setg(errp, "missing file path in URI");
         return;
     }
-    if (g_strlcpy(cfg->vdi, uri->path + 1, SD_MAX_VDI_LEN)
+    if (g_strlcpy(cfg->vdi, path + 1, SD_MAX_VDI_LEN)
         >= SD_MAX_VDI_LEN) {
         error_setg(errp, "VDI name is too long");
         return;
     }
 
-    qp = query_params_parse(uri->query);
-
     if (is_unix) {
         /* sheepdog+unix:///vdiname?socket=path */
-        if (uri->server || uri->port) {
+        if (server || port > 0) {
             error_setg(errp, "URI scheme %s doesn't accept a server address",
-                       uri->scheme);
+                       scheme);
             return;
         }
-        if (!qp->n) {
+        if (!socket) {
             error_setg(errp,
                        "URI scheme %s requires query parameter 'socket'",
-                       uri->scheme);
-            return;
-        }
-        if (qp->n != 1 || strcmp(qp->p[0].name, "socket")) {
-            error_setg(errp, "unexpected query parameters");
+                       scheme);
             return;
         }
-        cfg->path = g_strdup(qp->p[0].value);
+        cfg->path = g_strdup(socket);
     } else {
         /* sheepdog[+tcp]://[host:port]/vdiname */
-        if (qp->n) {
-            error_setg(errp, "unexpected query parameters");
-            return;
-        }
-        cfg->host = g_strdup(uri->server);
-        cfg->port = uri->port;
+        cfg->host = g_strdup(server);
+        cfg->port = port;
     }
 
     /* snapshot tag */
-    if (uri->fragment) {
-        if (!sd_parse_snapid_or_tag(uri->fragment,
+    if (fragment) {
+        if (!sd_parse_snapid_or_tag(fragment,
                                     &cfg->snap_id, cfg->tag)) {
             error_setg(errp, "'%s' is not a valid snapshot ID",
-                       uri->fragment);
+                       fragment);
             return;
         }
     } else {
-- 
2.27.0.221.ga08a83db2b

[PATCH 11/13] nfs: add GUri-based URI parsing

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/nfs.c | 96 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 65 insertions(+), 31 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 93d719551d2..0b24044535d 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -77,6 +77,31 @@ typedef struct NFSRPC {
 
 static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
 {
+    const char *scheme, *server, *path, *key, *value;
+
+#ifdef HAVE_GLIB_GURI
+    g_autoptr(GUri) uri = NULL;
+    g_autoptr(GHashTable) params = NULL;
+    g_autoptr(GError) err = NULL;
+    GHashTableIter iter;
+
+    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
+    if (!uri) {
+        error_setg(errp, "Failed to parse NFS URI: %s", err->message);
+        return -EINVAL;
+    }
+
+    params = g_uri_parse_params(g_uri_get_query(uri), -1,
+                                "&;", G_URI_PARAMS_NONE, &err);
+    if (err) {
+        error_report("Failed to parse NFS URI query: %s", err->message);
+        return -EINVAL;
+    }
+
+    scheme = g_uri_get_scheme(uri);
+    server = g_uri_get_host(uri);
+    path = g_uri_get_path(uri);
+#else
     g_autoptr(URI) uri = NULL;
     g_autoptr(QueryParams) qp = NULL;
     int i;
@@ -86,58 +111,67 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
         error_setg(errp, "Invalid URI specified");
         return -EINVAL;
     }
-    if (g_strcmp0(uri->scheme, "nfs") != 0) {
-        error_setg(errp, "URI scheme must be 'nfs'");
+
+    qp = query_params_parse(uri->query);
+    if (!qp) {
+        error_setg(errp, "could not parse query parameters");
         return -EINVAL;
     }
 
-    if (!uri->server) {
-        error_setg(errp, "missing hostname in URI");
+    scheme = uri->scheme;
+    server = uri->server;
+    path = uri->path;
+#endif
+    if (g_strcmp0(scheme, "nfs") != 0) {
+        error_setg(errp, "URI scheme must be 'nfs'");
         return -EINVAL;
     }
 
-    if (!uri->path) {
-        error_setg(errp, "missing file path in URI");
+    if (!server) {
+        error_setg(errp, "missing hostname in URI");
         return -EINVAL;
     }
 
-    qp = query_params_parse(uri->query);
-    if (!qp) {
-        error_setg(errp, "could not parse query parameters");
+    if (!path) {
+        error_setg(errp, "missing file path in URI");
         return -EINVAL;
     }
 
-    qdict_put_str(options, "server.host", uri->server);
+    qdict_put_str(options, "server.host", server);
     qdict_put_str(options, "server.type", "inet");
-    qdict_put_str(options, "path", uri->path);
+    qdict_put_str(options, "path", path);
 
+#ifdef HAVE_GLIB_GURI
+    g_hash_table_iter_init(&iter, params);
+    while (g_hash_table_iter_next(&iter, (void **)&key, (void **)&value)) {
+#else
     for (i = 0; i < qp->n; i++) {
+        key = qp->p[i].name;
+        value = qp->p[i].value;
+#endif
         unsigned long long val;
-        if (!qp->p[i].value) {
-            error_setg(errp, "Value for NFS parameter expected: %s",
-                       qp->p[i].name);
+        if (!value) {
+            error_setg(errp, "Value for NFS parameter expected: %s", key);
             return -EINVAL;
         }
-        if (parse_uint_full(qp->p[i].value, &val, 0)) {
-            error_setg(errp, "Illegal value for NFS parameter: %s",
-                       qp->p[i].name);
+        if (parse_uint_full(value, &val, 0)) {
+            error_setg(errp, "Illegal value for NFS parameter: %s", key);
             return -EINVAL;
         }
-        if (!strcmp(qp->p[i].name, "uid")) {
-            qdict_put_str(options, "user", qp->p[i].value);
-        } else if (!strcmp(qp->p[i].name, "gid")) {
-            qdict_put_str(options, "group", qp->p[i].value);
-        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
-            qdict_put_str(options, "tcp-syn-count", qp->p[i].value);
-        } else if (!strcmp(qp->p[i].name, "readahead")) {
-            qdict_put_str(options, "readahead-size", qp->p[i].value);
-        } else if (!strcmp(qp->p[i].name, "pagecache")) {
-            qdict_put_str(options, "page-cache-size", qp->p[i].value);
-        } else if (!strcmp(qp->p[i].name, "debug")) {
-            qdict_put_str(options, "debug", qp->p[i].value);
+        if (!strcmp(key, "uid")) {
+            qdict_put_str(options, "user", value);
+        } else if (!strcmp(key, "gid")) {
+            qdict_put_str(options, "group", value);
+        } else if (!strcmp(key, "tcp-syncnt")) {
+            qdict_put_str(options, "tcp-syn-count", value);
+        } else if (!strcmp(key, "readahead")) {
+            qdict_put_str(options, "readahead-size", value);
+        } else if (!strcmp(key, "pagecache")) {
+            qdict_put_str(options, "page-cache-size", value);
+        } else if (!strcmp(key, "debug")) {
+            qdict_put_str(options, "debug", value);
         } else {
-            error_setg(errp, "Unknown NFS parameter name: %s",
-                       qp->p[i].name);
+            error_setg(errp, "Unknown NFS parameter name: %s", key);
             return -EINVAL;
         }
     }
-- 
2.27.0.221.ga08a83db2b

[PATCH 12/13] gluster: add GUri-based URI parsing

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/gluster.c | 81 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 61 insertions(+), 20 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index c06eca1c12f..2cad76deabf 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -288,9 +288,9 @@ static void glfs_clear_preopened(glfs_t *fs)
     }
 }
 
-static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
+static int parse_volume_options(BlockdevOptionsGluster *gconf, const char *path)
 {
-    char *p, *q;
+    const char *p, *q;
 
     if (!path) {
         return -EINVAL;
@@ -349,56 +349,97 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
                                   const char *filename)
 {
     SocketAddress *gsconf;
+    bool is_unix = false;
+    int ret, port;
+    const char *scheme, *server, *path, *key = NULL, *value = NULL;
+    size_t nparams;
+
+#ifdef HAVE_GLIB_GURI
+    g_autoptr(GUri) uri = NULL;
+    g_autoptr(GHashTable) params = NULL;
+    g_autoptr(GError) err = NULL;
+
+    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
+    if (!uri) {
+        error_report("Failed to parse gluster URI: %s", err->message);
+        return -EINVAL;
+    }
+
+    params = g_uri_parse_params(g_uri_get_query(uri), -1,
+                                "&;", G_URI_PARAMS_NONE, &err);
+    if (err) {
+        error_report("Failed to parse gluster URI query: %s", err->message);
+        return -EINVAL;
+    }
+
+    scheme = g_uri_get_scheme(uri);
+    server = g_uri_get_host(uri);
+    port = g_uri_get_port(uri);
+    path = g_uri_get_path(uri);
+    nparams = g_hash_table_size(params);
+    g_hash_table_lookup_extended(params, "socket",
+                                 (void **)&key, (void **)&value);
+#else
     g_autoptr(URI) uri = NULL;
     g_autoptr(QueryParams) qp = NULL;
-    bool is_unix = false;
-    int ret;
 
     uri = uri_parse(filename);
     if (!uri) {
         return -EINVAL;
     }
 
+    qp = query_params_parse(uri->query);
+
+    scheme = uri->scheme;
+    server = uri->server;
+    port = uri->port;
+    path = uri->path;
+    nparams = qp->n;
+    if (nparams > 0) {
+        key = qp->p[0].name;
+        value = qp->p[0].value;
+    }
+#endif
+
+    if (nparams > 1 || (is_unix && !nparams) || (!is_unix && nparams)) {
+        return -EINVAL;
+    }
+
     gconf->server = g_new0(SocketAddressList, 1);
     gconf->server->value = gsconf = g_new0(SocketAddress, 1);
 
     /* transport */
-    if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
+    if (!scheme || !strcmp(scheme, "gluster")) {
         gsconf->type = SOCKET_ADDRESS_TYPE_INET;
-    } else if (!strcmp(uri->scheme, "gluster+tcp")) {
+    } else if (!strcmp(scheme, "gluster+tcp")) {
         gsconf->type = SOCKET_ADDRESS_TYPE_INET;
-    } else if (!strcmp(uri->scheme, "gluster+unix")) {
+    } else if (!strcmp(scheme, "gluster+unix")) {
         gsconf->type = SOCKET_ADDRESS_TYPE_UNIX;
         is_unix = true;
-    } else if (!strcmp(uri->scheme, "gluster+rdma")) {
+    } else if (!strcmp(scheme, "gluster+rdma")) {
         gsconf->type = SOCKET_ADDRESS_TYPE_INET;
         warn_report("rdma feature is not supported, falling back to tcp");
     } else {
         return -EINVAL;
     }
 
-    ret = parse_volume_options(gconf, uri->path);
+    ret = parse_volume_options(gconf, path);
     if (ret < 0) {
         return ret;
     }
 
-    qp = query_params_parse(uri->query);
-    if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) {
-        return -EINVAL;
-    }
-
     if (is_unix) {
-        if (uri->server || uri->port) {
+        if (server || port) {
             return -EINVAL;
         }
-        if (strcmp(qp->p[0].name, "socket")) {
+        if (g_strcmp0(key, "socket")) {
             return -EINVAL;
         }
-        gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
+        gsconf->u.q_unix.path = g_strdup(value);
     } else {
-        gsconf->u.inet.host = g_strdup(uri->server ? uri->server : "localhost");
-        if (uri->port) {
-            gsconf->u.inet.port = g_strdup_printf("%d", uri->port);
+        gsconf->u.inet.host = g_strdup(server ? server : "localhost");
+        if (port) {
+            gsconf->u.inet.port = g_strdup_printf("%d", port);
         } else {
             gsconf->u.inet.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
         }
-- 
2.27.0.221.ga08a83db2b

[PATCH 13/13] ssh: add GUri-based URI parsing

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 block/ssh.c | 75 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index c8f6ad79e3c..d2bc6277613 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -180,9 +180,37 @@ static void sftp_error_trace(BDRVSSHState *s, const char *op)
 
 static int parse_uri(const char *filename, QDict *options, Error **errp)
 {
+    g_autofree char *port_str = NULL;
+    const char *scheme, *server, *path, *user, *key, *value;
+    gint port;
+
+#ifdef HAVE_GLIB_GURI
+    g_autoptr(GUri) uri = NULL;
+    g_autoptr(GHashTable) params = NULL;
+    g_autoptr(GError) err = NULL;
+    GHashTableIter iter;
+
+    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
+    if (!uri) {
+        error_setg(errp, "Failed to parse SSH URI: %s", err->message);
+        return -EINVAL;
+    }
+
+    params = g_uri_parse_params(g_uri_get_query(uri), -1,
+                                "&;", G_URI_PARAMS_NONE, &err);
+    if (err) {
+        error_report("Failed to parse SSH URI query: %s", err->message);
+        return -EINVAL;
+    }
+
+    scheme = g_uri_get_scheme(uri);
+    user = g_uri_get_user(uri);
+    server = g_uri_get_host(uri);
+    path = g_uri_get_path(uri);
+    port = g_uri_get_port(uri);
+#else
     g_autoptr(URI) uri = NULL;
     g_autoptr(QueryParams) qp = NULL;
-    g_autofree char *port_str = NULL;
     int i;
 
     uri = uri_parse(filename);
@@ -190,44 +218,57 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
         return -EINVAL;
     }
 
-    if (g_strcmp0(uri->scheme, "ssh") != 0) {
-        error_setg(errp, "URI scheme must be 'ssh'");
+    qp = query_params_parse(uri->query);
+    if (!qp) {
+        error_setg(errp, "could not parse query parameters");
         return -EINVAL;
     }
 
-    if (!uri->server || strcmp(uri->server, "") == 0) {
-        error_setg(errp, "missing hostname in URI");
+    scheme = uri->scheme;
+    user = uri->user;
+    server = uri->server;
+    path = uri->path;
+    port = uri->port;
+#endif
+    if (g_strcmp0(scheme, "ssh") != 0) {
+        error_setg(errp, "URI scheme must be 'ssh'");
         return -EINVAL;
     }
 
-    if (!uri->path || strcmp(uri->path, "") == 0) {
-        error_setg(errp, "missing remote path in URI");
+    if (!server || strcmp(server, "") == 0) {
+        error_setg(errp, "missing hostname in URI");
         return -EINVAL;
     }
 
-    qp = query_params_parse(uri->query);
-    if (!qp) {
-        error_setg(errp, "could not parse query parameters");
+    if (!path || strcmp(path, "") == 0) {
+        error_setg(errp, "missing remote path in URI");
         return -EINVAL;
     }
 
-    if(uri->user && strcmp(uri->user, "") != 0) {
-        qdict_put_str(options, "user", uri->user);
+    if (user && strcmp(user, "") != 0) {
+        qdict_put_str(options, "user", user);
     }
 
-    qdict_put_str(options, "server.host", uri->server);
+    qdict_put_str(options, "server.host", server);
 
-    port_str = g_strdup_printf("%d", uri->port ?: 22);
+    port_str = g_strdup_printf("%d", port ?: 22);
     qdict_put_str(options, "server.port", port_str);
 
-    qdict_put_str(options, "path", uri->path);
+    qdict_put_str(options, "path", path);
 
     /* Pick out any query parameters that we understand, and ignore
      * the rest.
      */
+#ifdef HAVE_GLIB_GURI
+    g_hash_table_iter_init(&iter, params);
+    while (g_hash_table_iter_next(&iter, (void **)&key, (void **)&value)) {
+#else
     for (i = 0; i < qp->n; ++i) {
-        if (strcmp(qp->p[i].name, "host_key_check") == 0) {
-            qdict_put_str(options, "host_key_check", qp->p[i].value);
+        key = qp->p[i].name;
+        value = qp->p[i].value;
+#endif
+        if (g_strcmp0(key, "host_key_check") == 0) {
+            qdict_put_str(options, "host_key_check", value);
         }
     }
 
-- 
2.27.0.221.ga08a83db2b

Re: [PATCH 13/13] ssh: add GUri-based URI parsing

[Richard W.M. Jones]

On Thu, Jul 09, 2020 at 11:42:34PM +0400, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  block/ssh.c | 75 +++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index c8f6ad79e3c..d2bc6277613 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -180,9 +180,37 @@ static void sftp_error_trace(BDRVSSHState *s, const char *op)
>  
>  static int parse_uri(const char *filename, QDict *options, Error **errp)
>  {
> +    g_autofree char *port_str = NULL;
> +    const char *scheme, *server, *path, *user, *key, *value;
> +    gint port;
> +
> +#ifdef HAVE_GLIB_GURI
> +    g_autoptr(GUri) uri = NULL;
> +    g_autoptr(GHashTable) params = NULL;
> +    g_autoptr(GError) err = NULL;
> +    GHashTableIter iter;
> +
> +    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
> +    if (!uri) {
> +        error_setg(errp, "Failed to parse SSH URI: %s", err->message);
> +        return -EINVAL;
> +    }
> +
> +    params = g_uri_parse_params(g_uri_get_query(uri), -1,
> +                                "&;", G_URI_PARAMS_NONE, &err);
> +    if (err) {
> +        error_report("Failed to parse SSH URI query: %s", err->message);
> +        return -EINVAL;
> +    }
> +
> +    scheme = g_uri_get_scheme(uri);
> +    user = g_uri_get_user(uri);
> +    server = g_uri_get_host(uri);
> +    path = g_uri_get_path(uri);
> +    port = g_uri_get_port(uri);
> +#else
>      g_autoptr(URI) uri = NULL;
>      g_autoptr(QueryParams) qp = NULL;
> -    g_autofree char *port_str = NULL;
>      int i;

As Dan said already, this conditional code looks horrible and is going
to be a maintenance burden.  Was there a later version of this patch
series that resolved this?  I don't think I saw one.

Rich.


>      uri = uri_parse(filename);
> @@ -190,44 +218,57 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
>          return -EINVAL;
>      }
>  
> -    if (g_strcmp0(uri->scheme, "ssh") != 0) {
> -        error_setg(errp, "URI scheme must be 'ssh'");
> +    qp = query_params_parse(uri->query);
> +    if (!qp) {
> +        error_setg(errp, "could not parse query parameters");
>          return -EINVAL;
>      }
>  
> -    if (!uri->server || strcmp(uri->server, "") == 0) {
> -        error_setg(errp, "missing hostname in URI");
> +    scheme = uri->scheme;
> +    user = uri->user;
> +    server = uri->server;
> +    path = uri->path;
> +    port = uri->port;
> +#endif
> +    if (g_strcmp0(scheme, "ssh") != 0) {
> +        error_setg(errp, "URI scheme must be 'ssh'");
>          return -EINVAL;
>      }
>  
> -    if (!uri->path || strcmp(uri->path, "") == 0) {
> -        error_setg(errp, "missing remote path in URI");
> +    if (!server || strcmp(server, "") == 0) {
> +        error_setg(errp, "missing hostname in URI");
>          return -EINVAL;
>      }
>  
> -    qp = query_params_parse(uri->query);
> -    if (!qp) {
> -        error_setg(errp, "could not parse query parameters");
> +    if (!path || strcmp(path, "") == 0) {
> +        error_setg(errp, "missing remote path in URI");
>          return -EINVAL;
>      }
>  
> -    if(uri->user && strcmp(uri->user, "") != 0) {
> -        qdict_put_str(options, "user", uri->user);
> +    if (user && strcmp(user, "") != 0) {
> +        qdict_put_str(options, "user", user);
>      }
>  
> -    qdict_put_str(options, "server.host", uri->server);
> +    qdict_put_str(options, "server.host", server);
>  
> -    port_str = g_strdup_printf("%d", uri->port ?: 22);
> +    port_str = g_strdup_printf("%d", port ?: 22);
>      qdict_put_str(options, "server.port", port_str);
>  
> -    qdict_put_str(options, "path", uri->path);
> +    qdict_put_str(options, "path", path);
>  
>      /* Pick out any query parameters that we understand, and ignore
>       * the rest.
>       */
> +#ifdef HAVE_GLIB_GURI
> +    g_hash_table_iter_init(&iter, params);
> +    while (g_hash_table_iter_next(&iter, (void **)&key, (void **)&value)) {
> +#else
>      for (i = 0; i < qp->n; ++i) {
> -        if (strcmp(qp->p[i].name, "host_key_check") == 0) {
> -            qdict_put_str(options, "host_key_check", qp->p[i].value);
> +        key = qp->p[i].name;
> +        value = qp->p[i].value;
> +#endif
> +        if (g_strcmp0(key, "host_key_check") == 0) {
> +            qdict_put_str(options, "host_key_check", value);
>          }
>      }
>  
> -- 
> 2.27.0.221.ga08a83db2b

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [PATCH 13/13] ssh: add GUri-based URI parsing

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 5992 bytes --]

Hi

On Thu, Jul 23, 2020 at 4:33 PM Richard W.M. Jones <rjones@redhat.com>
wrote:

> On Thu, Jul 09, 2020 at 11:42:34PM +0400, Marc-André Lureau wrote:
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  block/ssh.c | 75 +++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 58 insertions(+), 17 deletions(-)
> >
> > diff --git a/block/ssh.c b/block/ssh.c
> > index c8f6ad79e3c..d2bc6277613 100644
> > --- a/block/ssh.c
> > +++ b/block/ssh.c
> > @@ -180,9 +180,37 @@ static void sftp_error_trace(BDRVSSHState *s, const
> char *op)
> >
> >  static int parse_uri(const char *filename, QDict *options, Error **errp)
> >  {
> > +    g_autofree char *port_str = NULL;
> > +    const char *scheme, *server, *path, *user, *key, *value;
> > +    gint port;
> > +
> > +#ifdef HAVE_GLIB_GURI
> > +    g_autoptr(GUri) uri = NULL;
> > +    g_autoptr(GHashTable) params = NULL;
> > +    g_autoptr(GError) err = NULL;
> > +    GHashTableIter iter;
> > +
> > +    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
> > +    if (!uri) {
> > +        error_setg(errp, "Failed to parse SSH URI: %s", err->message);
> > +        return -EINVAL;
> > +    }
> > +
> > +    params = g_uri_parse_params(g_uri_get_query(uri), -1,
> > +                                "&;", G_URI_PARAMS_NONE, &err);
> > +    if (err) {
> > +        error_report("Failed to parse SSH URI query: %s", err->message);
> > +        return -EINVAL;
> > +    }
> > +
> > +    scheme = g_uri_get_scheme(uri);
> > +    user = g_uri_get_user(uri);
> > +    server = g_uri_get_host(uri);
> > +    path = g_uri_get_path(uri);
> > +    port = g_uri_get_port(uri);
> > +#else
> >      g_autoptr(URI) uri = NULL;
> >      g_autoptr(QueryParams) qp = NULL;
> > -    g_autofree char *port_str = NULL;
> >      int i;
>
> As Dan said already, this conditional code looks horrible and is going
> to be a maintenance burden.  Was there a later version of this patch
> series that resolved this?  I don't think I saw one.
>

The patch is quite experimental. glib didn't even yet receive a release
with GUri! But since I am working on the glib side, I wanted to make sure
it covers qemu needs.

I will revisit the series once GUri is frozen & released (in
mid-september),and use a copy version fallback.

Although, as I said in the cover, this is a bit riskier than having a
transition period with both the old libxml-based parser and glib-based one
for very recent distro.

Tbh, I think having both is not a big burden, because there is very low
activity around those functions. Iow, we are not spreading qemu with a lot
of extra conditionals, but only in very limited mostly static places.


> Rich.
>
>
> >      uri = uri_parse(filename);
> > @@ -190,44 +218,57 @@ static int parse_uri(const char *filename, QDict
> *options, Error **errp)
> >          return -EINVAL;
> >      }
> >
> > -    if (g_strcmp0(uri->scheme, "ssh") != 0) {
> > -        error_setg(errp, "URI scheme must be 'ssh'");
> > +    qp = query_params_parse(uri->query);
> > +    if (!qp) {
> > +        error_setg(errp, "could not parse query parameters");
> >          return -EINVAL;
> >      }
> >
> > -    if (!uri->server || strcmp(uri->server, "") == 0) {
> > -        error_setg(errp, "missing hostname in URI");
> > +    scheme = uri->scheme;
> > +    user = uri->user;
> > +    server = uri->server;
> > +    path = uri->path;
> > +    port = uri->port;
> > +#endif
> > +    if (g_strcmp0(scheme, "ssh") != 0) {
> > +        error_setg(errp, "URI scheme must be 'ssh'");
> >          return -EINVAL;
> >      }
> >
> > -    if (!uri->path || strcmp(uri->path, "") == 0) {
> > -        error_setg(errp, "missing remote path in URI");
> > +    if (!server || strcmp(server, "") == 0) {
> > +        error_setg(errp, "missing hostname in URI");
> >          return -EINVAL;
> >      }
> >
> > -    qp = query_params_parse(uri->query);
> > -    if (!qp) {
> > -        error_setg(errp, "could not parse query parameters");
> > +    if (!path || strcmp(path, "") == 0) {
> > +        error_setg(errp, "missing remote path in URI");
> >          return -EINVAL;
> >      }
> >
> > -    if(uri->user && strcmp(uri->user, "") != 0) {
> > -        qdict_put_str(options, "user", uri->user);
> > +    if (user && strcmp(user, "") != 0) {
> > +        qdict_put_str(options, "user", user);
> >      }
> >
> > -    qdict_put_str(options, "server.host", uri->server);
> > +    qdict_put_str(options, "server.host", server);
> >
> > -    port_str = g_strdup_printf("%d", uri->port ?: 22);
> > +    port_str = g_strdup_printf("%d", port ?: 22);
> >      qdict_put_str(options, "server.port", port_str);
> >
> > -    qdict_put_str(options, "path", uri->path);
> > +    qdict_put_str(options, "path", path);
> >
> >      /* Pick out any query parameters that we understand, and ignore
> >       * the rest.
> >       */
> > +#ifdef HAVE_GLIB_GURI
> > +    g_hash_table_iter_init(&iter, params);
> > +    while (g_hash_table_iter_next(&iter, (void **)&key, (void
> **)&value)) {
> > +#else
> >      for (i = 0; i < qp->n; ++i) {
> > -        if (strcmp(qp->p[i].name, "host_key_check") == 0) {
> > -            qdict_put_str(options, "host_key_check", qp->p[i].value);
> > +        key = qp->p[i].name;
> > +        value = qp->p[i].value;
> > +#endif
> > +        if (g_strcmp0(key, "host_key_check") == 0) {
> > +            qdict_put_str(options, "host_key_check", value);
> >          }
> >      }
> >
> > --
> > 2.27.0.221.ga08a83db2b
>
> --
> Richard Jones, Virtualization Group, Red Hat
> http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> libguestfs lets you edit virtual machines.  Supports shell scripting,
> bindings from many languages.  http://libguestfs.org
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 8306 bytes --]

Re: [PATCH 13/13] ssh: add GUri-based URI parsing

[Daniel P. Berrangé]

On Thu, Jul 23, 2020 at 04:58:48PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jul 23, 2020 at 4:33 PM Richard W.M. Jones <rjones@redhat.com>
> wrote:
> 
> > On Thu, Jul 09, 2020 at 11:42:34PM +0400, Marc-André Lureau wrote:
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  block/ssh.c | 75 +++++++++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 58 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/block/ssh.c b/block/ssh.c
> > > index c8f6ad79e3c..d2bc6277613 100644
> > > --- a/block/ssh.c
> > > +++ b/block/ssh.c
> > > @@ -180,9 +180,37 @@ static void sftp_error_trace(BDRVSSHState *s, const
> > char *op)
> > >
> > >  static int parse_uri(const char *filename, QDict *options, Error **errp)
> > >  {
> > > +    g_autofree char *port_str = NULL;
> > > +    const char *scheme, *server, *path, *user, *key, *value;
> > > +    gint port;
> > > +
> > > +#ifdef HAVE_GLIB_GURI
> > > +    g_autoptr(GUri) uri = NULL;
> > > +    g_autoptr(GHashTable) params = NULL;
> > > +    g_autoptr(GError) err = NULL;
> > > +    GHashTableIter iter;
> > > +
> > > +    uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);
> > > +    if (!uri) {
> > > +        error_setg(errp, "Failed to parse SSH URI: %s", err->message);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    params = g_uri_parse_params(g_uri_get_query(uri), -1,
> > > +                                "&;", G_URI_PARAMS_NONE, &err);
> > > +    if (err) {
> > > +        error_report("Failed to parse SSH URI query: %s", err->message);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    scheme = g_uri_get_scheme(uri);
> > > +    user = g_uri_get_user(uri);
> > > +    server = g_uri_get_host(uri);
> > > +    path = g_uri_get_path(uri);
> > > +    port = g_uri_get_port(uri);
> > > +#else
> > >      g_autoptr(URI) uri = NULL;
> > >      g_autoptr(QueryParams) qp = NULL;
> > > -    g_autofree char *port_str = NULL;
> > >      int i;
> >
> > As Dan said already, this conditional code looks horrible and is going
> > to be a maintenance burden.  Was there a later version of this patch
> > series that resolved this?  I don't think I saw one.
> >
> 
> The patch is quite experimental. glib didn't even yet receive a release
> with GUri! But since I am working on the glib side, I wanted to make sure
> it covers qemu needs.
> 
> I will revisit the series once GUri is frozen & released (in
> mid-september),and use a copy version fallback.
> 
> Although, as I said in the cover, this is a bit riskier than having a
> transition period with both the old libxml-based parser and glib-based one
> for very recent distro.

The risk here is that the GUri has different semantics/behaviour to the
libxml one.

If that risk is large, then we shouldn't use GUri at all.

If the risk is low enough that we consider GUri a viable option, then
we should use it exclusively IMHO. This guarantees that all deployments
of QEMU will have identical behaviour, which will lower our support
burden in the event that bugs do appear, as we'll only have one codepath
to worry about.

The test suite you provided for GUri looks pretty comprehensive, and
QEMU's iotests are pretty decent too, so I think between the two we
have enough coverage that we should have confidence in using GUri
exclusively. If we hit obscure edge cases, we can just deal with it
as it arises.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|