[libvirt] [PATCH 00/14] Ignore backing files with inline authentication (blockdev-add saga)
If a backing file string contains authentication data, many things can break as libvirt is not tracking it since we use the secret driver for this. Stop considering such files as viable backing store entries. Peter Krempa (14): util: storage: Simplify cleanup path handling in virStorageSourceParseBackingJSONInternal util: storagefile: Remove cleanup label from virStorageSourceParseBackingJSONiSCSI util: storagefile: Simplify cleanup handling in virStorageSourceParseBackingURI util: storagefile: Simplify cleanup in virStorageSourceParseBackingJSON tests: viruri: Add test for password in URI userinfo tests: storage: Refactor cleanup in testBackingParse util: storage: Modify return value of virStorageSourceNewFromBacking util: storagefile: Preserve return value in virStorageSourceParseBackingJSONUriStr util: storagefile: Modify arguments of virStorageSourceNewFromBackingAbsolue tests: virstorage: Allow testing return value of virStorageSourceNewFromBackingAbsolute util: storagefile: Add handling of unusable storage sources util: storagefile: Clarify docs for '@report_broken' of virStorageFileGetMetadata util: storagefile: Don't traverse storage sources unusable by VM util: storagefile: Flag backing store strings with authentication src/storage/storage_util.c | 2 +- src/util/virstoragefile.c | 215 ++++++++++++++++++++++--------------- src/util/virstoragefile.h | 7 +- tests/qemublocktest.c | 3 +- tests/virstoragetest.c | 66 +++++++++--- tests/viruritest.c | 1 + 6 files changed, 185 insertions(+), 109 deletions(-) -- 2.21.0
Automatically free the intermediate JSON data to get rid of the cleanup section. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ba56f452e9..520f531088 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3590,22 +3590,21 @@ static int virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, virJSONValuePtr json) { - virJSONValuePtr deflattened = NULL; + VIR_AUTOPTR(virJSONValue) deflattened = NULL; virJSONValuePtr file; const char *drvname; size_t i; - int ret = -1; VIR_AUTOFREE(char *) str = NULL; if (!(deflattened = virJSONValueObjectDeflatten(json))) - goto cleanup; + return -1; if (!(file = virJSONValueObjectGetObject(deflattened, "file"))) { str = virJSONValueToString(json, false); virReportError(VIR_ERR_INVALID_ARG, _("JSON backing volume definition '%s' lacks 'file' object"), NULLSTR(str)); - goto cleanup; + return -1; } if (!(drvname = virJSONValueObjectGetString(file, "driver"))) { @@ -3613,23 +3612,18 @@ virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, virReportError(VIR_ERR_INVALID_ARG, _("JSON backing volume definition '%s' lacks driver name"), NULLSTR(str)); - goto cleanup; + return -1; } for (i = 0; i < ARRAY_CARDINALITY(jsonParsers); i++) { - if (STREQ(drvname, jsonParsers[i].drvname)) { - ret = jsonParsers[i].func(src, file, jsonParsers[i].opaque); - goto cleanup; - } + if (STREQ(drvname, jsonParsers[i].drvname)) + return jsonParsers[i].func(src, file, jsonParsers[i].opaque); } virReportError(VIR_ERR_INTERNAL_ERROR, _("missing parser implementation for JSON backing volume " "driver '%s'"), drvname); - - cleanup: - virJSONValueFree(deflattened); - return ret; + return -1; } -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:22PM +0200, Peter Krempa wrote:
Automatically free the intermediate JSON data to get rid of the cleanup section.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There is no cleanup code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 520f531088..8af45bfbd2 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3263,7 +3263,6 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src, const char *lun = virJSONValueObjectGetStringOrNumber(json, "lun"); const char *uri; char *port; - int ret = -1; /* legacy URI based syntax passed via 'filename' option */ if ((uri = virJSONValueObjectGetString(json, "filename"))) @@ -3277,14 +3276,14 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src, lun = "0"; if (VIR_ALLOC(src->hosts) < 0) - goto cleanup; + return -1; src->nhosts = 1; if (STRNEQ_NULLABLE(transport, "tcp")) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("only TCP transport is supported for iSCSI volumes")); - goto cleanup; + return -1; } src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; @@ -3292,33 +3291,30 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src, if (!portal) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("missing 'portal' address in iSCSI backing definition")); - goto cleanup; + return -1; } if (!target) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("missing 'target' in iSCSI backing definition")); - goto cleanup; + return -1; } if (VIR_STRDUP(src->hosts->name, portal) < 0) - goto cleanup; + return -1; if ((port = strrchr(src->hosts->name, ':')) && !strchr(port, ']')) { if (virStringParsePort(port + 1, &src->hosts->port) < 0) - goto cleanup; + return -1; *port = '\0'; } if (virAsprintf(&src->path, "%s/%s", target, lun) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:23PM +0200, Peter Krempa wrote:
There is no cleanup code.
Do not try and bend the cleanup code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Automatically clean the 'uri' variable and get rid of the 'cleanup' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8af45bfbd2..e93f6285b0 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2664,32 +2664,31 @@ static int virStorageSourceParseBackingURI(virStorageSourcePtr src, const char *uristr) { - virURIPtr uri = NULL; + VIR_AUTOPTR(virURI)uri = NULL; const char *path = NULL; - int ret = -1; VIR_AUTOSTRINGLIST scheme = NULL; if (!(uri = virURIParse(uristr))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse backing file location '%s'"), uristr); - goto cleanup; + return -1; } if (VIR_ALLOC(src->hosts) < 0) - goto cleanup; + return -1; src->nhosts = 1; if (!(scheme = virStringSplit(uri->scheme, "+", 2))) - goto cleanup; + return -1; if (!scheme[0] || (src->protocol = virStorageNetProtocolTypeFromString(scheme[0])) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid backing protocol '%s'"), NULLSTR(scheme[0])); - goto cleanup; + return -1; } if (scheme[1] && @@ -2697,13 +2696,13 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid protocol transport type '%s'"), scheme[1]); - goto cleanup; + return -1; } /* handle socket stored as a query */ if (uri->query) { if (VIR_STRDUP(src->hosts->socket, STRSKIP(uri->query, "socket=")) < 0) - goto cleanup; + return -1; } /* XXX We currently don't support auth, so don't bother parsing it */ @@ -2725,7 +2724,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, path = NULL; if (VIR_STRDUP(src->path, path) < 0) - goto cleanup; + return -1; if (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) { char *tmp; @@ -2733,7 +2732,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, if (!src->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("missing volume name and path for gluster volume")); - goto cleanup; + return -1; } if (!(tmp = strchr(src->path, '/')) || @@ -2741,13 +2740,13 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("missing volume name or file name in " "gluster source path '%s'"), src->path); - goto cleanup; + return -1; } src->volume = src->path; if (VIR_STRDUP(src->path, tmp + 1) < 0) - goto cleanup; + return -1; tmp[0] = '\0'; } @@ -2755,13 +2754,9 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, src->hosts->port = uri->port; if (VIR_STRDUP(src->hosts->name, uri->server) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virURIFree(uri); - return ret; + return 0; } -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:24PM +0200, Peter Krempa wrote:
Automatically clean the 'uri' variable and get rid of the 'cleanup' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8af45bfbd2..e93f6285b0 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2664,32 +2664,31 @@ static int virStorageSourceParseBackingURI(virStorageSourcePtr src, const char *uristr) { - virURIPtr uri = NULL; + VIR_AUTOPTR(virURI)uri = NULL;
Space between the closing parenthesis and the variable name, please.
const char *path = NULL; - int ret = -1; VIR_AUTOSTRINGLIST scheme = NULL;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Automatically free the 'root' temporary variable to get rid fo some complexity. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e93f6285b0..6fff013e3a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3622,16 +3622,12 @@ static int virStorageSourceParseBackingJSON(virStorageSourcePtr src, const char *json) { - virJSONValuePtr root = NULL; - int ret = -1; + VIR_AUTOPTR(virJSONValue) root = NULL; if (!(root = virJSONValueFromString(json))) return -1; - ret = virStorageSourceParseBackingJSONInternal(src, root); - - virJSONValueFree(root); - return ret; + return virStorageSourceParseBackingJSONInternal(src, root); } -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:25PM +0200, Peter Krempa wrote:
Automatically free the 'root' temporary variable to get rid fo some
s/fo/of/
complexity.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
While it's a bad idea to use userinfo to pass credentials via an URI add a test that we at least do the correct thing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/viruritest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/viruritest.c b/tests/viruritest.c index d419711135..3255e2333a 100644 --- a/tests/viruritest.c +++ b/tests/viruritest.c @@ -171,6 +171,7 @@ mymain(void) TEST_PARSE("test://example.com", "test", "example.com", 0, NULL, NULL, NULL, NULL, NULL); TEST_PARSE("test://foo@example.com", "test", "example.com", 0, NULL, NULL, NULL, "foo", NULL); + TEST_PARSE("test://foo:pass@example.com", "test", "example.com", 0, NULL, NULL, NULL, "foo:pass", NULL); TEST_PARSE("test://example.com:123", "test", "example.com", 123, NULL, NULL, NULL, NULL, NULL); TEST_PARSE("test://example.com:123/system?name=value#foo", "test", "example.com", 123, "/system", "name=value", "foo", NULL, params); TEST_PARSE("test://127.0.0.1:123/system", "test", "127.0.0.1", 123, "/system", NULL, NULL, NULL, NULL); -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:26PM +0200, Peter Krempa wrote:
While it's a bad idea to use userinfo to pass credentials via an URI add
s/an URI/a URI/
a test that we at least do the correct thing.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/viruritest.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Automatically clean the temporary buffer and get rid of the cleanup label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstoragetest.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index ef16b3c6e0..1c7ba466f1 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -609,28 +609,27 @@ static int testBackingParse(const void *args) { const struct testBackingParseData *data = args; - virBuffer buf = VIR_BUFFER_INITIALIZER; - int ret = -1; + VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER; VIR_AUTOFREE(char *) xml = NULL; VIR_AUTOUNREF(virStorageSourcePtr) src = NULL; if (!(src = virStorageSourceNewFromBackingAbsolute(data->backing))) { if (!data->expect) - ret = 0; - - goto cleanup; + return 0; + else + return -1; } if (src && !data->expect) { fprintf(stderr, "parsing of backing store string '%s' should " "have failed\n", data->backing); - goto cleanup; + return -1; } if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, 0, NULL) < 0 || !(xml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); - goto cleanup; + return -1; } if (STRNEQ(xml, data->expect)) { @@ -638,15 +637,10 @@ testBackingParse(const void *args) "expected storage source xml:\n%s\n" "actual storage source xml:\n%s\n", data->backing, data->expect, xml); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - virBufferFreeAndReset(&buf); - - return ret; + return 0; } -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:27PM +0200, Peter Krempa wrote:
Automatically clean the temporary buffer and get rid of the cleanup label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstoragetest.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Return the storage source definition via a pointer in the arguments and document the returned values. This will simplify the possibility to ignore certain backing store types which are not representable by libvirt. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_util.c | 2 +- src/util/virstoragefile.c | 59 ++++++++++++++++++++++++-------------- src/util/virstoragefile.h | 4 ++- 3 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 62f857f9ea..24c5918aa3 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3391,7 +3391,7 @@ storageBackendProbeTarget(virStorageSourcePtr target, return -1; if (meta->backingStoreRaw) { - if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) + if (virStorageSourceNewFromBacking(meta, &target->backingStore) < 0) return -1; target->backingStore->format = backingStoreFormat; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6fff013e3a..4e57a0438e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3679,42 +3679,57 @@ virStorageSourceNewFromBackingAbsolute(const char *path) } -virStorageSourcePtr -virStorageSourceNewFromBacking(virStorageSourcePtr parent) +/** + * virStorageSourceNewFromBacking: + * @parent: storage source parent + * @backing: returned backing store definition + * + * Creates a storage source which describes the backing image of @parent and + * fills it into @backing depending on the 'backingStoreRaw' property of @parent + * 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). + */ +int +virStorageSourceNewFromBacking(virStorageSourcePtr parent, + virStorageSourcePtr *backing) { struct stat st; - virStorageSourcePtr ret = NULL; VIR_AUTOUNREF(virStorageSourcePtr) def = NULL; + *backing = NULL; + if (virStorageIsRelative(parent->backingStoreRaw)) def = virStorageSourceNewFromBackingRelative(parent, parent->backingStoreRaw); else def = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw); - if (def) { - /* possibly update local type */ - if (def->type == VIR_STORAGE_TYPE_FILE) { - if (stat(def->path, &st) == 0) { - if (S_ISDIR(st.st_mode)) { - def->type = VIR_STORAGE_TYPE_DIR; - def->format = VIR_STORAGE_FILE_DIR; - } else if (S_ISBLK(st.st_mode)) { - def->type = VIR_STORAGE_TYPE_BLOCK; - } + if (!def) + return -1; + + /* possibly update local type */ + if (def->type == VIR_STORAGE_TYPE_FILE) { + if (stat(def->path, &st) == 0) { + if (S_ISDIR(st.st_mode)) { + def->type = VIR_STORAGE_TYPE_DIR; + def->format = VIR_STORAGE_FILE_DIR; + } else if (S_ISBLK(st.st_mode)) { + def->type = VIR_STORAGE_TYPE_BLOCK; } } + } - /* copy parent's labelling and other top level stuff */ - if (virStorageSourceInitChainElement(def, parent, true) < 0) - return NULL; + /* copy parent's labelling and other top level stuff */ + if (virStorageSourceInitChainElement(def, parent, true) < 0) + return -1; - def->readonly = true; - def->detected = true; - } + def->readonly = true; + def->detected = true; - VIR_STEAL_PTR(ret, def); - return ret; + VIR_STEAL_PTR(*backing, def); + return 0; } @@ -4899,7 +4914,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; if (src->backingStoreRaw) { - if (!(backingStore = virStorageSourceNewFromBacking(src))) + if (virStorageSourceNewFromBacking(src, &backingStore) < 0) goto cleanup; if (backingFormat == VIR_STORAGE_FILE_AUTO) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 2882bacf3e..3a72c62ad7 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -444,7 +444,9 @@ int virStorageSourceUpdateCapacity(virStorageSourcePtr src, char *buf, ssize_t len, bool probe); -virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); +int virStorageSourceNewFromBacking(virStorageSourcePtr parent, + virStorageSourcePtr *backing); + virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, bool backingChain) ATTRIBUTE_NONNULL(1); -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:28PM +0200, Peter Krempa wrote:
Return the storage source definition via a pointer in the arguments and document the returned values. This will simplify the possibility to ignore certain backing store types which are not representable by libvirt.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_util.c | 2 +- src/util/virstoragefile.c | 59 ++++++++++++++++++++++++-------------- src/util/virstoragefile.h | 4 ++- 3 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6fff013e3a..4e57a0438e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3679,42 +3679,57 @@ virStorageSourceNewFromBackingAbsolute(const char *path) }
-virStorageSourcePtr -virStorageSourceNewFromBacking(virStorageSourcePtr parent) +/** + * virStorageSourceNewFromBacking: + * @parent: storage source parent + * @backing: returned backing store definition + * + * Creates a storage source which describes the backing image of @parent and + * fills it into @backing depending on the 'backingStoreRaw' property of @parent + * and other data. Note that for local storage this function interrogates the
Not quite sure what 'interrogates' is supposed to mean in this context. Perhaps you could use another verb?
+ * actual type of the backing store. + * + * Returns 0 and fills @backing, or -1 on error (with appropriate error reported). + */ +int +virStorageSourceNewFromBacking(virStorageSourcePtr parent, + virStorageSourcePtr *backing) {
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virStorageSourceParseBackingURI will report special return values in some cases. Preserve it in virStorageSourceParseBackingJSONUriStr. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 4e57a0438e..192a79c025 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3073,7 +3073,9 @@ virStorageSourceParseBackingJSONUriStr(virStorageSourcePtr src, const char *uri, int protocol) { - if (virStorageSourceParseBackingURI(src, uri) < 0) + int rc; + + if ((rc = virStorageSourceParseBackingURI(src, uri)) < 0) return -1; if (src->protocol != protocol) { @@ -3085,7 +3087,7 @@ virStorageSourceParseBackingJSONUriStr(virStorageSourcePtr src, return -1; } - return 0; + return rc; } -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:29PM +0200, Peter Krempa wrote:
virStorageSourceParseBackingURI will report special return values in some cases. Preserve it in virStorageSourceParseBackingJSONUriStr.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Return the parsed storage source via an pointer in arguments and return an integer from the function. Describe the semantics with a comment for the function and adjust callers to the new semantics. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 43 ++++++++++++++++++++++++--------------- src/util/virstoragefile.h | 3 ++- tests/qemublocktest.c | 3 ++- tests/virstoragetest.c | 2 +- 4 files changed, 32 insertions(+), 19 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 192a79c025..8447c014f0 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3633,22 +3633,32 @@ virStorageSourceParseBackingJSON(virStorageSourcePtr src, } -virStorageSourcePtr -virStorageSourceNewFromBackingAbsolute(const char *path) +/** + * virStorageSourceNewFromBackingAbsolute + * @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. + */ +int +virStorageSourceNewFromBackingAbsolute(const char *path, + virStorageSourcePtr *src) { const char *json; - virStorageSourcePtr ret = NULL; int rc; VIR_AUTOUNREF(virStorageSourcePtr) def = NULL; + *src = NULL; + if (!(def = virStorageSourceNew())) - return NULL; + return -1; if (virStorageIsFile(path)) { def->type = VIR_STORAGE_TYPE_FILE; if (VIR_STRDUP(def->path, path) < 0) - return NULL; + return -1; } else { def->type = VIR_STORAGE_TYPE_NETWORK; @@ -3663,7 +3673,7 @@ virStorageSourceNewFromBackingAbsolute(const char *path) rc = virStorageSourceParseBackingColon(def, path); if (rc < 0) - return NULL; + return -1; virStorageSourceNetworkAssignDefaultPorts(def); @@ -3676,8 +3686,8 @@ virStorageSourceNewFromBackingAbsolute(const char *path) } } - VIR_STEAL_PTR(ret, def); - return ret; + VIR_STEAL_PTR(*src, def); + return 0; } @@ -3702,14 +3712,15 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent, *backing = NULL; - if (virStorageIsRelative(parent->backingStoreRaw)) - def = virStorageSourceNewFromBackingRelative(parent, - parent->backingStoreRaw); - else - def = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw); - - if (!def) - return -1; + if (virStorageIsRelative(parent->backingStoreRaw)) { + if (!(def = virStorageSourceNewFromBackingRelative(parent, + parent->backingStoreRaw))) + return -1; + } else { + if (virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw, + &def) < 0) + return -1; + } /* possibly update local type */ if (def->type == VIR_STORAGE_TYPE_FILE) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3a72c62ad7..2cceaf6954 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -473,7 +473,8 @@ int virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, int virStorageFileCheckCompat(const char *compat); -virStorageSourcePtr virStorageSourceNewFromBackingAbsolute(const char *path); +int virStorageSourceNewFromBackingAbsolute(const char *path, + virStorageSourcePtr *src); bool virStorageSourceIsRelative(virStorageSourcePtr src); diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 9321531f6c..e5d77c423c 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -85,7 +85,8 @@ testBackingXMLjsonXML(const void *args) if (virAsprintf(&protocolwrapper, "json:%s", propsstr) < 0) return -1; - if (!(jsonsrc = virStorageSourceNewFromBackingAbsolute(protocolwrapper))) { + if (virStorageSourceNewFromBackingAbsolute(protocolwrapper, + &jsonsrc) < 0) { fprintf(stderr, "failed to parse disk json\n"); return -1; } diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 1c7ba466f1..0495308318 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -613,7 +613,7 @@ testBackingParse(const void *args) VIR_AUTOFREE(char *) xml = NULL; VIR_AUTOUNREF(virStorageSourcePtr) src = NULL; - if (!(src = virStorageSourceNewFromBackingAbsolute(data->backing))) { + if (virStorageSourceNewFromBackingAbsolute(data->backing, &src) < 0) { if (!data->expect) return 0; else -- 2.21.0
On Fri, Aug 16, 2019 at 12:39:30PM +0200, Peter Krempa wrote:
Return the parsed storage source via an pointer in arguments and return an integer from the function. Describe the semantics with a comment for the function and adjust callers to the new semantics.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 43 ++++++++++++++++++++++++--------------- src/util/virstoragefile.h | 3 ++- tests/qemublocktest.c | 3 ++- tests/virstoragetest.c | 2 +- 4 files changed, 32 insertions(+), 19 deletions(-)
On Wed, Aug 21, 2019 at 05:11:38PM +0200, Ján Tomko wrote:
On Fri, Aug 16, 2019 at 12:39:30PM +0200, Peter Krempa wrote:
Return the parsed storage source via an pointer in arguments and return an integer from the function. Describe the semantics with a comment for the function and adjust callers to the new semantics.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 43 ++++++++++++++++++++++++--------------- src/util/virstoragefile.h | 3 ++- tests/qemublocktest.c | 3 ++- tests/virstoragetest.c | 2 +- 4 files changed, 32 insertions(+), 19 deletions(-)
This should've said: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Modiy testBackingParse to allow testing other return values of the backing store string parser. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
On Fri, Aug 16, 2019 at 12:39:31PM +0200, Peter Krempa wrote:
Modiy testBackingParse to allow testing other return values of the
Modify
backing store string parser.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstoragetest.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On Fri, Aug 16, 2019 at 12:39:32PM +0200, Peter Krempa wrote:
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@redhat.com> --- src/util/virstoragefile.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On Fri, Aug 16, 2019 at 12:39:33PM +0200, Peter Krempa wrote:
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
unfortunately
offload it to qemu.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On Fri, Aug 16, 2019 at 12:39:34PM +0200, Peter Krempa wrote:
virStorageFileGetMetadataRecurse would include in the backing chain files which would not really be usable by libvirt directly e.g. when
include files in the backing chain
such file would be promoted to the top layer by a active block commit as
an active
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@redhat.com> --- src/util/virstoragefile.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On Fri, Aug 16, 2019 at 12:39:35PM +0200, Peter Krempa wrote:
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@redhat.com> --- src/util/virstoragefile.c | 11 +++++++++-- tests/virstoragetest.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa