[libvirt] [PATCH 0/2] util: storage: Fix parsing of URIs as disk sources
see 2/2 for explanation Peter Krempa (2): util: storage: Rename '@path' argument of virStorageSourceParseBackingURI util: storage: Properly parse URIs with missing trailing slash src/util/virstoragefile.c | 28 +++++++++++++++++++++------- tests/qemublocktest.c | 3 +++ tests/virstoragetest.c | 20 ++++++++++++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) -- 2.17.1
The name is misleading. Change it to 'uristr' so that 'path' can be reused in the proper context later. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b2169b618d..eba82918c1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2576,16 +2576,16 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, static int virStorageSourceParseBackingURI(virStorageSourcePtr src, - const char *path) + const char *uristr) { virURIPtr uri = NULL; char **scheme = NULL; int ret = -1; - if (!(uri = virURIParse(path))) { + if (!(uri = virURIParse(uristr))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse backing file location '%s'"), - path); + uristr); goto cleanup; } -- 2.17.1
The URI parser used by libvirt does not populate uri->path if the trailing slash is missing. The code virStorageSourceParseBackingURI would then not populate src->path. As only NBD network disks are allowed to have the 'name' field in the XML defining the disk source omitted we'd generate an invalid XML which we'd not parse again. Fix it by populating src->path with an empty string if the uri is lacking slash. As pointed out above NBD is special in this case since we actually allow it being NULL. The URI path is used as export name. Since an empty export does not make sense the new approach clears the src->path if the trailing slash is present but nothing else. Add test cases now to cover all the various cases for NBD and non-NBD uris as there was to time only 1 test abusing the quirk witout slash for NBD and all other URIs contained the slash or in case of NBD also the export name. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 22 ++++++++++++++++++---- tests/qemublocktest.c | 3 +++ tests/virstoragetest.c | 20 ++++++++++++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index eba82918c1..94c32d818e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2579,6 +2579,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, const char *uristr) { virURIPtr uri = NULL; + const char *path = NULL; char **scheme = NULL; int ret = -1; @@ -2621,10 +2622,23 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, /* XXX We currently don't support auth, so don't bother parsing it */ - /* possibly skip the leading slash */ - if (uri->path && - VIR_STRDUP(src->path, - *uri->path == '/' ? uri->path + 1 : uri->path) < 0) + /* uri->path is NULL if the URI does not contain slash after host: + * transport://host:port */ + if (uri->path) + path = uri->path; + else + path = ""; + + /* possibly skip the leading slash */ + if (path[0] == '/') + path++; + + /* NBD allows empty export name (path) */ + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_NBD && + path[0] == '\0') + path = NULL; + + if (VIR_STRDUP(src->path, path) < 0) goto cleanup; if (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) { diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 6d85718d92..5848f6b5b5 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -373,6 +373,9 @@ mymain(void) /* type VIR_STORAGE_TYPE_BLOCK is not tested since it parses back to 'file' */ /* type VIR_STORAGE_TYPE_DIR it is a 'format' driver in qemu */ + TEST_JSON_FORMAT_NET("<source protocol='http' name=''>\n" + " <host name='example.com' port='80'/>\n" + "</source>\n"); TEST_JSON_FORMAT_NET("<source protocol='http' name='file'>\n" " <host name='example.com' port='80'/>\n" "</source>\n"); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 83680a63a0..be4f558035 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1303,6 +1303,14 @@ mymain(void) TEST_BACKING_PARSE("path", "<source file='path'/>\n"); TEST_BACKING_PARSE("://", NULL); + TEST_BACKING_PARSE("http://example.com", + "<source protocol='http' name=''>\n" + " <host name='example.com' port='80'/>\n" + "</source>\n"); + TEST_BACKING_PARSE("http://example.com/", + "<source protocol='http' name=''>\n" + " <host name='example.com' port='80'/>\n" + "</source>\n"); TEST_BACKING_PARSE("http://example.com/file", "<source protocol='http' name='file'>\n" " <host name='example.com' port='80'/>\n" @@ -1315,6 +1323,18 @@ mymain(void) "<source protocol='nbd' name='blah'>\n" " <host name='example.org' port='6000'/>\n" "</source>\n"); + TEST_BACKING_PARSE("nbd://example.org:1234", + "<source protocol='nbd'>\n" + " <host name='example.org' port='1234'/>\n" + "</source>\n"); + TEST_BACKING_PARSE("nbd://example.org:1234/", + "<source protocol='nbd'>\n" + " <host name='example.org' port='1234'/>\n" + "</source>\n"); + TEST_BACKING_PARSE("nbd://example.org:1234/exportname", + "<source protocol='nbd' name='exportname'>\n" + " <host name='example.org' port='1234'/>\n" + "</source>\n"); #ifdef WITH_YAJL TEST_BACKING_PARSE("json:", NULL); -- 2.17.1
On 10/11/18 6:01 AM, Peter Krempa wrote:
The URI parser used by libvirt does not populate uri->path if the trailing slash is missing. The code virStorageSourceParseBackingURI would then not populate src->path.
As only NBD network disks are allowed to have the 'name' field in the XML defining the disk source omitted we'd generate an invalid XML which we'd not parse again.
Fix it by populating src->path with an empty string if the uri is lacking slash.
As pointed out above NBD is special in this case since we actually allow it being NULL. The URI path is used as export name. Since an empty export does not make sense the new approach clears the src->path if the trailing slash is present but nothing else.
Add test cases now to cover all the various cases for NBD and non-NBD uris as there was to time only 1 test abusing the quirk witout slash for
s/witout/without/
NBD and all other URIs contained the slash or in case of NBD also the export name.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 22 ++++++++++++++++++---- tests/qemublocktest.c | 3 +++ tests/virstoragetest.c | 20 ++++++++++++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-)
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 10/11/2018 12:47 PM, Peter Krempa wrote:
see 2/2 for explanation
Peter Krempa (2): util: storage: Rename '@path' argument of virStorageSourceParseBackingURI util: storage: Properly parse URIs with missing trailing slash
src/util/virstoragefile.c | 28 +++++++++++++++++++++------- tests/qemublocktest.c | 3 +++ tests/virstoragetest.c | 20 ++++++++++++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-)
ACK Michal
participants (3)
-
Eric Blake -
Michal Privoznik -
Peter Krempa