util: storagefile: Properly set transport type when parsing NBD strings

[libvirt] [PATCH] util: storagefile: Properly set transport type when parsing NBD strings

Posted by Peter Krempa 4 years, 3 months ago

When parsing legacy NBD backing file strings such as
'nbd:unix:/tmp/sock:exportname=/' we'd fail to set the transport to
VIR_STORAGE_NET_HOST_TRANS_UNIX. This started to be a problem once we
actually started to generate config of the backing store on the command
line with -blockdev as the JSON code would try to format it as TCP and
fail with:

 internal error: argument key 'host' must not have null value

Set the type properly and add a test.

This bug was found by the libguestfs test suite in:

https://bugzilla.redhat.com/show_bug.cgi?id=1791614

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/util/virstoragefile.c | 2 +-
 tests/virstoragetest.c    | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 1397f532fd..7a2af0ad94 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2964,7 +2964,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr,
         }

         src->hosts->socket = g_strdup(backing[2]);
-
+        src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_UNIX;
    } else {
         src->hosts->name = g_strdup(backing[1]);

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 2862758752..370e19252b 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1258,6 +1258,10 @@ mymain(void)
                        "<source protocol='nbd' name=':test'>\n"
                        "  <host name='example.org' port='6000'/>\n"
                        "</source>\n");
