src/util/virstoragefile.c | 90 +++++++++++++++++++++++---------------- tests/virstoragetest.c | 16 +++++++ 2 files changed, 70 insertions(+), 36 deletions(-)
Our implementation wasn't quite able to parse everything that qemu does.
This patch rewrites the parser to a code that semantically resembles the
combination of 'nbd_parse_filename' and 'inet_parse' methods in qemu to
be able to parse the strings in an equivalent manner.
The only thing that libvirt doesn't do is to check the lenghts of
various components in the nbd string in places where qemu uses constant
size buffers.
The test cases validate that some of the corner cases involving colons
are parsed properly.
https://bugzilla.redhat.com/show_bug.cgi?id=1826652
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
src/util/virstoragefile.c | 90 +++++++++++++++++++++++----------------
tests/virstoragetest.c | 16 +++++++
2 files changed, 70 insertions(+), 36 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index ffc8bdb344..a2ecdc3928 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3072,59 +3072,77 @@ static int
virStorageSourceParseNBDColonString(const char *nbdstr,
virStorageSourcePtr src)
{
- VIR_AUTOSTRINGLIST backing = NULL;
- const char *exportname;
-
- if (!(backing = virStringSplit(nbdstr, ":", 0)))
- return -1;
-
- /* we know that backing[0] now equals to "nbd" */
-
- if (VIR_ALLOC_N(src->hosts, 1) < 0)
- return -1;
+ g_autofree char *nbd = g_strdup(nbdstr);
+ char *export_name;
+ char *host_spec;
+ char *unixpath;
+ char *port;
+ src->hosts = g_new0(virStorageNetHostDef, 1);
src->nhosts = 1;
- src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
+
+ /* We extract the parameters in a similar way qemu does it */
/* format: [] denotes optional sections, uppercase are variable strings
* nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME]
* nbd:HOSTNAME:PORT[:exportname=EXPORTNAME]
*/
- if (!backing[1]) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("missing remote information in '%s' for protocol nbd"),
- nbdstr);
- return -1;
- } else if (STREQ(backing[1], "unix")) {
- if (!backing[2]) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("missing unix socket path in nbd backing string %s"),
- nbdstr);
- return -1;
- }
- src->hosts->socket = g_strdup(backing[2]);
+ /* first look for ':exportname=' and cut it off */
+ if ((export_name = strstr(nbd, ":exportname="))) {
+ src->path = g_strdup(export_name + strlen(":exportname="));
+ export_name[0] = '\0';
+ }
+
+ /* Verify the prefix and contents. Note that we require a
+ * "host_spec" part to be present. */
+ if (!(host_spec = STRSKIP(nbd, "nbd:")) || host_spec[0] == '\0')
+ goto malformed;
+
+ if ((unixpath = STRSKIP(host_spec, "unix:"))) {
src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_UNIX;
- } else {
- src->hosts->name = g_strdup(backing[1]);
- if (!backing[2]) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("missing port in nbd string '%s'"),
- nbdstr);
- return -1;
+ if (unixpath[0] == '\0')
+ goto malformed;
+
+ src->hosts->socket = g_strdup(unixpath);
+ } else {
+ src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
+
+ if (host_spec[0] == ':') {
+ /* no host given */
+ goto malformed;
+ } else if (host_spec[0] == '[') {
+ host_spec++;
+ /* IPv6 addr */
+ if (!(port = strstr(host_spec, "]:")))
+ goto malformed;
+
+ port[0] = '\0';
+ port += 2;
+
+ if (host_spec[0] == '\0')
+ goto malformed;
+ } else {
+ if (!(port = strchr(host_spec, ':')))
+ goto malformed;
+
+ port[0] = '\0';
+ port++;
}
- if (virStringParsePort(backing[2], &src->hosts->port) < 0)
+ if (virStringParsePort(port, &src->hosts->port) < 0)
return -1;
- }
- if ((exportname = strstr(nbdstr, "exportname="))) {
- exportname += strlen("exportname=");
- src->path = g_strdup(exportname);
+ src->hosts->name = g_strdup(host_spec);
}
return 0;
+
+ malformed:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed nbd string '%s'"), nbdstr);
+ return -1;
}
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 6d2b21c25f..ac1480de4e 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1261,10 +1261,26 @@ mymain(void)
"<source protocol='nbd' name=':test'>\n"
" <host name='example.org' port='6000'/>\n"
"</source>\n");
+ TEST_BACKING_PARSE("nbd:[::1]:6000:exportname=:test",
+ "<source protocol='nbd' name=':test'>\n"
+ " <host name='::1' port='6000'/>\n"
+ "</source>\n");
+ TEST_BACKING_PARSE("nbd:127.0.0.1:6000:exportname=:test",
+ "<source protocol='nbd' name=':test'>\n"
+ " <host name='127.0.0.1' 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:unix:/tmp/sock:",
+ "<source protocol='nbd'>\n"
+ " <host transport='unix' socket='/tmp/sock:'/>\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.26.0
On 4/24/20 7:04 AM, Peter Krempa wrote:
> Our implementation wasn't quite able to parse everything that qemu does.
> This patch rewrites the parser to a code that semantically resembles the
> combination of 'nbd_parse_filename' and 'inet_parse' methods in qemu to
> be able to parse the strings in an equivalent manner.
>
> The only thing that libvirt doesn't do is to check the lenghts of
lengths
> various components in the nbd string in places where qemu uses constant
> size buffers.
>
> The test cases validate that some of the corner cases involving colons
> are parsed properly.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1826652
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> src/util/virstoragefile.c | 90 +++++++++++++++++++++++----------------
> tests/virstoragetest.c | 16 +++++++
> 2 files changed, 70 insertions(+), 36 deletions(-)
>
> +++ b/tests/virstoragetest.c
> @@ -1261,10 +1261,26 @@ mymain(void)
> "<source protocol='nbd' name=':test'>\n"
> " <host name='example.org' port='6000'/>\n"
> "</source>\n");
> + TEST_BACKING_PARSE("nbd:[::1]:6000:exportname=:test",
> + "<source protocol='nbd' name=':test'>\n"
> + " <host name='::1' port='6000'/>\n"
> + "</source>\n");
> + TEST_BACKING_PARSE("nbd:127.0.0.1:6000:exportname=:test",
> + "<source protocol='nbd' name=':test'>\n"
> + " <host name='127.0.0.1' port='6000'/>\n"
> + "</source>\n");
Hideous abuse of both socket name and export name, but I concur that
fixing libvirt to parse the ugly strings in the same way as qemu is
worthwhile. I'd highly recommend anyone thinking of actually doing this
in practice (rather than just in a QE setup) to use the NBD URI syntax:
nbd://[::1]:6000/:test
nbd://127.0.0.1:6000/:test
> 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:unix:/tmp/sock:",
> + "<source protocol='nbd'>\n"
> + " <host transport='unix' socket='/tmp/sock:'/>\n"
> + "</source>\n");
nbd+unix:///?socket=/tmp/sock:
> + TEST_BACKING_PARSE("nbd:unix:/tmp/sock::exportname=:",
> + "<source protocol='nbd' name=':'>\n"
> + " <host transport='unix' socket='/tmp/sock:'/>\n"
> + "</source>\n");
nbd+unix:///:?socket=/tmp/sock:
Whether you also add the proper URI counterparts to the test is not
directly relevant to the fix at hand.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On Fri, Apr 24, 2020 at 10:02:49 -0500, Eric Blake wrote:
> On 4/24/20 7:04 AM, Peter Krempa wrote:
> > Our implementation wasn't quite able to parse everything that qemu does.
> > This patch rewrites the parser to a code that semantically resembles the
> > combination of 'nbd_parse_filename' and 'inet_parse' methods in qemu to
> > be able to parse the strings in an equivalent manner.
> >
> > The only thing that libvirt doesn't do is to check the lenghts of
>
> lengths
>
> > various components in the nbd string in places where qemu uses constant
> > size buffers.
> >
> > The test cases validate that some of the corner cases involving colons
> > are parsed properly.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1826652
> >
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > src/util/virstoragefile.c | 90 +++++++++++++++++++++++----------------
> > tests/virstoragetest.c | 16 +++++++
> > 2 files changed, 70 insertions(+), 36 deletions(-)
> >
>
> > + TEST_BACKING_PARSE("nbd:unix:/tmp/sock::exportname=:",
> > + "<source protocol='nbd' name=':'>\n"
> > + " <host transport='unix' socket='/tmp/sock:'/>\n"
> > + "</source>\n");
>
> nbd+unix:///:?socket=/tmp/sock:
>
> Whether you also add the proper URI counterparts to the test is not directly
> relevant to the fix at hand.
I can add them but separately. The URIs are parsed via the libxml URI
module so they are not related in any way to the colon-delimited string
code-wise.
© 2016 - 2024 Red Hat, Inc.