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

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

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

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 :|