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

["Richard W.M. Jones" <rjones@redhat.com>]

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