| Message ID | 20240328140607.2433889-7-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Switch to glib URI parsing code | expand |
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; > const char *p; > - QueryParams *qp = NULL; > + GHashTable *qp = NULL; > + 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); > if (!uri) { > return -EINVAL; > } > > /* transport */ > - if (!g_strcmp0(uri->scheme, "nbd")) { > + uri_scheme = g_uri_get_scheme(uri); > + if (!g_strcmp0(uri_scheme, "nbd")) { > is_unix = false; > - } else if (!g_strcmp0(uri->scheme, "nbd+tcp")) { > + } else if (!g_strcmp0(uri_scheme, "nbd+tcp")) { > is_unix = false; > - } else if (!g_strcmp0(uri->scheme, "nbd+unix")) { > + } else if (!g_strcmp0(uri_scheme, "nbd+unix")) { > is_unix = true; > } else { > ret = -EINVAL; > goto out; > } > > - p = uri->path ? uri->path : ""; > + p = g_uri_get_path(uri) ?: ""; > if (p[0] == '/') { > p++; > } > @@ -1545,51 +1548,58 @@ static int nbd_parse_uri(const char *filename, QDict *options) > qdict_put_str(options, "export", p); > } > > - qp = query_params_parse(uri->query); > - if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) { > - ret = -EINVAL; > - goto out; > + uri_query = g_uri_get_query(uri); > + if (uri_query) { > + qp = g_uri_parse_params(uri_query, -1, "&", G_URI_PARAMS_NONE, NULL); > + if (!qp) { > + ret = -EINVAL; > + goto out; > + } > + qp_n = g_hash_table_size(qp); > + if (qp_n > 1 || (is_unix && !qp_n) || (!is_unix && qp_n)) { > + ret = -EINVAL; > + goto out; > + } > + } > + > + uri_server = g_uri_get_host(uri); > + if (uri_server && !uri_server[0]) { > + uri_server = NULL; > } > + uri_port = g_uri_get_port(uri); > > if (is_unix) { > /* nbd+unix:///export?socket=path */ > - if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) { > + const char *uri_socket = g_hash_table_lookup(qp, "socket"); > + if (uri_server || uri_port != -1 || !uri_socket) { > ret = -EINVAL; > goto out; > } > qdict_put_str(options, "server.type", "unix"); > - qdict_put_str(options, "server.path", qp->p[0].value); > + qdict_put_str(options, "server.path", uri_socket); > } else { > - QString *host; > char *port_str; > > /* 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); > 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; > } Looks ok, Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
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.
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? > > > 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 ? With regards, Daniel
Adjusting cc list to add upstream NBD and drop developers unrelated to this part of the qemu series... On Thu, Mar 28, 2024 at 02:13:42PM +0000, Richard W.M. Jones 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. > > > > > > if (is_unix) { > > /* nbd+unix:///export?socket=path */ > > - if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) { > > + const char *uri_socket = g_hash_table_lookup(qp, "socket"); > > + if (uri_server || uri_port != -1 || !uri_socket) { > > ret = -EINVAL; > > goto out; > > } The spec for NBD URIs is at: https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md Should any of this spec mention case-insensitive concerns, such as whether 'NBD://' may be equivalent to 'nbd://', and whether 'nbd+unix:///?socket=x' is equivalent to 'nbd+unix:///?Socket=x'? Right now, I think that most implementations of NBD servers and clients happen to use case-sensitive parsing; but glib provides the option to do case-insensitive query parsing. If I read https://docs.gtk.org/glib/type_func.Uri.parse_params.html correctly, passing G_URI_PARAMS_CASE_INSENSITIVE (which you did not do) would mean that 'nbd+unix:///?socket=ignore&Socket=/for/real' would result in this g_hash_table_lookup finding only "Socket", not "socket". Maybe it is worth an explicit addition to the NBD URI spec to mention that we intend to be case-sensitive (in the parts where it can be; I'm not sure if the schema part must be handled case-insensitively without re-reading the RFCs), and therefore that 'Socket=' does NOT have the same meaning as 'socket='.
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
On Thu, Mar 28, 2024 at 10:06:01AM -0500, Eric Blake wrote: > Adjusting cc list to add upstream NBD and drop developers unrelated to > this part of the qemu series... > > On Thu, Mar 28, 2024 at 02:13:42PM +0000, Richard W.M. Jones 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. > > > > > > > > > > if (is_unix) { > > > /* nbd+unix:///export?socket=path */ > > > - if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) { > > > + const char *uri_socket = g_hash_table_lookup(qp, "socket"); > > > + if (uri_server || uri_port != -1 || !uri_socket) { > > > ret = -EINVAL; > > > goto out; > > > } > > The spec for NBD URIs is at: > > https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md > > Should any of this spec mention case-insensitive concerns, such as > whether 'NBD://' may be equivalent to 'nbd://', and whether > 'nbd+unix:///?socket=x' is equivalent to 'nbd+unix:///?Socket=x'? > Right now, I think that most implementations of NBD servers and > clients happen to use case-sensitive parsing; but glib provides the > option to do case-insensitive query parsing. I haven't thought about this before, but do note that the NBD URI spec defers to "IETF standards describing URIs" for all unanswered questions. RFC3986 does talk about this incidentally. About the scheme field it says (section 3.1): Although schemes are case- insensitive, the canonical form is lowercase and documents that specify schemes must do so with lowercase letters. An implementation should accept uppercase letters as equivalent to lowercase in scheme names (e.g., allow "HTTP" as well as "http") for the sake of robustness but should only produce lowercase scheme names for consistency. Hostname is also (obviously) case insensitive. There's also a section (6.2.3) which talks about normalization of URIs. Overall it seems the intention of the RFC writer is that parsers should handle any case; but when generating URIs (and for examples, documentation etc) we should only generate lowercase. libnbd absolutely does *not* get this right, eg: $ nbdinfo NBD://localhost nbdinfo: nbd_connect_uri: unknown NBD URI scheme: NBD: Invalid argument so that's a bug too. > If I read https://docs.gtk.org/glib/type_func.Uri.parse_params.html > correctly, passing G_URI_PARAMS_CASE_INSENSITIVE (which you did not > do) would mean that 'nbd+unix:///?socket=ignore&Socket=/for/real' > would result in this g_hash_table_lookup finding only "Socket", not > "socket". Maybe it is worth an explicit addition to the NBD URI spec > to mention that we intend to be case-sensitive (in the parts where it > can be; I'm not sure if the schema part must be handled > case-insensitively without re-reading the RFCs), and therefore that > 'Socket=' does NOT have the same meaning as 'socket='. We could mention this in the spec for clarity, but the current meaning (as above) would be that case-insensitive parsing is recommended. Rich.
On Thu, Mar 28, 2024 at 04:40:10PM +0000, Richard W.M. Jones wrote: > libnbd absolutely does *not* get this right, eg: > > $ nbdinfo NBD://localhost > nbdinfo: nbd_connect_uri: unknown NBD URI scheme: NBD: Invalid argument > > so that's a bug too. Proposed fix: https://gitlab.com/nbdkit/libnbd/-/merge_requests/6 Rich.
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; const char *p; - QueryParams *qp = NULL; + GHashTable *qp = NULL; + 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); if (!uri) { return -EINVAL; } /* transport */ - if (!g_strcmp0(uri->scheme, "nbd")) { + uri_scheme = g_uri_get_scheme(uri); + if (!g_strcmp0(uri_scheme, "nbd")) { is_unix = false; - } else if (!g_strcmp0(uri->scheme, "nbd+tcp")) { + } else if (!g_strcmp0(uri_scheme, "nbd+tcp")) { is_unix = false; - } else if (!g_strcmp0(uri->scheme, "nbd+unix")) { + } else if (!g_strcmp0(uri_scheme, "nbd+unix")) { is_unix = true; } else { ret = -EINVAL; goto out; } - p = uri->path ? uri->path : ""; + p = g_uri_get_path(uri) ?: ""; if (p[0] == '/') { p++; } @@ -1545,51 +1548,58 @@ static int nbd_parse_uri(const char *filename, QDict *options) qdict_put_str(options, "export", p); } - qp = query_params_parse(uri->query); - if (qp->n > 1 || (is_unix && !qp->n) || (!is_unix && qp->n)) { - ret = -EINVAL; - goto out; + uri_query = g_uri_get_query(uri); + if (uri_query) { + qp = g_uri_parse_params(uri_query, -1, "&", G_URI_PARAMS_NONE, NULL); + if (!qp) { + ret = -EINVAL; + goto out; + } + qp_n = g_hash_table_size(qp); + if (qp_n > 1 || (is_unix && !qp_n) || (!is_unix && qp_n)) { + ret = -EINVAL; + goto out; + } + } + + uri_server = g_uri_get_host(uri); + if (uri_server && !uri_server[0]) { + uri_server = NULL; } + uri_port = g_uri_get_port(uri); if (is_unix) { /* nbd+unix:///export?socket=path */ - if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) { + const char *uri_socket = g_hash_table_lookup(qp, "socket"); + if (uri_server || uri_port != -1 || !uri_socket) { ret = -EINVAL; goto out; } qdict_put_str(options, "server.type", "unix"); - qdict_put_str(options, "server.path", qp->p[0].value); + qdict_put_str(options, "server.path", uri_socket); } else { - QString *host; char *port_str; /* 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); 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; }
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(-)