+    TEST_BACKING_PARSE("nbd:unix:/tmp/sock:exportname=/",
+                       "<source protocol='nbd' name='/'>\n"
+                       "  <host transport='unix' socket='/tmp/sock'/>\n"
+                       "</source>\n");
     TEST_BACKING_PARSE("nbd://example.org:1234",
                        "<source protocol='nbd'>\n"
                        "  <host name='example.org' port='1234'/>\n"
-- 
2.24.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: storagefile: Properly set transport type when parsing NBD strings

Posted by Richard W.M. Jones 4 years, 3 months ago

On Thu, Jan 16, 2020 at 12:41:37PM +0100, Peter Krempa wrote:
> When parsing legacy NBD backing file strings such as
> 'nbd:unix:/tmp/sock:exportname=/' we'd fail to set the transport to
> VIR_STORAGE_NET_HOST_TRANS_UNIX. This started to be a problem once we
> actually started to generate config of the backing store on the command
> line with -blockdev as the JSON code would try to format it as TCP and
> fail with:
> 
>  internal error: argument key 'host' must not have null value
> 
> Set the type properly and add a test.
> 
> This bug was found by the libguestfs test suite in:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1791614

The bug was found by Ming Xie of the virt-v2v QE team.

The patch itself looks fine as far as I can tell.  But I'm having real
problems actually testing it.  Can you suggest any way to test libvirt
parsing these URIs?

My current method (which doesn't work for reasons that I don't
understand) is:

(1) Compile libvirt from source.

(2) Run ./run src/libvirtd &

(3) Create an NBD server + overlay file:

rm /tmp/sock
nbdkit -U /tmp/sock memory 1G
qemu-img create -f qcow2 overlay.qcow2 -b nbd:unix:/tmp/sock:exportname=/ -F raw

(4) Try to boot a libvirt guest using the overlay:

virt-install --import --name test --disk path=overlay.qcow2,format=raw --memory 1024

But for some reason libvirt just ignores the overlay:

2020-01-16 13:00:44.980+0000: 2378711: warning : virStorageBackendVolOpen:1527 : ignoring missing file 'nbd:unix:/tmp/sock'

I verified that the overlay works in programs like qemu-img so it
doesn't seem to be a problem with the overlay itself.

Rich.

> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/util/virstoragefile.c | 2 +-
>  tests/virstoragetest.c    | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 1397f532fd..7a2af0ad94 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2964,7 +2964,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr,
>          }
> 
>          src->hosts->socket = g_strdup(backing[2]);
> -
> +        src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_UNIX;
>     } else {
>          src->hosts->name = g_strdup(backing[1]);
> 
> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
> index 2862758752..370e19252b 100644
> --- a/tests/virstoragetest.c
> +++ b/tests/virstoragetest.c
> @@ -1258,6 +1258,10 @@ mymain(void)
>                         "<source protocol='nbd' name=':test'>\n"
>                         "  <host name='example.org' port='6000'/>\n"
>                         "</source>\n");
> +    TEST_BACKING_PARSE("nbd:unix:/tmp/sock:exportname=/",
> +                       "<source protocol='nbd' name='/'>\n"
> +                       "  <host transport='unix' socket='/tmp/sock'/>\n"
> +                       "</source>\n");
>      TEST_BACKING_PARSE("nbd://example.org:1234",
>                         "<source protocol='nbd'>\n"
>                         "  <host name='example.org' port='1234'/>\n"
> -- 
> 2.24.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: storagefile: Properly set transport type when parsing NBD strings

Posted by Richard W.M. Jones 4 years, 3 months ago

On Thu, Jan 16, 2020 at 01:13:38PM +0000, Richard W.M. Jones wrote:
> On Thu, Jan 16, 2020 at 12:41:37PM +0100, Peter Krempa wrote:
> > When parsing legacy NBD backing file strings such as
> > 'nbd:unix:/tmp/sock:exportname=/' we'd fail to set the transport to
> > VIR_STORAGE_NET_HOST_TRANS_UNIX. This started to be a problem once we
> > actually started to generate config of the backing store on the command
> > line with -blockdev as the JSON code would try to format it as TCP and
> > fail with:
> > 
> >  internal error: argument key 'host' must not have null value
> > 
> > Set the type properly and add a test.
> > 
> > This bug was found by the libguestfs test suite in:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1791614
> 
> The bug was found by Ming Xie of the virt-v2v QE team.
> 
> The patch itself looks fine as far as I can tell.  But I'm having real
> problems actually testing it.  Can you suggest any way to test libvirt
> parsing these URIs?
> 
> My current method (which doesn't work for reasons that I don't
> understand) is:
> 
> (1) Compile libvirt from source.
> 
> (2) Run ./run src/libvirtd &
> 
> (3) Create an NBD server + overlay file:
> 
> rm /tmp/sock
> nbdkit -U /tmp/sock memory 1G
> qemu-img create -f qcow2 overlay.qcow2 -b nbd:unix:/tmp/sock:exportname=/ -F raw
> 
> (4) Try to boot a libvirt guest using the overlay:
> 
> virt-install --import --name test --disk path=overlay.qcow2,format=raw --memory 1024
> 
> But for some reason libvirt just ignores the overlay:
> 
> 2020-01-16 13:00:44.980+0000: 2378711: warning : virStorageBackendVolOpen:1527 : ignoring missing file 'nbd:unix:/tmp/sock'
> 
> I verified that the overlay works in programs like qemu-img so it
> doesn't seem to be a problem with the overlay itself.

The problem was too old qemu.  With qemu 4.2 I can verify that
the patch is working, so:

Tested-by: Richard W.M. Jones <rjones@redhat.com>

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: [libvirt] [PATCH] util: storagefile: Properly set transport type when parsing NBD strings

Posted by Peter Krempa 4 years, 3 months ago

On Thu, Jan 16, 2020 at 13:13:38 +0000, Richard W.M. Jones wrote:
> On Thu, Jan 16, 2020 at 12:41:37PM +0100, Peter Krempa wrote:
> > When parsing legacy NBD backing file strings such as
> > 'nbd:unix:/tmp/sock:exportname=/' we'd fail to set the transport to
> > VIR_STORAGE_NET_HOST_TRANS_UNIX. This started to be a problem once we
> > actually started to generate config of the backing store on the command
> > line with -blockdev as the JSON code would try to format it as TCP and
> > fail with:
> > 
> >  internal error: argument key 'host' must not have null value
> > 
> > Set the type properly and add a test.
> > 
> > This bug was found by the libguestfs test suite in:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1791614
> 
> The bug was found by Ming Xie of the virt-v2v QE team.

I'll add a

Reported-by: Ming Xie <mxie@redhat.com>

> 
> The patch itself looks fine as far as I can tell.  But I'm having real
> problems actually testing it.  Can you suggest any way to test libvirt
> parsing these URIs?
> 
> My current method (which doesn't work for reasons that I don't
> understand) is:
> 
> (1) Compile libvirt from source.
> 
> (2) Run ./run src/libvirtd &
> 
> (3) Create an NBD server + overlay file:
> 
> rm /tmp/sock
> nbdkit -U /tmp/sock memory 1G
> qemu-img create -f qcow2 overlay.qcow2 -b nbd:unix:/tmp/sock:exportname=/ -F raw
> 
> (4) Try to boot a libvirt guest using the overlay:
> 
> virt-install --import --name test --disk path=overlay.qcow2,format=raw --memory 1024
> 
> But for some reason libvirt just ignores the overlay:

Do you also use qemu-4.2? The bug manifests itself only when -blockdev
is used. That's why it was dormant at least since 2014. I did not bother
looking further into the history.

At any rate, if it's inconvenient to install qemu-4.2 you can
force-enable blockdev for testing using:

<domain type='kvm' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
   ...
   <qemu:capabilities>
     <qemu:add capability='blockdev'/>
   </qemu:capabilities>
</domain>



> 
> 2020-01-16 13:00:44.980+0000: 2378711: warning : virStorageBackendVolOpen:1527 : ignoring missing file 'nbd:unix:/tmp/sock'
> 
> I verified that the overlay works in programs like qemu-img so it
> doesn't seem to be a problem with the overlay itself.
> 
> Rich.
> 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/util/virstoragefile.c | 2 +-
> >  tests/virstoragetest.c    | 4 ++++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> > index 1397f532fd..7a2af0ad94 100644
> > --- a/src/util/virstoragefile.c
> > +++ b/src/util/virstoragefile.c
> > @@ -2964,7 +2964,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr,
> >          }
> > 
> >          src->hosts->socket = g_strdup(backing[2]);
> > -
> > +        src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_UNIX;
> >     } else {
> >          src->hosts->name = g_strdup(backing[1]);
> > 
> > diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
> > index 2862758752..370e19252b 100644
> > --- a/tests/virstoragetest.c
> > +++ b/tests/virstoragetest.c
> > @@ -1258,6 +1258,10 @@ mymain(void)
> >                         "<source protocol='nbd' name=':test'>\n"
> >                         "  <host name='example.org' port='6000'/>\n"
> >                         "</source>\n");
> > +    TEST_BACKING_PARSE("nbd:unix:/tmp/sock:exportname=/",
> > +                       "<source protocol='nbd' name='/'>\n"
> > +                       "  <host transport='unix' socket='/tmp/sock'/>\n"
> > +                       "</source>\n");
> >      TEST_BACKING_PARSE("nbd://example.org:1234",
> >                         "<source protocol='nbd'>\n"
> >                         "  <host name='example.org' port='1234'/>\n"
> > -- 
> > 2.24.1
> > 
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: storagefile: Properly set transport type when parsing NBD strings

Posted by Eric Blake 4 years, 3 months ago

On 1/16/20 5:41 AM, Peter Krempa wrote:
> When parsing legacy NBD backing file strings such as
> 'nbd:unix:/tmp/sock:exportname=/' we'd fail to set the transport to
> VIR_STORAGE_NET_HOST_TRANS_UNIX. This started to be a problem once we
> actually started to generate config of the backing store on the command
> line with -blockdev as the JSON code would try to format it as TCP and
> fail with:
> 
>   internal error: argument key 'host' must not have null value
> 
> Set the type properly and add a test.
> 
> This bug was found by the libguestfs test suite in:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1791614
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   src/util/virstoragefile.c | 2 +-
>   tests/virstoragetest.c    | 4 ++++
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org