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

[Eric Blake]

On Thu, Mar 28, 2024 at 03:06:03PM +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. The g_uri_get_host() also takes care
> of removing the square brackets from IPv6 addresses, so we can
> drop that part of the QEMU code now, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  block/nbd.c | 66 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index ef05f7cdfd..95b507f872 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -31,7 +31,6 @@
>  #include "qemu/osdep.h"
>  
>  #include "trace.h"
> -#include "qemu/uri.h"
>  #include "qemu/option.h"
>  #include "qemu/cutils.h"
>  #include "qemu/main-loop.h"
> @@ -1514,30 +1513,34 @@ static void nbd_client_close(BlockDriverState *bs)
>  
>  static int nbd_parse_uri(const char *filename, QDict *options)
>  {
> -    URI *uri;
> +    GUri *uri;

Is it worth using 'g_autoptr(GUri) uri = NULL;' here, to simplify cleanup later?

>      const char *p;
> -    QueryParams *qp = NULL;
> +    GHashTable *qp = NULL;

Presumably would be easier if qp is also auto-free.

> +    int qp_n;
>      int ret = 0;
>      bool is_unix;
> +    const char *uri_scheme, *uri_query, *uri_server;
> +    int uri_port;
>  
> -    uri = uri_parse(filename);
> +    uri = g_uri_parse(filename, G_URI_FLAGS_NONE, NULL);

The glib API is fairly close to what we have in qemu, making this a
nice switchover.

>          /* nbd[+tcp]://host[:port]/export */
> -        if (!uri->server) {
> +        if (!uri_server) {
>              ret = -EINVAL;
>              goto out;
>          }
>  
> -        /* strip braces from literal IPv6 address */
> -        if (uri->server[0] == '[') {
> -            host = qstring_from_substr(uri->server, 1,
> -                                       strlen(uri->server) - 1);
> -        } else {
> -            host = qstring_from_str(uri->server);
> -        }
> -
>          qdict_put_str(options, "server.type", "inet");
> -        qdict_put(options, "server.host", host);
> +        qdict_put_str(options, "server.host", uri_server);
>  
> -        port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
> +        port_str = g_strdup_printf("%d", uri_port != -1 ? uri_port
> +                                                        : NBD_DEFAULT_PORT);
>          qdict_put_str(options, "server.port", port_str);

If a user requests nbd://hostname:0/export, this now sets server.port
to "0" instead of "10809".  Is that an intentional change?  No one
actually passes an explicit ":0" port on purpose, but we do have to
worry about malicious URIs.

>          g_free(port_str);
>      }
>  
>  out:
>      if (qp) {
> -        query_params_free(qp);
> +        g_hash_table_destroy(qp);
>      }
> -    uri_free(uri);
> +    g_uri_unref(uri);
>      return ret;

It may be possible to eliminate the out label altogether, if
GHashTable has the appropriate auto-free magic.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org