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;
}
--
2.44.0On 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
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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 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>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
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.
> >
> > 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>
Or maybe not. This caused a regression in the nbdkit test suite (when
we use qemu-img from 9.1). It seems the exportname part of the NBD
URI gets munged:
https://gitlab.com/qemu-project/qemu/-/issues/2584
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
On Sun, Sep 22, 2024 at 08:51:22PM GMT, Richard W.M. Jones wrote:
> 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.
> > >
> > >
> > > - p = uri->path ? uri->path : "";
> > > + p = g_uri_get_path(uri) ?: "";
> > > if (p[0] == '/') {
> > > p++;
> > > }
> > Looks ok,
> >
> > Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
>
> Or maybe not. This caused a regression in the nbdkit test suite (when
> we use qemu-img from 9.1). It seems the exportname part of the NBD
> URI gets munged:
>
> https://gitlab.com/qemu-project/qemu/-/issues/2584
To be more specific, it looks like
g_uri_get_path("./name//with//..//slashes") is getting munged to
"name/slashes". That is, glib is blindly assuming that ./ and XXX/../
can be dropped, and // can be simplified to /, which may be true for
arbitrary file names but not true for abitrary URIs (since URIs have
application-specific semantics, which may not match path name
traversal semantics). Looks like we need to report a bug to glib,
and/or see if glib's URI functions have a flag for turning off this
unwanted munging.
Or we may just want to document this corner case change as
intentional.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
On 23/09/2024 18.03, Eric Blake wrote:
> On Sun, Sep 22, 2024 at 08:51:22PM GMT, Richard W.M. Jones wrote:
>> 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.
>>>>
>
>>>>
>>>> - p = uri->path ? uri->path : "";
>>>> + p = g_uri_get_path(uri) ?: "";
>>>> if (p[0] == '/') {
>>>> p++;
>>>> }
>
>>> Looks ok,
>>>
>>> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
>>
>> Or maybe not. This caused a regression in the nbdkit test suite (when
>> we use qemu-img from 9.1). It seems the exportname part of the NBD
>> URI gets munged:
>>
>> https://gitlab.com/qemu-project/qemu/-/issues/2584
>
> To be more specific, it looks like
> g_uri_get_path("./name//with//..//slashes") is getting munged to
> "name/slashes". That is, glib is blindly assuming that ./ and XXX/../
> can be dropped, and // can be simplified to /, which may be true for
> arbitrary file names but not true for abitrary URIs (since URIs have
> application-specific semantics, which may not match path name
> traversal semantics). Looks like we need to report a bug to glib,
> and/or see if glib's URI functions have a flag for turning off this
> unwanted munging.
>
> Or we may just want to document this corner case change as
> intentional.
Ok ... so how bad is this for NBD? Can we go along with the odditiy or is
this breaking some real world NBD scenarios?
... in the worst case, we have to revert the patch ...
Thomas
On Tue, Sep 24, 2024 at 09:52:04AM +0200, Thomas Huth wrote:
> On 23/09/2024 18.03, Eric Blake wrote:
> >On Sun, Sep 22, 2024 at 08:51:22PM GMT, Richard W.M. Jones wrote:
> >>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.
> >>>>
> >
> >>>>- p = uri->path ? uri->path : "";
> >>>>+ p = g_uri_get_path(uri) ?: "";
> >>>> if (p[0] == '/') {
> >>>> p++;
> >>>> }
> >
> >>>Looks ok,
> >>>
> >>>Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> >>
> >>Or maybe not. This caused a regression in the nbdkit test suite (when
> >>we use qemu-img from 9.1). It seems the exportname part of the NBD
> >>URI gets munged:
> >>
> >>https://gitlab.com/qemu-project/qemu/-/issues/2584
> >
> >To be more specific, it looks like
> >g_uri_get_path("./name//with//..//slashes") is getting munged to
> >"name/slashes". That is, glib is blindly assuming that ./ and XXX/../
> >can be dropped, and // can be simplified to /, which may be true for
> >arbitrary file names but not true for abitrary URIs (since URIs have
> >application-specific semantics, which may not match path name
> >traversal semantics). Looks like we need to report a bug to glib,
> >and/or see if glib's URI functions have a flag for turning off this
> >unwanted munging.
> >
> >Or we may just want to document this corner case change as
> >intentional.
>
> Ok ... so how bad is this for NBD? Can we go along with the odditiy
> or is this breaking some real world NBD scenarios?
After sleeping on it, I don't think it's serious at all. It's also
permitted (albeit not required) by the URI RFCs, so other
implementations in future might do the same thing.
> ... in the worst case, we have to revert the patch ...
I closed the bug just now and have decided to go with a documentation
update instead.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
On Mon, Sep 23, 2024 at 11:03:08AM -0500, Eric Blake wrote:
> On Sun, Sep 22, 2024 at 08:51:22PM GMT, Richard W.M. Jones wrote:
> > 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.
> > > >
>
> > > >
> > > > - p = uri->path ? uri->path : "";
> > > > + p = g_uri_get_path(uri) ?: "";
> > > > if (p[0] == '/') {
> > > > p++;
> > > > }
>
> > > Looks ok,
> > >
> > > Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> >
> > Or maybe not. This caused a regression in the nbdkit test suite (when
> > we use qemu-img from 9.1). It seems the exportname part of the NBD
> > URI gets munged:
> >
> > https://gitlab.com/qemu-project/qemu/-/issues/2584
>
> To be more specific, it looks like
> g_uri_get_path("./name//with//..//slashes") is getting munged to
> "name/slashes". That is, glib is blindly assuming that ./ and XXX/../
> can be dropped, and // can be simplified to /, which may be true for
> arbitrary file names but not true for abitrary URIs (since URIs have
> application-specific semantics, which may not match path name
> traversal semantics). Looks like we need to report a bug to glib,
> and/or see if glib's URI functions have a flag for turning off this
> unwanted munging.
The source code indicates it is doing some normalization
based on this:
https://datatracker.ietf.org/doc/html/rfc3986#section-5.2.4
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Sep 23, 2024 at 06:38:27PM +0200, Daniel P. Berrangé wrote:
> On Mon, Sep 23, 2024 at 11:03:08AM -0500, Eric Blake wrote:
> > On Sun, Sep 22, 2024 at 08:51:22PM GMT, Richard W.M. Jones wrote:
> > > 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.
> > > > >
> >
> > > > >
> > > > > - p = uri->path ? uri->path : "";
> > > > > + p = g_uri_get_path(uri) ?: "";
> > > > > if (p[0] == '/') {
> > > > > p++;
> > > > > }
> >
> > > > Looks ok,
> > > >
> > > > Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> > >
> > > Or maybe not. This caused a regression in the nbdkit test suite (when
> > > we use qemu-img from 9.1). It seems the exportname part of the NBD
> > > URI gets munged:
> > >
> > > https://gitlab.com/qemu-project/qemu/-/issues/2584
> >
> > To be more specific, it looks like
> > g_uri_get_path("./name//with//..//slashes") is getting munged to
> > "name/slashes". That is, glib is blindly assuming that ./ and XXX/../
> > can be dropped, and // can be simplified to /, which may be true for
> > arbitrary file names but not true for abitrary URIs (since URIs have
> > application-specific semantics, which may not match path name
> > traversal semantics). Looks like we need to report a bug to glib,
> > and/or see if glib's URI functions have a flag for turning off this
> > unwanted munging.
>
> The source code indicates it is doing some normalization
> based on this:
>
> https://datatracker.ietf.org/doc/html/rfc3986#section-5.2.4
I wrote a bit about this in the bug:
https://gitlab.com/qemu-project/qemu/-/issues/2584#note_2125192404
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
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='.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
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.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
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. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
© 2016 - 2026 Red Hat, Inc.