|
From: | Thomas Huth |
Subject: | Re: [PATCH for-9.1 6/9] block/nbd: Use URI parsing code from glib |
Date: | Thu, 28 Mar 2024 16:34:54 +0100 |
User-agent: | Mozilla Thunderbird |
On 28/03/2024 15.59, Daniel P. Berrangé wrote:
On Thu, Mar 28, 2024 at 09:54:49AM -0500, Eric Blake wrote: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?
Sounds like a good idea, I'll give it a try!
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.Passing '0' will cause the kernel to allocate a random free port, so that is potentially introducing new semantics ?
Ok, so passing a 0 does not really make sense, since QEMU needs to know the exact port. I'll change the check from "uri_port != -1" to "uri_port > 0" in the next version.
Thomas
[Prev in Thread] | Current Thread | [Next in Thread] |