[PATCH] nbd-client: Support leading / in NBD URI

[PATCH] nbd-client: Support leading / in NBD URI

[Eric Blake]

The NBD URI specification [1] states that only one leading slash at
the beginning of the URI path component is stripped, not all such
slashes.  This becomes important to a patch I just proposed to nbdkit
[2], which would allow the exportname to select a file embedded within
an ext2 image: ext2fs demands an absolute pathname beginning with '/',
and because qemu was inadvertantly stripping it, my nbdkit patch had
to work around the behavior.

[1] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md
[2] https://www.redhat.com/archives/libguestfs/2020-February/msg00109.html

Note that the qemu bug only affects handling of URIs such as
nbd://host:port//abs/path (where '/abs/path' should be the export
name); it is still possible to use --image-opts and pass the desired
export name with a leading slash directly through JSON even without
this patch.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index d085554f21ea..82f9b7ef50a5 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1516,8 +1516,10 @@ static int nbd_parse_uri(const char *filename, QDict *options)
         goto out;
     }

-    p = uri->path ? uri->path : "/";
-    p += strspn(p, "/");
+    p = uri->path ? uri->path : "";
+    if (p[0] == '/') {
+        p++;
+    }
     if (p[0]) {
         qdict_put_str(options, "export", p);
     }
-- 
2.24.1

Re: [PATCH] nbd-client: Support leading / in NBD URI

[Ján Tomko]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Tue, Feb 11, 2020 at 08:31:01PM -0600, Eric Blake wrote:
>The NBD URI specification [1] states that only one leading slash at
>the beginning of the URI path component is stripped, not all such
>slashes.  This becomes important to a patch I just proposed to nbdkit
>[2], which would allow the exportname to select a file embedded within
>an ext2 image: ext2fs demands an absolute pathname beginning with '/',
>and because qemu was inadvertantly stripping it, my nbdkit patch had
>to work around the behavior.
>
>[1] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md
>[2] https://www.redhat.com/archives/libguestfs/2020-February/msg00109.html
>
>Note that the qemu bug only affects handling of URIs such as
>nbd://host:port//abs/path (where '/abs/path' should be the export
>name); it is still possible to use --image-opts and pass the desired
>export name with a leading slash directly through JSON even without
>this patch.
>
>Signed-off-by: Eric Blake <eblake@redhat.com>
>---
> block/nbd.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] nbd-client: Support leading / in NBD URI

[Richard W.M. Jones]

On Tue, Feb 11, 2020 at 08:31:01PM -0600, Eric Blake wrote:
> The NBD URI specification [1] states that only one leading slash at
> the beginning of the URI path component is stripped, not all such
> slashes.  This becomes important to a patch I just proposed to nbdkit
> [2], which would allow the exportname to select a file embedded within
> an ext2 image: ext2fs demands an absolute pathname beginning with '/',
> and because qemu was inadvertantly stripping it, my nbdkit patch had
> to work around the behavior.
> 
> [1] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md
> [2] https://www.redhat.com/archives/libguestfs/2020-February/msg00109.html
> 
> Note that the qemu bug only affects handling of URIs such as
> nbd://host:port//abs/path (where '/abs/path' should be the export
> name); it is still possible to use --image-opts and pass the desired
> export name with a leading slash directly through JSON even without
> this patch.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/nbd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index d085554f21ea..82f9b7ef50a5 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1516,8 +1516,10 @@ static int nbd_parse_uri(const char *filename, QDict *options)
>          goto out;
>      }
> 
> -    p = uri->path ? uri->path : "/";
> -    p += strspn(p, "/");
> +    p = uri->path ? uri->path : "";
> +    if (p[0] == '/') {
> +        p++;
> +    }
>      if (p[0]) {
>          qdict_put_str(options, "export", p);
>      }

Looks reasonable, ACK.

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

Re: [PATCH] nbd-client: Support leading / in NBD URI

[Maxim Levitsky]

On Wed, 2020-02-12 at 14:33 +0100, Ján Tomko wrote:
> On Tue, Feb 11, 2020 at 08:31:01PM -0600, Eric Blake wrote:
> > The NBD URI specification [1] states that only one leading slash at
> > the beginning of the URI path component is stripped, not all such
> > slashes.  This becomes important to a patch I just proposed to nbdkit
> > [2], which would allow the exportname to select a file embedded within
> > an ext2 image: ext2fs demands an absolute pathname beginning with '/',
> > and because qemu was inadvertantly stripping it, my nbdkit patch had
> > to work around the behavior.
> > 
> > [1] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md
> > [2] https://www.redhat.com/archives/libguestfs/2020-February/msg00109.html
> > 
> > Note that the qemu bug only affects handling of URIs such as
> > nbd://host:port//abs/path (where '/abs/path' should be the export
> > name); it is still possible to use --image-opts and pass the desired
> > export name with a leading slash directly through JSON even without
> > this patch.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > block/nbd.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> 
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> 
> Jano
Note that I had a bug open for this.
https://bugzilla.redhat.com/show_bug.cgi?id=1728545

I expected this to be a feature to be honest,
and was afraid to break existing users that might rely on this.

Best regards,
	Maxim Levitsky