On Fri, Aug 16, 2019 at 12:39:24PM +0200, Peter Krempa wrote: Space between the closing parenthesis and the variable name, please. Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

On Fri, Aug 16, 2019 at 12:39:28PM +0200, Peter Krempa wrote: Not quite sure what 'interrogates' is supposed to mean in this context. Perhaps you could use another verb? Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

Modiy testBackingParse to allow testing other return values of the backing store string parser. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/virstoragetest.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 0495308318..be5cb98262 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -603,6 +603,7 @@ testPathRelative(const void *args) struct testBackingParseData { const char *backing; const char *expect; + int rv; }; static int @@ -612,14 +613,21 @@ testBackingParse(const void *args) VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER; VIR_AUTOFREE(char *) xml = NULL; VIR_AUTOUNREF(virStorageSourcePtr) src = NULL; + int rc; + int erc = data->rv; - if (virStorageSourceNewFromBackingAbsolute(data->backing, &src) < 0) { - if (!data->expect) - return 0; - else - return -1; + /* expect failure return code with NULL expected data */ + if (!data->expect) + erc = -1; + + if ((rc = virStorageSourceNewFromBackingAbsolute(data->backing, &src)) != erc) { + fprintf(stderr, "expected return value '%d' actual '%d'\n", erc, rc); + return -1; } + if (!src) + return 0; + if (src && !data->expect) { fprintf(stderr, "parsing of backing store string '%s' should " "have failed\n", data->backing); @@ -1225,15 +1233,19 @@ mymain(void) virTestCounterReset("Backing store parse "); -#define TEST_BACKING_PARSE(bck, xml) \ +#define TEST_BACKING_PARSE_FULL(bck, xml, rc) \ do { \ data5.backing = bck; \ data5.expect = xml; \ + data5.rv = rc; \ if (virTestRun(virTestCounterNext(), \ testBackingParse, &data5) < 0) \ ret = -1; \ } while (0) +#define TEST_BACKING_PARSE(bck, xml) \ + TEST_BACKING_PARSE_FULL(bck, xml, 0) + TEST_BACKING_PARSE("path", "<source file='path'/>\n"); TEST_BACKING_PARSE("://", NULL); TEST_BACKING_PARSE("http://example.com", -- 2.21.0

Introduce new semantics to virStorageSourceNewFromBacking and some of the helpers used by it which propagate the return value from the callers. The new return value introduced by this patch allows to notify the calller that the parsed virStorageSource correctly describes the source but contains data such as inline authentication which libvirt does not want to support directly. This means that such file would e.g. unusable as a storage source (e.g. when actively commiting the overlay to it) or would not work with blockdev. The caller will then be able to decide whether to consider this backing file as viable or just fall back to qemu dealing with it. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virstoragefile.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8447c014f0..fa0233376b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3554,6 +3554,13 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src, struct virStorageSourceJSONDriverParser { const char *drvname; + /** + * The callback gets a pre-allocated storage source @src and the JSON + * object to parse. The callback shall return -1 on error and report error + * 0 on success and 1 in cases when the configuration itself is valid, but + * can't be converted to libvirt's configuration (e.g. inline authentication + * credentials are present). + */ int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque); int opaque; }; @@ -3638,15 +3645,17 @@ virStorageSourceParseBackingJSON(virStorageSourcePtr src, * @path: string representing absolute location of a storage source * @src: filled with virStorageSource object representing @path * - * Returns 0 on success and fills @src or -1 on error and reports appropriate - * error. + * Returns 0 on success, 1 if we could parse all location data but @path + * specified other data unrepresentable by libvirt (e.g. inline authentication). + * In both cases @src is filled. On error -1 is returned @src is NULL and an + * error is reported. */ int virStorageSourceNewFromBackingAbsolute(const char *path, virStorageSourcePtr *src) { const char *json; - int rc; + int rc = 0; VIR_AUTOUNREF(virStorageSourcePtr) def = NULL; *src = NULL; @@ -3687,7 +3696,7 @@ virStorageSourceNewFromBackingAbsolute(const char *path, } VIR_STEAL_PTR(*src, def); - return 0; + return rc; } @@ -3701,7 +3710,11 @@ virStorageSourceNewFromBackingAbsolute(const char *path, * and other data. Note that for local storage this function interrogates the * actual type of the backing store. * - * Returns 0 and fills @backing, or -1 on error (with appropriate error reported). + * Returns 0 on success, 1 if we could parse all location data but the backinig + * store specification contained other data unrepresentable by libvirt (e.g. + * inline authentication). + * In both cases @src is filled. On error -1 is returned @src is NULL and an + * error is reported. */ int virStorageSourceNewFromBacking(virStorageSourcePtr parent, @@ -3709,6 +3722,7 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent, { struct stat st; VIR_AUTOUNREF(virStorageSourcePtr) def = NULL; + int rc = 0; *backing = NULL; @@ -3717,8 +3731,8 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent, parent->backingStoreRaw))) return -1; } else { - if (virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw, - &def) < 0) + if ((rc = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw, + &def)) < 0) return -1; } @@ -3742,7 +3756,7 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent, def->detected = true; VIR_STEAL_PTR(*backing, def); - return 0; + return rc; } -- 2.21.0

