Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib
On 12/04/2024 16.40, Eric Blake wrote: On Fri, Apr 12, 2024 at 03:24:11PM +0200, 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 --- block/gluster.c | 71 - 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index cc74af06dc..1c9505f8bb 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -17,7 +17,6 @@ #include "qapi/error.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" -#include "qemu/uri.h" #include "qemu/error-report.h" #include "qemu/module.h" #include "qemu/option.h" @@ -289,9 +288,9 @@ static void glfs_clear_preopened(glfs_t *fs) } } -static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) +static int parse_volume_options(BlockdevOptionsGluster *gconf, const char *path) Is it worth mentioning in the commit message that this includes a const-correctness tweak? I can add something like: "Since g_uri_get_path() returns a const pointer, we also need to tweak the parameter of parse_volume_options() (where we use the result of g_uri_get_path() as input)" @@ -364,57 +363,57 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf, QAPI_LIST_PREPEND(gconf->server, gsconf); /* transport */ -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { +uri_scheme = g_uri_get_scheme(uri); +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) { Pre-existing, but per RFC 3986, we should probably be using strcasecmp for scheme comparisons (I'm not sure if g_uri_parse guarantees a lower-case return, even when the user passed in upper case). That can be a separate patch. As Daniel mentioned, g_uri_get_scheme() returns a lowercase string, so we should be fine. Reviewed-by: Eric Blake Thanks! Thomas
Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib
On Fri, Apr 12, 2024 at 09:40:11AM -0500, Eric Blake wrote: > On Fri, Apr 12, 2024 at 03:24:11PM +0200, 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 > > --- > > block/gluster.c | 71 - > > 1 file changed, 35 insertions(+), 36 deletions(-) > > > > diff --git a/block/gluster.c b/block/gluster.c > > index cc74af06dc..1c9505f8bb 100644 > > --- a/block/gluster.c > > +++ b/block/gluster.c > > @@ -17,7 +17,6 @@ > > #include "qapi/error.h" > > #include "qapi/qmp/qdict.h" > > #include "qapi/qmp/qerror.h" > > -#include "qemu/uri.h" > > #include "qemu/error-report.h" > > #include "qemu/module.h" > > #include "qemu/option.h" > > @@ -289,9 +288,9 @@ static void glfs_clear_preopened(glfs_t *fs) > > } > > } > > > > -static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) > > +static int parse_volume_options(BlockdevOptionsGluster *gconf, const char > > *path) > > Is it worth mentioning in the commit message that this includes a > const-correctness tweak? > > > @@ -364,57 +363,57 @@ static int > > qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf, > > QAPI_LIST_PREPEND(gconf->server, gsconf); > > > > /* transport */ > > -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { > > +uri_scheme = g_uri_get_scheme(uri); > > +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) { > > Pre-existing, but per RFC 3986, we should probably be using strcasecmp > for scheme comparisons (I'm not sure if g_uri_parse guarantees a > lower-case return, even when the user passed in upper case). That can > be a separate patch. Docs say it is lowercase: https://developer-old.gnome.org/glib/stable/glib-URI-Functions.html "on return, contains the scheme (converted to lowercase), or NULL." 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 :|
Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib
On Fri, Apr 12, 2024 at 03:24:11PM +0200, 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 > --- > block/gluster.c | 71 - > 1 file changed, 35 insertions(+), 36 deletions(-) Reviewed-by: Daniel P. Berrangé 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 :|
Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib
On Fri, Apr 12, 2024 at 09:40:18AM -0500, Eric Blake wrote: > > @@ -364,57 +363,57 @@ static int > > qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf, > > QAPI_LIST_PREPEND(gconf->server, gsconf); > > > > /* transport */ > > -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { > > +uri_scheme = g_uri_get_scheme(uri); > > +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) { > > Pre-existing, but per RFC 3986, we should probably be using strcasecmp > for scheme comparisons (I'm not sure if g_uri_parse guarantees a > lower-case return, even when the user passed in upper case). That can > be a separate patch. Even beter, g_ascii_strcasecmp() (since strcasecmp can be locale-specific which is generally not what we need here) -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib
On Fri, Apr 12, 2024 at 03:24:11PM +0200, 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 > --- > block/gluster.c | 71 - > 1 file changed, 35 insertions(+), 36 deletions(-) > > diff --git a/block/gluster.c b/block/gluster.c > index cc74af06dc..1c9505f8bb 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -17,7 +17,6 @@ > #include "qapi/error.h" > #include "qapi/qmp/qdict.h" > #include "qapi/qmp/qerror.h" > -#include "qemu/uri.h" > #include "qemu/error-report.h" > #include "qemu/module.h" > #include "qemu/option.h" > @@ -289,9 +288,9 @@ static void glfs_clear_preopened(glfs_t *fs) > } > } > > -static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) > +static int parse_volume_options(BlockdevOptionsGluster *gconf, const char > *path) Is it worth mentioning in the commit message that this includes a const-correctness tweak? > @@ -364,57 +363,57 @@ static int > qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf, > QAPI_LIST_PREPEND(gconf->server, gsconf); > > /* transport */ > -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { > +uri_scheme = g_uri_get_scheme(uri); > +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) { Pre-existing, but per RFC 3986, we should probably be using strcasecmp for scheme comparisons (I'm not sure if g_uri_parse guarantees a lower-case return, even when the user passed in upper case). That can be a separate patch. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org