<div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 23, 2020 at 4:33 PM Richard W.M. Jones <<a href="mailto:rjones@redhat.com">rjones@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Jul 09, 2020 at 11:42:34PM +0400, Marc-André Lureau wrote:<br>
> Signed-off-by: Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com" target="_blank">marcandre.lureau@redhat.com</a>><br>
> ---<br>
> block/ssh.c | 75 +++++++++++++++++++++++++++++++++++++++++------------<br>
> 1 file changed, 58 insertions(+), 17 deletions(-)<br>
> <br>
> diff --git a/block/ssh.c b/block/ssh.c<br>
> index c8f6ad79e3c..d2bc6277613 100644<br>
> --- a/block/ssh.c<br>
> +++ b/block/ssh.c<br>
> @@ -180,9 +180,37 @@ static void sftp_error_trace(BDRVSSHState *s, const char *op)<br>
> <br>
> static int parse_uri(const char *filename, QDict *options, Error **errp)<br>
> {<br>
> + g_autofree char *port_str = NULL;<br>
> + const char *scheme, *server, *path, *user, *key, *value;<br>
> + gint port;<br>
> +<br>
> +#ifdef HAVE_GLIB_GURI<br>
> + g_autoptr(GUri) uri = NULL;<br>
> + g_autoptr(GHashTable) params = NULL;<br>
> + g_autoptr(GError) err = NULL;<br>
> + GHashTableIter iter;<br>
> +<br>
> + uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, &err);<br>
> + if (!uri) {<br>
> + error_setg(errp, "Failed to parse SSH URI: %s", err->message);<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + params = g_uri_parse_params(g_uri_get_query(uri), -1,<br>
> + "&;", G_URI_PARAMS_NONE, &err);<br>
> + if (err) {<br>
> + error_report("Failed to parse SSH URI query: %s", err->message);<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + scheme = g_uri_get_scheme(uri);<br>
> + user = g_uri_get_user(uri);<br>
> + server = g_uri_get_host(uri);<br>
> + path = g_uri_get_path(uri);<br>
> + port = g_uri_get_port(uri);<br>
> +#else<br>
> g_autoptr(URI) uri = NULL;<br>
> g_autoptr(QueryParams) qp = NULL;<br>
> - g_autofree char *port_str = NULL;<br>
> int i;<br>
<br>
As Dan said already, this conditional code looks horrible and is going<br>
to be a maintenance burden. Was there a later version of this patch<br>
series that resolved this? I don't think I saw one.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>I will revisit the series once GUri is frozen & released (in mid-september),and use a copy version fallback.</div><div><br></div><div>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.<br></div><div><br></div><div>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.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Rich.<br>
<br>
<br>
> uri = uri_parse(filename);<br>
> @@ -190,44 +218,57 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)<br>
> return -EINVAL;<br>
> }<br>
> <br>
> - if (g_strcmp0(uri->scheme, "ssh") != 0) {<br>
> - error_setg(errp, "URI scheme must be 'ssh'");<br>
> + qp = query_params_parse(uri->query);<br>
> + if (!qp) {<br>
> + error_setg(errp, "could not parse query parameters");<br>
> return -EINVAL;<br>
> }<br>
> <br>
> - if (!uri->server || strcmp(uri->server, "") == 0) {<br>
> - error_setg(errp, "missing hostname in URI");<br>
> + scheme = uri->scheme;<br>
> + user = uri->user;<br>
> + server = uri->server;<br>
> + path = uri->path;<br>
> + port = uri->port;<br>
> +#endif<br>
> + if (g_strcmp0(scheme, "ssh") != 0) {<br>
> + error_setg(errp, "URI scheme must be 'ssh'");<br>
> return -EINVAL;<br>
> }<br>
> <br>
> - if (!uri->path || strcmp(uri->path, "") == 0) {<br>
> - error_setg(errp, "missing remote path in URI");<br>
> + if (!server || strcmp(server, "") == 0) {<br>
> + error_setg(errp, "missing hostname in URI");<br>
> return -EINVAL;<br>
> }<br>
> <br>
> - qp = query_params_parse(uri->query);<br>
> - if (!qp) {<br>
> - error_setg(errp, "could not parse query parameters");<br>
> + if (!path || strcmp(path, "") == 0) {<br>
> + error_setg(errp, "missing remote path in URI");<br>
> return -EINVAL;<br>
> }<br>
> <br>
> - if(uri->user && strcmp(uri->user, "") != 0) {<br>
> - qdict_put_str(options, "user", uri->user);<br>
> + if (user && strcmp(user, "") != 0) {<br>
> + qdict_put_str(options, "user", user);<br>
> }<br>
> <br>
> - qdict_put_str(options, "server.host", uri->server);<br>
> + qdict_put_str(options, "server.host", server);<br>
> <br>
> - port_str = g_strdup_printf("%d", uri->port ?: 22);<br>
> + port_str = g_strdup_printf("%d", port ?: 22);<br>
> qdict_put_str(options, "server.port", port_str);<br>
> <br>
> - qdict_put_str(options, "path", uri->path);<br>
> + qdict_put_str(options, "path", path);<br>
> <br>
> /* Pick out any query parameters that we understand, and ignore<br>
> * the rest.<br>
> */<br>
> +#ifdef HAVE_GLIB_GURI<br>
> + g_hash_table_iter_init(&iter, params);<br>
> + while (g_hash_table_iter_next(&iter, (void **)&key, (void **)&value)) {<br>
> +#else<br>
> for (i = 0; i < qp->n; ++i) {<br>
> - if (strcmp(qp->p[i].name, "host_key_check") == 0) {<br>
> - qdict_put_str(options, "host_key_check", qp->p[i].value);<br>
> + key = qp->p[i].name;<br>
> + value = qp->p[i].value;<br>
> +#endif<br>
> + if (g_strcmp0(key, "host_key_check") == 0) {<br>
> + qdict_put_str(options, "host_key_check", value);<br>
> }<br>
> }<br>
> <br>
> -- <br>
> 2.27.0.221.ga08a83db2b<br>
<br>
-- <br>
Richard Jones, Virtualization Group, Red Hat <a href="http://people.redhat.com/~rjones" rel="noreferrer" target="_blank">http://people.redhat.com/~rjones</a><br>
Read my programming and virtualization blog: <a href="http://rwmj.wordpress.com" rel="noreferrer" target="_blank">http://rwmj.wordpress.com</a><br>
libguestfs lets you edit virtual machines. Supports shell scripting,<br>
bindings from many languages. <a href="http://libguestfs.org" rel="noreferrer" target="_blank">http://libguestfs.org</a><br>
<br>
<br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>