virStorageFileGetMetadata does not report error if we can't interrogate the file somehow. Clarify this in the description of the @report_broken flag as it implies we should report an error in that case. The problem is that we don't know whether there's a problem and unforntnately just offload it to qemu. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virstoragefile.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index fa0233376b..32afee1ca7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4996,7 +4996,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, * other non-raw format at will. * * If @report_broken is true, the whole function fails with a possibly sane - * error instead of just returning a broken chain. + * error instead of just returning a broken chain. Note that the inability for + * libvirt to traverse a given source is not considered an error. * * Caller MUST free result after use via virObjectUnref. */ -- 2.21.0

virStorageFileGetMetadataRecurse would include in the backing chain files which would not really be usable by libvirt directly e.g. when such file would be promoted to the top layer by a active block commit as for example inline authentication data can't be represented in the VM xml file. The idea is to use secrets for this. With the changes to the backing store string parsers we can report and propagate if such a thing is present in the configuration and thus start skipping those files in the backing chain traversal code. This approach still allows to report the appropriate backing store string in the storage driver which doesn't directly use the backing file. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virstoragefile.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 32afee1ca7..efc1d84048 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4941,9 +4941,15 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; if (src->backingStoreRaw) { - if (virStorageSourceNewFromBacking(src, &backingStore) < 0) + if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0) goto cleanup; + if (rv == 1) { + /* the backing file would not be usable for VM usage */ + ret = 0; + goto cleanup; + } + if (backingFormat == VIR_STORAGE_FILE_AUTO) backingStore->format = VIR_STORAGE_FILE_RAW; else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) -- 2.21.0

Using inline authentication for storage volumes will not work properly as libvirt requires use of the secret driver for the auth data and thus would not be able to represent the passwords stored in the backing store string. Make sure that the backing store parsers return 1 which is a sign for the caller to not use the file in certain cases. The test data include iscsi via a json pseudo-protocol string and URIs with the userinfo part being present. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virstoragefile.c | 11 +++++++++-- tests/virstoragetest.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index efc1d84048..437dcc015d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2705,8 +2705,6 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, return -1; } - /* XXX We currently don't support auth, so don't bother parsing it */ - /* uri->path is NULL if the URI does not contain slash after host: * transport://host:port */ if (uri->path) @@ -2756,6 +2754,10 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, if (VIR_STRDUP(src->hosts->name, uri->server) < 0) return -1; + /* Libvirt doesn't handle inline authentication. Make the caller aware. */ + if (uri->user) + return 1; + return 0; } @@ -3311,6 +3313,11 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src, if (virAsprintf(&src->path, "%s/%s", target, lun) < 0) return -1; + /* Libvirt doesn't handle inline authentication. Make the caller aware. */ + if (virJSONValueObjectGetString(json, "user") || + virJSONValueObjectGetString(json, "password")) + return 1; + return 0; } diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index be5cb98262..1d06abe8b6 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1260,6 +1260,10 @@ mymain(void) "<source protocol='http' name='file'>\n" " <host name='example.com' port='80'/>\n" "</source>\n"); + TEST_BACKING_PARSE_FULL("http://user:pass@example.com/file", + "<source protocol='http' name='file'>\n" + " <host name='example.com' port='80'/>\n" + "</source>\n", 1); TEST_BACKING_PARSE("rbd:testshare:id=asdf:mon_host=example.com", "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" @@ -1280,6 +1284,10 @@ mymain(void) "<source protocol='nbd' name='exportname'>\n" " <host name='example.org' port='1234'/>\n" "</source>\n"); + TEST_BACKING_PARSE_FULL("iscsi://testuser:testpass@example.org:1234/exportname", + "<source protocol='iscsi' name='exportname'>\n" + " <host name='example.org' port='1234'/>\n" + "</source>\n", 1); #ifdef WITH_YAJL TEST_BACKING_PARSE("json:", NULL); @@ -1484,6 +1492,26 @@ mymain(void) "<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-noauth.target/0'>\n" " <host name='test.org' port='3260'/>\n" "</source>\n"); + TEST_BACKING_PARSE_FULL("json:{\"file\":{\"driver\":\"iscsi\"," + "\"transport\":\"tcp\"," + "\"portal\":\"test.org\"," + "\"user\":\"testuser\"," + "\"target\":\"iqn.2016-12.com.virttest:emulated-iscsi-auth.target\"" + "}" + "}", + "<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-auth.target/0'>\n" + " <host name='test.org' port='3260'/>\n" + "</source>\n", 1); + TEST_BACKING_PARSE_FULL("json:{\"file\":{\"driver\":\"iscsi\"," + "\"transport\":\"tcp\"," + "\"portal\":\"test.org\"," + "\"password\":\"testpass\"," + "\"target\":\"iqn.2016-12.com.virttest:emulated-iscsi-auth.target\"" + "}" + "}", + "<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-auth.target/0'>\n" + " <host name='test.org' port='3260'/>\n" + "</source>\n", 1); TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"iscsi\"," "\"transport\":\"tcp\"," "\"portal\":\"test.org:1234\"," -- 2.21.0