Re: [PATCH for-9.1 8/9] block/ssh: Use URI parsing code from glib

From: "Richard W.M. Jones" <rjones@redhat.com>
To: Thomas Huth <thuth@redhat.com>
Cc: "Alex Bennée" <alex.bennee@linaro.org>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Eric Blake" <eblake@redhat.com>,
	"Vladimir Sementsov-Ogievskiy" <vsementsov@yandex-team.ru>,
	"Peter Lieven" <pl@kamp.de>,
	qemu-devel@nongnu.org, "Kevin Wolf" <kwolf@redhat.com>,
	"Hanna Reitz" <hreitz@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Konstantin Kostiuk" <kkostiuk@redhat.com>,
	qemu-block@nongnu.org
Subject: Re: [PATCH for-9.1 8/9] block/ssh: Use URI parsing code from glib
Date: Thu, 28 Mar 2024 14:15:01 +0000	[thread overview]
Message-ID: <20240328141501.GL7636@redhat.com> (raw)
In-Reply-To: <20240328140607.2433889-9-thuth@redhat.com>

On Thu, Mar 28, 2024 at 03:06:05PM +0100, Thomas Huth wrote:
> Since version 2.66, glib has useful URI parsing functions, too.
> Use those instead of the QEMU-internal ones to be finally able
> to get rid of the latter.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  block/ssh.c | 69 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 2748253d4a..c0caf59793 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -37,7 +37,6 @@
>  #include "qemu/ctype.h"
>  #include "qemu/cutils.h"
>  #include "qemu/sockets.h"
> -#include "qemu/uri.h"
>  #include "qapi/qapi-visit-sockets.h"
>  #include "qapi/qapi-visit-block-core.h"
>  #include "qapi/qmp/qdict.h"
> @@ -181,64 +180,76 @@ 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;
> +    GUri *uri;
> +    const char *uri_host, *uri_path, *uri_user, *uri_query;
>      char *port_str;
> -    int i;
> +    int port;
> +    g_autoptr(GError) gerror = NULL;
> +    char *qp_name, *qp_value;
> +    GUriParamsIter qp;
>  
> -    uri = uri_parse(filename);
> +    uri = g_uri_parse(filename, G_URI_FLAGS_NONE, NULL);
>      if (!uri) {
>          return -EINVAL;
>      }
>  
> -    if (g_strcmp0(uri->scheme, "ssh") != 0) {
> +    if (g_strcmp0(g_uri_get_scheme(uri), "ssh") != 0) {
>          error_setg(errp, "URI scheme must be 'ssh'");
>          goto err;
>      }
>  
> -    if (!uri->server || strcmp(uri->server, "") == 0) {
> +    uri_host = g_uri_get_host(uri);
> +    if (!uri_host || g_str_equal(uri_host, "")) {
>          error_setg(errp, "missing hostname in URI");
>          goto err;
>      }
>  
> -    if (!uri->path || strcmp(uri->path, "") == 0) {
> +    uri_path = g_uri_get_path(uri);
> +    if (!uri_path || g_str_equal(uri_path, "")) {
>          error_setg(errp, "missing remote path in URI");
>          goto err;
>      }
>  
> -    qp = query_params_parse(uri->query);
> -    if (!qp) {
> -        error_setg(errp, "could not parse query parameters");
> -        goto err;
> -    }
> -
> -    if(uri->user && strcmp(uri->user, "") != 0) {
> -        qdict_put_str(options, "user", uri->user);
> +    uri_user = g_uri_get_user(uri);
> +    if (uri_user && !g_str_equal(uri_user, "")) {
> +        qdict_put_str(options, "user", uri_user);
>      }
>  
> -    qdict_put_str(options, "server.host", uri->server);
> +    qdict_put_str(options, "server.host", uri_host);
>  
> -    port_str = g_strdup_printf("%d", uri->port ?: 22);
> +    port = g_uri_get_port(uri);
> +    port_str = g_strdup_printf("%d", port != -1 ? port : 22);
>      qdict_put_str(options, "server.port", port_str);
>      g_free(port_str);
>  
> -    qdict_put_str(options, "path", uri->path);
> -
> -    /* Pick out any query parameters that we understand, and ignore
> -     * the rest.
> -     */
> -    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);
> +    qdict_put_str(options, "path", uri_path);
> +
> +    uri_query = g_uri_get_query(uri);
> +    if (uri_query) {
> +        g_uri_params_iter_init(&qp, uri_query, -1, "&", G_URI_PARAMS_NONE);
> +        while (g_uri_params_iter_next(&qp, &qp_name, &qp_value, &gerror)) {
> +            if (!qp_name || !qp_value || gerror) {
> +                warn_report("Failed to parse SSH URI parameters '%s'.",
> +                            uri_query);
> +                break;
> +            }
> +            /*
> +             * Pick out the query parameters that we understand, and ignore
> +             * (or rather warn about) the rest.
> +             */
> +            if (g_str_equal(qp_name, "host_key_check")) {
> +                qdict_put_str(options, "host_key_check", qp_value);
> +            } else {
> +                warn_report("Unsupported parameter '%s' in URI.", qp_name);
> +            }
>          }
>      }
>  
> -    query_params_free(qp);
> -    uri_free(uri);
> +    g_uri_unref(uri);
>      return 0;
>  
>   err:
> -    uri_free(uri);
> +    g_uri_unref(uri);
>      return -EINVAL;
>  }

Seems reasonable too,

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v