[libvirt] [PATCHv5 00/19] block pull/commit on gluster volumes with relative backing
This version is based on Eric's "[PATCH v3 0/5] Next round of active commit support". Otherwise I've fixed a few nits and just rebased it on top of the said series. Peter Krempa (19): util: string: Add helper to free non-NULL terminated string arrays util: storagefile: Introduce universal function to canonicalize paths storage: gluster: Add backend to return unique storage file path util: storage: Add helper to resolve relative path difference tests: virstoragetest: Remove "expBackingStore" field storage: Store relative path only for relatively backed storage tests: virstoragetest: Remove now unused pathAbs util: storage: Remove now redundant backingRelative from virStorageSource tests: virstoragetest: Don't test relative start of backing chains tests: virstoragetest: Remove unneeded relative test plumbing storage: Don't canonicalize paths unnecessarily storage: Don't store parent directory of an image explicitly qemu: caps: Add capability for change-backing-file command qemu: monitor: Add argument for specifying backing name for block commit qemu: monitor: Add support for backing name specification for block-stream lib: Introduce flag VIR_DOMAIN_BLOCK_COMMIT_RELATIVE lib: Introduce flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE qemu: Add support for networked disks for block commit qemu: Add support for networked disks for block pull/block rebase include/libvirt/libvirt.h.in | 6 + src/libvirt.c | 10 + src/libvirt_private.syms | 3 + src/qemu/qemu_capabilities.c | 4 +- src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 87 ++++++- src/qemu/qemu_migration.c | 6 +- src/qemu/qemu_monitor.c | 20 +- src/qemu/qemu_monitor.h | 4 +- src/qemu/qemu_monitor_json.c | 17 ++ src/qemu/qemu_monitor_json.h | 2 + src/storage/storage_backend_gluster.c | 80 ++++++ src/storage/storage_driver.c | 15 +- src/util/virstoragefile.c | 392 ++++++++++++++++++++++++------ src/util/virstoragefile.h | 20 +- src/util/virstring.c | 20 ++ src/util/virstring.h | 1 + tests/qemumonitorjsontest.c | 6 +- tests/virstoragetest.c | 444 +++++++++++++++++++++------------- tools/virsh-domain.c | 29 ++- tools/virsh.pod | 9 +- 21 files changed, 882 insertions(+), 294 deletions(-) -- 1.9.3
Sometimes the length of the string list is known but the array isn't NULL terminated. Add helper to free the array in such cases. --- src/libvirt_private.syms | 1 + src/util/virstring.c | 20 ++++++++++++++++++++ src/util/virstring.h | 1 + 3 files changed, 22 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 46c0f02..6bd87e6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1918,6 +1918,7 @@ virStrcpy; virStrdup; virStringArrayHasString; virStringFreeList; +virStringFreeListCount; virStringJoin; virStringListLength; virStringReplace; diff --git a/src/util/virstring.c b/src/util/virstring.c index 6dcc7a8..35b99a5 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -187,6 +187,26 @@ void virStringFreeList(char **strings) } +/** + * virStringFreeListCount: + * @strings: array of strings to free + * @count: number of elements in the array + * + * Frees a string array of @count length. + */ +void +virStringFreeListCount(char **strings, + size_t count) +{ + size_t i; + + for (i = 0; i < count; i++) + VIR_FREE(strings[i]); + + VIR_FREE(strings); +} + + bool virStringArrayHasString(char **strings, const char *needle) { diff --git a/src/util/virstring.h b/src/util/virstring.h index 6ddcff5..df25441 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -42,6 +42,7 @@ char *virStringJoin(const char **strings, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virStringFreeList(char **strings); +void virStringFreeListCount(char **strings, size_t count); bool virStringArrayHasString(char **strings, const char *needle); -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Sometimes the length of the string list is known but the array isn't NULL terminated. Add helper to free the array in such cases. --- src/libvirt_private.syms | 1 + src/util/virstring.c | 20 ++++++++++++++++++++ src/util/virstring.h | 1 + 3 files changed, 22 insertions(+)
I'm still not fully convinced we needed this (if you ALWAYS over-allocate the array large enough for a NULL terminator, including in VIR_APPEND_ELEMENT and friends), but it's easier to let it in than to force you to work around it. I would like to see coverage added in tests/virstringtest.c, though. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/20/14 00:41, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Sometimes the length of the string list is known but the array isn't NULL terminated. Add helper to free the array in such cases. --- src/libvirt_private.syms | 1 + src/util/virstring.c | 20 ++++++++++++++++++++ src/util/virstring.h | 1 + 3 files changed, 22 insertions(+)
I'm still not fully convinced we needed this (if you ALWAYS over-allocate the array large enough for a NULL terminator, including in
Well I'm using it to free a string list that had the individual string elements stolen from it, where the regular freeing function would finish possibly on the first element as it might be already NULL and stolen. This function should make sure that if some strings from the array are not stolen for some reason that they are still freed.
VIR_APPEND_ELEMENT and friends), but it's easier to let it in than to force you to work around it. I would like to see coverage added in tests/virstringtest.c, though.
Hmm testing a freeing function that frees the entire array? Not sure how I'd approach it. Also is it really worth? Peter
On 06/20/2014 01:59 AM, Peter Krempa wrote:
On 06/20/14 00:41, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Sometimes the length of the string list is known but the array isn't NULL terminated. Add helper to free the array in such cases. --- src/libvirt_private.syms | 1 + src/util/virstring.c | 20 ++++++++++++++++++++ src/util/virstring.h | 1 + 3 files changed, 22 insertions(+)
I'm still not fully convinced we needed this (if you ALWAYS over-allocate the array large enough for a NULL terminator, including in
Well I'm using it to free a string list that had the individual string elements stolen from it, where the regular freeing function would finish possibly on the first element as it might be already NULL and stolen.
Ah, _that_ explains it. Put this reason in your commit message, and then I'm fine giving ACK.
Hmm testing a freeing function that frees the entire array? Not sure how I'd approach it. Also is it really worth?
Create an array of { "mallocd", NULL, "mallocd", NULL }, then free it. If the test doesn't crash, and if valgrind doesn't report leaks or double-free, then it was worth the test. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Introduce a common function that will take a callback to resolve links that will be used to canonicalize paths on various storage systems and add extensive tests. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 7 ++ tests/virstoragetest.c | 108 +++++++++++++++++++++++++ 4 files changed, 311 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6bd87e6..4ed08ca 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1873,6 +1873,7 @@ virStorageGenerateQcowPassphrase; # util/virstoragefile.h +virStorageFileCanonicalizePath; virStorageFileChainGetBroken; virStorageFileChainLookup; virStorageFileFeatureTypeFromString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 09b5d10..efae692 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -40,6 +40,7 @@ #include "virutil.h" #include "viruri.h" #include "dirname.h" +#include "virbuffer.h" #if HAVE_SYS_SYSCALL_H # include <sys/syscall.h> #endif @@ -1935,3 +1936,197 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent) return ret; } + + +static char * +virStorageFileCanonicalizeFormatPath(char **components, + size_t ncomponents, + bool beginSlash, + bool beginDoubleSlash) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + size_t i; + char *ret = NULL; + + if (beginSlash) + virBufferAddLit(&buf, "/"); + + if (beginDoubleSlash) + virBufferAddLit(&buf, "/"); + + for (i = 0; i < ncomponents; i++) { + if (i != 0) + virBufferAddLit(&buf, "/"); + + virBufferAdd(&buf, components[i], -1); + } + + if (virBufferError(&buf) != 0) { + virReportOOMError(); + return NULL; + } + + /* if the output string is empty just return an empty string */ + if (!(ret = virBufferContentAndReset(&buf))) + ignore_value(VIR_STRDUP(ret, "")); + + return ret; +} + + +static int +virStorageFileCanonicalizeInjectSymlink(const char *path, + size_t at, + char ***components, + size_t *ncomponents) +{ + char **tmp = NULL; + char **next; + size_t ntmp = 0; + int ret = -1; + + if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp))) + goto cleanup; + + /* prepend */ + for (next = tmp; *next; next++) { + if (VIR_INSERT_ELEMENT(*components, at, *ncomponents, *next) < 0) + goto cleanup; + + at++; + } + + ret = 0; + + cleanup: + virStringFreeListCount(tmp, ntmp); + return ret; +} + + +char * +virStorageFileCanonicalizePath(const char *path, + virStorageFileSimplifyPathReadlinkCallback cb, + void *cbdata) +{ + virHashTablePtr cycle = NULL; + bool beginSlash = false; + bool beginDoubleSlash = false; + char **components = NULL; + size_t ncomponents = 0; + char *linkpath = NULL; + char *currentpath = NULL; + size_t i = 0; + int rc; + char *ret = NULL; + + if (path[0] == '/') { + beginSlash = true; + + if (path[1] == '/' && path[2] != '/') + beginDoubleSlash = true; + } + + if (!(cycle = virHashCreate(10, NULL))) + goto cleanup; + + if (!(components = virStringSplitCount(path, "/", 0, &ncomponents))) + goto cleanup; + + while (i < ncomponents) { + /* skip slashes and '.'s */ + if (STREQ(components[i], "") || + STREQ(components[i], ".")) { + VIR_FREE(components[i]); + VIR_DELETE_ELEMENT(components, i, ncomponents); + continue; + } + + /* resolve changes to parent directory */ + if (STREQ(components[i], "..")) { + if (!beginSlash && + (i == 0 || STREQ(components[i - 1], ".."))) { + i++; + continue; + } + + VIR_FREE(components[i]); + VIR_DELETE_ELEMENT(components, i, ncomponents); + + if (i != 0) { + VIR_FREE(components[i - 1]); + VIR_DELETE_ELEMENT(components, i - 1, ncomponents); + i--; + } + + continue; + } + + /* check if the actual path isn't resulting into a symlink */ + if (!(currentpath = virStorageFileCanonicalizeFormatPath(components, + i + 1, + beginSlash, + beginDoubleSlash))) + goto cleanup; + + if ((rc = cb(currentpath, &linkpath, cbdata)) < 0) + goto cleanup; + + if (rc == 0) { + if (virHashLookup(cycle, currentpath)) { + virReportSystemError(ELOOP, + _("Failed to canonicalize path '%s'"), path); + goto cleanup; + } + + if (virHashAddEntry(cycle, currentpath, (void *) 1) < 0) + goto cleanup; + + if (linkpath[0] == '/') { + /* kill everything from the beginning including the actual component */ + i++; + while (i--) { + VIR_FREE(components[0]); + VIR_DELETE_ELEMENT(components, 0, ncomponents); + } + beginSlash = true; + + if (linkpath[1] == '/' && linkpath[2] != '/') + beginDoubleSlash = true; + else + beginDoubleSlash = false; + + i = 0; + } else { + VIR_FREE(components[i]); + VIR_DELETE_ELEMENT(components, i, ncomponents); + } + + if (virStorageFileCanonicalizeInjectSymlink(linkpath, + i, + &components, + &ncomponents) < 0) + goto cleanup; + + VIR_FREE(linkpath); + VIR_FREE(currentpath); + + continue; + } + + VIR_FREE(currentpath); + + i++; + } + + ret = virStorageFileCanonicalizeFormatPath(components, ncomponents, + beginSlash, beginDoubleSlash); + + cleanup: + virHashFree(cycle); + virStringFreeListCount(components, ncomponents); + VIR_FREE(linkpath); + VIR_FREE(currentpath); + + return ret; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 34b3625..fd5c89e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -325,5 +325,12 @@ void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceClearBackingStore(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); +typedef int +(*virStorageFileSimplifyPathReadlinkCallback)(const char *path, + char **link, + void *data); +char *virStorageFileCanonicalizePath(const char *path, + virStorageFileSimplifyPathReadlinkCallback cb, + void *cbdata); #endif /* __VIR_STORAGE_FILE_H__ */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index e15578c..0bc4a42 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -525,6 +525,71 @@ testStorageLookup(const void *args) return ret; } + +struct testPathCanonicalizeData +{ + const char *path; + const char *expect; +}; + +static const char *testPathCanonicalizeSymlinks[][2] = +{ + {"/path/blah", "/other/path/huzah"}, + {"/path/to/relative/symlink", "../../actual/file"}, + {"/cycle", "/cycle"}, + {"/cycle2/link", "./link"}, +}; + +static int +testPathCanonicalizeReadlink(const char *path, + char **link, + void *data ATTRIBUTE_UNUSED) +{ + size_t i; + + *link = NULL; + + for (i = 0; i < ARRAY_CARDINALITY(testPathCanonicalizeSymlinks); i++) { + if (STREQ(path, testPathCanonicalizeSymlinks[i][0])) { + if (VIR_STRDUP(*link, testPathCanonicalizeSymlinks[i][1]) < 0) + return -1; + + return 0; + } + } + + return 1; +} + + +static int +testPathCanonicalize(const void *args) +{ + const struct testPathCanonicalizeData *data = args; + char *canon = NULL; + int ret = -1; + + canon = virStorageFileCanonicalizePath(data->path, + testPathCanonicalizeReadlink, + NULL); + + if (STRNEQ_NULLABLE(data->expect, canon)) { + fprintf(stderr, + "path canonicalization of '%s' failed: expected '%s' got '%s'\n", + data->path, NULLSTR(data->expect), NULLSTR(canon)); + + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(canon); + + return ret; +} + + static int mymain(void) { @@ -532,6 +597,7 @@ mymain(void) virCommandPtr cmd = NULL; struct testChainData data; struct testLookupData data2; + struct testPathCanonicalizeData data3; virStorageSourcePtr chain = NULL; virStorageSourcePtr chain2; /* short for chain->backingStore */ virStorageSourcePtr chain3; /* short for chain2->backingStore */ @@ -1072,6 +1138,48 @@ mymain(void) TEST_LOOKUP_TARGET(80, "vda", chain3, "vda[2]", 2, NULL, NULL, NULL); TEST_LOOKUP_TARGET(81, "vda", NULL, "vda[3]", 3, NULL, NULL, NULL); +#define TEST_PATH_CANONICALIZE(id, PATH, EXPECT) \ + do { \ + data3.path = PATH; \ + data3.expect = EXPECT; \ + if (virtTestRun("Path canonicalize " #id, \ + testPathCanonicalize, &data3) < 0) \ + ret = -1; \ + } while (0) + + TEST_PATH_CANONICALIZE(1, "/", "/"); + TEST_PATH_CANONICALIZE(2, "/path", "/path"); + TEST_PATH_CANONICALIZE(3, "/path/to/blah", "/path/to/blah"); + TEST_PATH_CANONICALIZE(4, "/path/", "/path"); + TEST_PATH_CANONICALIZE(5, "///////", "/"); + TEST_PATH_CANONICALIZE(6, "//", "//"); + TEST_PATH_CANONICALIZE(7, "", ""); + TEST_PATH_CANONICALIZE(8, ".", ""); + TEST_PATH_CANONICALIZE(9, "../", ".."); + TEST_PATH_CANONICALIZE(10, "../../", "../.."); + TEST_PATH_CANONICALIZE(11, "../../blah", "../../blah"); + TEST_PATH_CANONICALIZE(12, "/./././blah", "/blah"); + TEST_PATH_CANONICALIZE(13, ".././../././../blah", "../../../blah"); + TEST_PATH_CANONICALIZE(14, "/././", "/"); + TEST_PATH_CANONICALIZE(15, "./././", ""); + TEST_PATH_CANONICALIZE(16, "blah/../foo", "foo"); + TEST_PATH_CANONICALIZE(17, "foo/bar/../blah", "foo/blah"); + TEST_PATH_CANONICALIZE(18, "foo/bar/.././blah", "foo/blah"); + TEST_PATH_CANONICALIZE(19, "/path/to/foo/bar/../../../../../../../../baz", "/baz"); + TEST_PATH_CANONICALIZE(20, "path/to/foo/bar/../../../../../../../../baz", "../../../../baz"); + TEST_PATH_CANONICALIZE(21, "path/to/foo/bar", "path/to/foo/bar"); + TEST_PATH_CANONICALIZE(22, "//foo//bar", "//foo/bar"); + TEST_PATH_CANONICALIZE(23, "/bar//foo", "/bar/foo"); + TEST_PATH_CANONICALIZE(24, "//../blah", "//blah"); + + /* test paths with symlinks */ + TEST_PATH_CANONICALIZE(25, "/path/blah", "/other/path/huzah"); + TEST_PATH_CANONICALIZE(26, "/path/to/relative/symlink", "/path/actual/file"); + TEST_PATH_CANONICALIZE(27, "/path/to/relative/symlink/blah", "/path/actual/file/blah"); + TEST_PATH_CANONICALIZE(28, "/path/blah/yippee", "/other/path/huzah/yippee"); + TEST_PATH_CANONICALIZE(29, "/cycle", NULL); + TEST_PATH_CANONICALIZE(30, "/cycle2/link", NULL); + cleanup: /* Final cleanup */ virStorageSourceFree(chain); -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Introduce a common function that will take a callback to resolve links that will be used to canonicalize paths on various storage systems and add extensive tests. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 7 ++ tests/virstoragetest.c | 108 +++++++++++++++++++++++++ 4 files changed, 311 insertions(+)
+static int +virStorageFileCanonicalizeInjectSymlink(const char *path, + size_t at, + char ***components, + size_t *ncomponents) +{ + char **tmp = NULL; + char **next; + size_t ntmp = 0; + int ret = -1; + + if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp))) + goto cleanup; + + /* prepend */ + for (next = tmp; *next; next++) { + if (VIR_INSERT_ELEMENT(*components, at, *ncomponents, *next) < 0) + goto cleanup;
Hmm. Laine's VIR_INSERT_ macros have so far only been used to insert one element at a time, but the underlying functions that the macros call are capable of inserting an array of elements in one go, which is more efficient. I wonder if it's worth adding a macro to expose insertion of an array, which we could then use here. But I won't insist - your series is already long enough that it could be done another day.
+ + TEST_PATH_CANONICALIZE(1, "/", "/"); + TEST_PATH_CANONICALIZE(2, "/path", "/path"); + TEST_PATH_CANONICALIZE(3, "/path/to/blah", "/path/to/blah"); + TEST_PATH_CANONICALIZE(4, "/path/", "/path"); + TEST_PATH_CANONICALIZE(5, "///////", "/"); + TEST_PATH_CANONICALIZE(6, "//", "//");
Please also test: "///" canonicalizes to "/".
+ TEST_PATH_CANONICALIZE(7, "", ""); + TEST_PATH_CANONICALIZE(8, ".", "");
I'm a bit fuzzy on whether this is a good idea; the empty string is required to fail with ENOENT, while a lone "." is worth preserving as-is. I think test 7 is okay, but if you could tweak the code to make test 8 return ".", I'd be happier (that is, special case the deletion of "." to not do that if it is the only remaining element in the array).
+ TEST_PATH_CANONICALIZE(9, "../", ".."); + TEST_PATH_CANONICALIZE(10, "../../", "../.."); + TEST_PATH_CANONICALIZE(11, "../../blah", "../../blah"); + TEST_PATH_CANONICALIZE(12, "/./././blah", "/blah"); + TEST_PATH_CANONICALIZE(13, ".././../././../blah", "../../../blah"); + TEST_PATH_CANONICALIZE(14, "/././", "/"); + TEST_PATH_CANONICALIZE(15, "./././", "");
Another case where I'd be happier if this returns "." (so again, if you special case that "." is deleted unless it is the last element of the simplified array)
+ TEST_PATH_CANONICALIZE(16, "blah/../foo", "foo"); + TEST_PATH_CANONICALIZE(17, "foo/bar/../blah", "foo/blah");
ACK if you can make those changes (you may want to post the interdiff) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/20/14 16:35, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Introduce a common function that will take a callback to resolve links that will be used to canonicalize paths on various storage systems and add extensive tests. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 7 ++ tests/virstoragetest.c | 108 +++++++++++++++++++++++++ 4 files changed, 311 insertions(+)
ACK if you can make those changes (you may want to post the interdiff)
The required changes to pass your suggested changes are: diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index be33398..613ba3c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2040,6 +2040,7 @@ virStorageFileCanonicalizePath(const char *path, char *linkpath = NULL; char *currentpath = NULL; size_t i = 0; + size_t j = 0; int rc; char *ret = NULL; @@ -2056,10 +2057,21 @@ virStorageFileCanonicalizePath(const char *path, if (!(components = virStringSplitCount(path, "/", 0, &ncomponents))) goto cleanup; + j = 0; + while (j < ncomponents) { + /* skip slashes */ + if (STREQ(components[j], "")) { + VIR_FREE(components[j]); + VIR_DELETE_ELEMENT(components, j, ncomponents); + continue; + } + j++; + } + while (i < ncomponents) { - /* skip slashes and '.'s */ - if (STREQ(components[i], "") || - STREQ(components[i], ".")) { + /* skip '.'s unless it's the last one remaining */ + if (STREQ(components[i], ".") && + (beginSlash || ncomponents > 1)) { VIR_FREE(components[i]); VIR_DELETE_ELEMENT(components, i, ncomponents); continue; @@ -2131,6 +2143,17 @@ virStorageFileCanonicalizePath(const char *path, &ncomponents) < 0) goto cleanup; + j = 0; + while (j < ncomponents) { + /* skip slashes */ + if (STREQ(components[j], "")) { + VIR_FREE(components[j]); + VIR_DELETE_ELEMENT(components, j, ncomponents); + continue; + } + j++; + } + VIR_FREE(linkpath); VIR_FREE(currentpath); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 0bc4a42..f86d25c 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1154,14 +1154,14 @@ mymain(void) TEST_PATH_CANONICALIZE(5, "///////", "/"); TEST_PATH_CANONICALIZE(6, "//", "//"); TEST_PATH_CANONICALIZE(7, "", ""); - TEST_PATH_CANONICALIZE(8, ".", ""); + TEST_PATH_CANONICALIZE(8, ".", "."); TEST_PATH_CANONICALIZE(9, "../", ".."); TEST_PATH_CANONICALIZE(10, "../../", "../.."); TEST_PATH_CANONICALIZE(11, "../../blah", "../../blah"); TEST_PATH_CANONICALIZE(12, "/./././blah", "/blah"); TEST_PATH_CANONICALIZE(13, ".././../././../blah", "../../../blah"); TEST_PATH_CANONICALIZE(14, "/././", "/"); - TEST_PATH_CANONICALIZE(15, "./././", ""); + TEST_PATH_CANONICALIZE(15, "./././", "."); TEST_PATH_CANONICALIZE(16, "blah/../foo", "foo"); TEST_PATH_CANONICALIZE(17, "foo/bar/../blah", "foo/blah"); TEST_PATH_CANONICALIZE(18, "foo/bar/.././blah", "foo/blah"); @@ -1179,6 +1179,7 @@ mymain(void) TEST_PATH_CANONICALIZE(28, "/path/blah/yippee", "/other/path/huzah/yippee"); TEST_PATH_CANONICALIZE(29, "/cycle", NULL); TEST_PATH_CANONICALIZE(30, "/cycle2/link", NULL); + TEST_PATH_CANONICALIZE(31, "///", "/"); cleanup: /* Final cleanup */ Peter
On 06/23/2014 02:28 AM, Peter Krempa wrote:
On 06/20/14 16:35, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Introduce a common function that will take a callback to resolve links that will be used to canonicalize paths on various storage systems and add extensive tests. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 7 ++ tests/virstoragetest.c | 108 +++++++++++++++++++++++++ 4 files changed, 311 insertions(+)
ACK if you can make those changes (you may want to post the interdiff)
The required changes to pass your suggested changes are:
+++ b/tests/virstoragetest.c @@ -1154,14 +1154,14 @@ mymain(void) TEST_PATH_CANONICALIZE(5, "///////", "/"); TEST_PATH_CANONICALIZE(6, "//", "//"); TEST_PATH_CANONICALIZE(7, "", ""); - TEST_PATH_CANONICALIZE(8, ".", ""); + TEST_PATH_CANONICALIZE(8, ".", "."); TEST_PATH_CANONICALIZE(9, "../", ".."); TEST_PATH_CANONICALIZE(10, "../../", "../.."); TEST_PATH_CANONICALIZE(11, "../../blah", "../../blah"); TEST_PATH_CANONICALIZE(12, "/./././blah", "/blah"); TEST_PATH_CANONICALIZE(13, ".././../././../blah", "../../../blah"); TEST_PATH_CANONICALIZE(14, "/././", "/"); - TEST_PATH_CANONICALIZE(15, "./././", ""); + TEST_PATH_CANONICALIZE(15, "./././", "."); TEST_PATH_CANONICALIZE(16, "blah/../foo", "foo"); TEST_PATH_CANONICALIZE(17, "foo/bar/../blah", "foo/blah"); TEST_PATH_CANONICALIZE(18, "foo/bar/.././blah", "foo/blah"); @@ -1179,6 +1179,7 @@ mymain(void) TEST_PATH_CANONICALIZE(28, "/path/blah/yippee", "/other/path/huzah/yippee"); TEST_PATH_CANONICALIZE(29, "/cycle", NULL); TEST_PATH_CANONICALIZE(30, "/cycle2/link", NULL); + TEST_PATH_CANONICALIZE(31, "///", "/");
Looks good. ACK to the squash-in. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use virStorageFileSimplifyPathInternal to canonicalize gluster paths via a callback and use it for the unique volume path retrieval API. --- src/storage/storage_backend_gluster.c | 80 +++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index b96d116..c8077a1 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -534,6 +534,7 @@ typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; struct _virStorageFileBackendGlusterPriv { glfs_t *vol; + char *canonpath; }; @@ -548,6 +549,7 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) if (priv->vol) glfs_fini(priv->vol); + VIR_FREE(priv->canonpath); VIR_FREE(priv); src->drv->priv = NULL; @@ -713,6 +715,80 @@ virStorageFileBackendGlusterAccess(virStorageSourcePtr src, return glfs_access(priv->vol, src->path, mode); } +static int +virStorageFileBackendGlusterReadlinkCallback(const char *path, + char **link, + void *data) +{ + virStorageFileBackendGlusterPrivPtr priv = data; + char *buf = NULL; + size_t bufsiz = 0; + ssize_t ret; + struct stat st; + + *link = NULL; + + if (glfs_stat(priv->vol, path, &st) < 0) { + virReportSystemError(errno, + _("failed to stat gluster path '%s'"), + path); + return -1; + } + + if (!S_ISLNK(st.st_mode)) + return 1; + + realloc: + if (VIR_EXPAND_N(buf, bufsiz, 256) < 0) + goto error; + + if ((ret = glfs_readlink(priv->vol, path, buf, bufsiz)) < 0) { + virReportSystemError(errno, + _("failed to read link of gluster file '%s'"), + path); + goto error; + } + + if (ret == bufsiz) + goto realloc; + + buf[ret] = '\0'; + + *link = buf; + + return 0; + + error: + VIR_FREE(buf); + return -1; +} + + +static const char * +virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + char *filePath = NULL; + + if (priv->canonpath) + return priv->canonpath; + + if (!(filePath = virStorageFileCanonicalizePath(src->path, + virStorageFileBackendGlusterReadlinkCallback, + priv))) + return NULL; + + ignore_value(virAsprintf(&priv->canonpath, "gluster://%s:%s/%s/%s", + src->hosts->name, + src->hosts->port, + src->volume, + filePath)); + + VIR_FREE(filePath); + + return priv->canonpath; +} + virStorageFileBackend virStorageFileBackendGluster = { .type = VIR_STORAGE_TYPE_NETWORK, @@ -725,4 +801,8 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileStat = virStorageFileBackendGlusterStat, .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, .storageFileAccess = virStorageFileBackendGlusterAccess, + + .storageFileGetUniqueIdentifier = virStorageFileBackendGlusterGetUniqueIdentifier, + + }; -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Use virStorageFileSimplifyPathInternal to canonicalize gluster paths via a callback and use it for the unique volume path retrieval API. --- src/storage/storage_backend_gluster.c | 80 +++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
+ + realloc: + if (VIR_EXPAND_N(buf, bufsiz, 256) < 0)
Expanding by the same length each iteration is quadratic in behavior, compared to expanding by a geometrically larger value (256 on iteration 1, 512 on iteration 2, ...). BUT, that is true only if you return to the label more than once. However, gluster has some (current) hard-baked limits of 256 as the maximum length, so you will only be repeating the label at most once, so it really doesn't matter in this patch :) ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/24/14 05:35, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Use virStorageFileSimplifyPathInternal to canonicalize gluster paths via a callback and use it for the unique volume path retrieval API. --- src/storage/storage_backend_gluster.c | 80 +++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
+ + realloc: + if (VIR_EXPAND_N(buf, bufsiz, 256) < 0)
Expanding by the same length each iteration is quadratic in behavior, compared to expanding by a geometrically larger value (256 on iteration 1, 512 on iteration 2, ...). BUT, that is true only if you return to the label more than once. However, gluster has some (current) hard-baked limits of 256 as the maximum length, so you will only be repeating the label at most once, so it really doesn't matter in this patch :)
Also having links longer than that seems insane. This loop will not be iterated much.
ACK
I've pushed 1-3 which enables libvirt to work with gluster backing chains now. I'll repost the rest as it depends on your active commit series and the last version caused rebase conflicts. Peter
This patch introduces a function that will allow us to resolve a relative difference between two elements of a disk backing chain. This fucntion will be used to allow relative block commit and block pull where we need to specify the new relative name of the image to qemu. This patch also adds unit tests for the function to verify that it works correctly. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 77 ++++++++++++++++++++++++ src/util/virstoragefile.h | 4 ++ tests/virstoragetest.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 228 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4ed08ca..8d5df87 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1884,6 +1884,7 @@ virStorageFileGetLVMKey; virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; virStorageFileGetMetadataInternal; +virStorageFileGetRelativeBackingPath; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index efae692..ac3bcba 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2130,3 +2130,80 @@ virStorageFileCanonicalizePath(const char *path, return ret; } + + +static char * +virStorageFileRemoveLastPathComponent(const char *path) +{ + char *tmp; + char *ret; + + if (VIR_STRDUP(ret, path ? path : "") < 0) + return NULL; + + if ((tmp = strrchr(ret, '/'))) + tmp[1] = '\0'; + else + ret[0] = '\0'; + + return ret; +} + + +/* + * virStorageFileGetRelativeBackingPath: + * + * Resolve relative path to be written to the overlay of @top image when + * collapsing the backing chain between @top and @base. + * + * Returns 0 on success; 1 if backing chain isn't relative and -1 on error. + */ +int +virStorageFileGetRelativeBackingPath(virStorageSourcePtr top, + virStorageSourcePtr base, + char **relpath) +{ + virStorageSourcePtr next; + char *tmp = NULL; + char *path = NULL; + char ret = -1; + + *relpath = NULL; + + for (next = top; next; next = next->backingStore) { + if (!next->backingRelative || !next->relPath) { + ret = 1; + goto cleanup; + } + + if (!(tmp = virStorageFileRemoveLastPathComponent(path))) + goto cleanup; + + VIR_FREE(path); + + if (virAsprintf(&path, "%s%s", tmp, next->relPath) < 0) + goto cleanup; + + VIR_FREE(tmp); + + if (next == base) + break; + } + + if (next != base) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to resolve relative backing name: " + "base image is not in backing chain")); + goto cleanup; + } + + *relpath = path; + path = NULL; + + ret = 0; + + cleanup: + VIR_FREE(path); + VIR_FREE(tmp); + return ret; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index fd5c89e..ff8130d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -333,4 +333,8 @@ char *virStorageFileCanonicalizePath(const char *path, virStorageFileSimplifyPathReadlinkCallback cb, void *cbdata); +int virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, + virStorageSourcePtr to, + char **relpath); + #endif /* __VIR_STORAGE_FILE_H__ */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 0bc4a42..72c7ca2 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -589,6 +589,104 @@ testPathCanonicalize(const void *args) return ret; } +virStorageSource backingchain[12]; + +static void +testPathRelativePrepare(void) +{ + size_t i; + + for (i = 0; i < ARRAY_CARDINALITY(backingchain); i++) { + if (i < ARRAY_CARDINALITY(backingchain) - 1) + backingchain[i].backingStore = &backingchain[i+1]; + else + backingchain[i].backingStore = NULL; + + backingchain[i].backingRelative = true; + } + + /* normal relative backing chain */ + backingchain[0].path = (char *) "/path/to/some/img"; + backingchain[0].relPath = (char *) "/path/to/some/img"; + backingchain[0].backingRelative = false; + + backingchain[1].path = (char *) "/path/to/some/asdf"; + backingchain[1].relPath = (char *) "asdf"; + + backingchain[2].path = (char *) "/path/to/some/test"; + backingchain[2].relPath = (char *) "test"; + + backingchain[3].path = (char *) "/path/to/some/blah"; + backingchain[3].relPath = (char *) "blah"; + + /* ovirt's backing chain */ + backingchain[4].path = (char *) "/path/to/volume/image1"; + backingchain[4].relPath = (char *) "/path/to/volume/image1"; + backingchain[4].backingRelative = false; + + backingchain[5].path = (char *) "/path/to/volume/image2"; + backingchain[5].relPath = (char *) "../volume/image2"; + + backingchain[6].path = (char *) "/path/to/volume/image3"; + backingchain[6].relPath = (char *) "../volume/image3"; + + backingchain[7].path = (char *) "/path/to/volume/image4"; + backingchain[7].relPath = (char *) "../volume/image4"; + + /* some arbitrarily crazy backing chains */ + backingchain[8].path = (char *) "/crazy/base/image"; + backingchain[8].relPath = (char *) "/crazy/base/image"; + backingchain[8].backingRelative = false; + + backingchain[9].path = (char *) "/crazy/base/directory/stuff/volumes/garbage/image2"; + backingchain[9].relPath = (char *) "directory/stuff/volumes/garbage/image2"; + + backingchain[10].path = (char *) "/crazy/base/directory/image3"; + backingchain[10].relPath = (char *) "../../../image3"; + + backingchain[11].path = (char *) "/crazy/base/blah/image4"; + backingchain[11].relPath = (char *) "../blah/image4"; +} + + +struct testPathRelativeBacking +{ + virStorageSourcePtr top; + virStorageSourcePtr base; + + const char *expect; +}; + +static int +testPathRelative(const void *args) +{ + const struct testPathRelativeBacking *data = args; + char *actual = NULL; + int ret = -1; + + if (virStorageFileGetRelativeBackingPath(data->top, + data->base, + &actual) < 0) { + fprintf(stderr, "relative backing path resolution failed\n"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(data->expect, actual)) { + fprintf(stderr, "relative path resolution from '%s' to '%s': " + "expected '%s', got '%s'\n", + data->top->path, data->base->path, + NULLSTR(data->expect), NULLSTR(actual)); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(actual); + + return ret; +} + static int mymain(void) @@ -598,6 +696,7 @@ mymain(void) struct testChainData data; struct testLookupData data2; struct testPathCanonicalizeData data3; + struct testPathRelativeBacking data4; virStorageSourcePtr chain = NULL; virStorageSourcePtr chain2; /* short for chain->backingStore */ virStorageSourcePtr chain3; /* short for chain2->backingStore */ @@ -1180,6 +1279,53 @@ mymain(void) TEST_PATH_CANONICALIZE(29, "/cycle", NULL); TEST_PATH_CANONICALIZE(30, "/cycle2/link", NULL); +#define TEST_RELATIVE_BACKING(id, TOP, BASE, EXPECT) \ + do { \ + data4.top = &TOP; \ + data4.base = &BASE; \ + data4.expect = EXPECT; \ + if (virtTestRun("Path relative resolve " #id, \ + testPathRelative, &data4) < 0) \ + ret = -1; \ + } while (0) + + testPathRelativePrepare(); + + /* few negative tests first */ + + /* a non-relative image is in the backing chain span */ + TEST_RELATIVE_BACKING(1, backingchain[0], backingchain[1], NULL); + TEST_RELATIVE_BACKING(2, backingchain[0], backingchain[2], NULL); + TEST_RELATIVE_BACKING(3, backingchain[0], backingchain[3], NULL); + TEST_RELATIVE_BACKING(4, backingchain[1], backingchain[5], NULL); + + /* image is not in chain (specified backwards) */ + TEST_RELATIVE_BACKING(5, backingchain[2], backingchain[1], NULL); + + /* positive tests */ + TEST_RELATIVE_BACKING(6, backingchain[1], backingchain[1], "asdf"); + TEST_RELATIVE_BACKING(7, backingchain[1], backingchain[2], "test"); + TEST_RELATIVE_BACKING(8, backingchain[1], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(9, backingchain[2], backingchain[2], "test"); + TEST_RELATIVE_BACKING(10, backingchain[2], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(11, backingchain[3], backingchain[3], "blah"); + + /* oVirt spelling */ + TEST_RELATIVE_BACKING(12, backingchain[5], backingchain[5], "../volume/image2"); + TEST_RELATIVE_BACKING(13, backingchain[5], backingchain[6], "../volume/../volume/image3"); + TEST_RELATIVE_BACKING(14, backingchain[5], backingchain[7], "../volume/../volume/../volume/image4"); + TEST_RELATIVE_BACKING(15, backingchain[6], backingchain[6], "../volume/image3"); + TEST_RELATIVE_BACKING(16, backingchain[6], backingchain[7], "../volume/../volume/image4"); + TEST_RELATIVE_BACKING(17, backingchain[7], backingchain[7], "../volume/image4"); + + /* crazy spellings */ + TEST_RELATIVE_BACKING(17, backingchain[9], backingchain[9], "directory/stuff/volumes/garbage/image2"); + TEST_RELATIVE_BACKING(18, backingchain[9], backingchain[10], "directory/stuff/volumes/garbage/../../../image3"); + TEST_RELATIVE_BACKING(19, backingchain[9], backingchain[11], "directory/stuff/volumes/garbage/../../../../blah/image4"); + TEST_RELATIVE_BACKING(20, backingchain[10], backingchain[10], "../../../image3"); + TEST_RELATIVE_BACKING(21, backingchain[10], backingchain[11], "../../../../blah/image4"); + TEST_RELATIVE_BACKING(22, backingchain[11], backingchain[11], "../blah/image4"); + cleanup: /* Final cleanup */ virStorageSourceFree(chain); -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
This patch introduces a function that will allow us to resolve a relative difference between two elements of a disk backing chain. This fucntion will be used to allow relative block commit and block pull
s/fucntion/function/
where we need to specify the new relative name of the image to qemu.
This patch also adds unit tests for the function to verify that it works correctly. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 77 ++++++++++++++++++++++++ src/util/virstoragefile.h | 4 ++ tests/virstoragetest.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 228 insertions(+)
Disclaimer - stream-of-consciousness review below. Read the whole mail before making any changes.
+ +static char * +virStorageFileRemoveLastPathComponent(const char *path) +{ + char *tmp; + char *ret; + + if (VIR_STRDUP(ret, path ? path : "") < 0) + return NULL; + + if ((tmp = strrchr(ret, '/'))) + tmp[1] = '\0';
So, for virStorageFileRemoveLastPathComponent("a//"), you return "a//", rather than the empty string. Is this intentional? Or will this function never be called with a trailing slash? I think the correct approach is to first remove all trailing slashes, before doing the strrchr for slash. As a special case, if the input is only slashes, then we might want to return "/" or "//" instead of stripping them all the way to "".
+/* + * virStorageFileGetRelativeBackingPath: + * + * Resolve relative path to be written to the overlay of @top image when + * collapsing the backing chain between @top and @base.
That is, given: base <- top <- overlay if top had a relative reference to base, and we are committing top into base, then we want to end up with the relative string to write so that we have: base <- overlay at the end of the operation. But fundamentally, I'm not sure if you have enough information given this function signature. Consider: /path/one/base <- /path/two/top (backing: ../one/base) <- /path/three/deep/overlay (backing: /path/two/top) vs. /path/one/base <- /path/two/top (backing: ../one/base) <- /path/three/deep/overlay2 (backing: ../../two/base) Going just off this description, I'm guessing your function would return: ../one/base, but that is NOT the correct relative name to be sticking in /path/three/deep/overlay (correct would be ../../one/base). But unless you know whether the overlay was using absolute or relative, or even whether the overlay is in the same directory as top, you can't know that the relative relation from top to base is still the correct relation from overlay to base.
+ * + * Returns 0 on success; 1 if backing chain isn't relative and -1 on error. + */ +int +virStorageFileGetRelativeBackingPath(virStorageSourcePtr top, + virStorageSourcePtr base, + char **relpath) +{ + virStorageSourcePtr next; + char *tmp = NULL; + char *path = NULL; + char ret = -1; + + *relpath = NULL; + + for (next = top; next; next = next->backingStore) { + if (!next->backingRelative || !next->relPath) { + ret = 1; + goto cleanup; + } + + if (!(tmp = virStorageFileRemoveLastPathComponent(path)))
How does this even work? Given my example above, top (and thus next) starts life as the storageSource pointing to /path/two/top, so next->backingRelative is true, and next->relPath is ../one/base. But 'path' is still NULL on the first iteration through the loop, so tmp = "".
+ goto cleanup; + + VIR_FREE(path); + + if (virAsprintf(&path, "%s%s", tmp, next->relPath) < 0) + goto cleanup;
and path then gets set to "../one/base".
+ + VIR_FREE(tmp); + + if (next == base) + break; + }
then we iterate. On the second iteration, next now points to the storageSource for /path/one/base, which has no backing file, so next->backingRelative is false, and this function returns 1. Or am I misunderstanding the arguments you pass to this function? Oh, does next->relPath refer to the path that the overlay used to open next with, rather than the relative path from next to its backing file? Okay, let's rework the example with that interpretation. On entry top (and thus next) starts life as the virStorageSource with next->backingRelative set to whether the overlay used a relative source. In the case of starting from /path/three/deep/overlay, the answer is false, so we break out with no relative path possible; in the case of /path/three/deep/overlay2, the answer is true and next/relPath is "../../two/top" (that is, the relative name that the overlay used in order to access next). So on the first iteration, path is NULL, tmp is "", then path is "../../two/top", then we iterate. On the second iteration, next->backingRelative is still true, and this time next->relPath is "../one/base" (the path used to get from the overlay into /path/one/base), tmp gets "../../two", and the loop ends with path being "../../two/../one/base". Okay, that makes a LOT more sense. It also seems to answer my above concern: it looks like virStorageFileRemoveLastPathComponent is only ever called with strings representing image file names (so we don't have to worry about the all slashes case, or with names ending in slashes, since neither of those cases would have resulted in a virStorageSourcePtr being opened in the first place). But it would still be worth a comment to that effect, that the input string must not have a trailing slash. /me goes and rereads the .h file: /* Name of the current file as spelled by the user (top level) or * metadata of the overlay (if this is a backing store). */ char *relPath; ah, so my first interpretation was wrong (next->relPath is not the name of next's backingStore), and the second was right (next->relPath is the name that we used in finding next). I've glanced ahead, and see how later patches in your series tries to make more sense of some of the fields in virStorageSource (deleting redundancy, and/or renaming things to be nicer); hopefully it's not too confusing at the end of the day with whatever we end up with.
+++ b/src/util/virstoragefile.h @@ -333,4 +333,8 @@ char *virStorageFileCanonicalizePath(const char *path, virStorageFileSimplifyPathReadlinkCallback cb, void *cbdata);
+int virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, + virStorageSourcePtr to, + char **relpath);
Probably worth ATTRIBUTE_NONNULL for all 3 parameters
+++ b/tests/virstoragetest.c @@ -589,6 +589,104 @@ testPathCanonicalize(const void *args) return ret; }
+virStorageSource backingchain[12];
Mark this as static.
+ +static void +testPathRelativePrepare(void) +{ + size_t i; + + for (i = 0; i < ARRAY_CARDINALITY(backingchain); i++) { + if (i < ARRAY_CARDINALITY(backingchain) - 1) + backingchain[i].backingStore = &backingchain[i+1];
Spaces around +
+ else + backingchain[i].backingStore = NULL; + + backingchain[i].backingRelative = true; + } + + /* normal relative backing chain */ + backingchain[0].path = (char *) "/path/to/some/img"; + backingchain[0].relPath = (char *) "/path/to/some/img";
Nasty that we're casting away const, but it works for this test. Okay, given my re-think of how it all works above
+ backingchain[0].backingRelative = false; + + backingchain[1].path = (char *) "/path/to/some/asdf"; + backingchain[1].relPath = (char *) "asdf";
So up to here, we have the chain: /path/to/some/asdf <- /path/to/some/img (backing: asdf)
+ + backingchain[2].path = (char *) "/path/to/some/test"; + backingchain[2].relPath = (char *) "test";
and here, we add another layer of base files: /path/to/some/test <- /path/to/some/asdf (backing: test)
+ + backingchain[3].path = (char *) "/path/to/some/blah"; + backingchain[3].relPath = (char *) "blah"; + + /* ovirt's backing chain */ + backingchain[4].path = (char *) "/path/to/volume/image1"; + backingchain[4].relPath = (char *) "/path/to/volume/image1"; + backingchain[4].backingRelative = false;
/path/to/volume/image1 <- /path/to/some/blah (backing: /path/to/volume/image1)
+ + backingchain[5].path = (char *) "/path/to/volume/image2"; + backingchain[5].relPath = (char *) "../volume/image2"; + + backingchain[6].path = (char *) "/path/to/volume/image3"; + backingchain[6].relPath = (char *) "../volume/image3"; + + backingchain[7].path = (char *) "/path/to/volume/image4"; + backingchain[7].relPath = (char *) "../volume/image4"; + + /* some arbitrarily crazy backing chains */ + backingchain[8].path = (char *) "/crazy/base/image"; + backingchain[8].relPath = (char *) "/crazy/base/image"; + backingchain[8].backingRelative = false; + + backingchain[9].path = (char *) "/crazy/base/directory/stuff/volumes/garbage/image2"; + backingchain[9].relPath = (char *) "directory/stuff/volumes/garbage/image2"; + + backingchain[10].path = (char *) "/crazy/base/directory/image3"; + backingchain[10].relPath = (char *) "../../../image3"; + + backingchain[11].path = (char *) "/crazy/base/blah/image4"; + backingchain[11].relPath = (char *) "../blah/image4"; +}
Okay, this chain makes sense.
+ testPathRelativePrepare(); + + /* few negative tests first */ + + /* a non-relative image is in the backing chain span */ + TEST_RELATIVE_BACKING(1, backingchain[0], backingchain[1], NULL);
Since we opened backingchain[0] /path/to/some/img as absolute, we cannot come up with a relative path for backingchain[1] /path/to/some/asdf to be used in the overlay of backingchain[0] (nevermind that backingchain[0] has no overlay, the point remains that after committing img into asdf, we want the absolute name of asdf in our XML). Correct.
+ TEST_RELATIVE_BACKING(2, backingchain[0], backingchain[2], NULL); + TEST_RELATIVE_BACKING(3, backingchain[0], backingchain[3], NULL); + TEST_RELATIVE_BACKING(4, backingchain[1], backingchain[5], NULL);
All correct.
+ + /* image is not in chain (specified backwards) */ + TEST_RELATIVE_BACKING(5, backingchain[2], backingchain[1], NULL); +
Correct.
+ /* positive tests */ + TEST_RELATIVE_BACKING(6, backingchain[1], backingchain[1], "asdf");
Huh? We never commit /path/to/some/asdf into itself. But I guess this notion (asking for the relative name when top == base) as shorthand for learning whether top was opened with a relative name doesn't hurt.
+ TEST_RELATIVE_BACKING(7, backingchain[1], backingchain[2], "test"); + TEST_RELATIVE_BACKING(8, backingchain[1], backingchain[3], "blah");
Both correct.
+ TEST_RELATIVE_BACKING(9, backingchain[2], backingchain[2], "test");
Another case of top==base; doesn't hurt.
+ TEST_RELATIVE_BACKING(10, backingchain[2], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(11, backingchain[3], backingchain[3], "blah"); + + /* oVirt spelling */ + TEST_RELATIVE_BACKING(12, backingchain[5], backingchain[5], "../volume/image2");
Another case of top==base.
+ TEST_RELATIVE_BACKING(13, backingchain[5], backingchain[6], "../volume/../volume/image3");
Yes, this is the one I was looking for. Of course, we could argue that the name might be simplified if we knew how to resolve .. arguments out of the string, but that can be a later patch. For the moment, this is a correct relative name, even if it is not the shortest possible.
+ TEST_RELATIVE_BACKING(14, backingchain[5], backingchain[7], "../volume/../volume/../volume/image4"); + TEST_RELATIVE_BACKING(15, backingchain[6], backingchain[6], "../volume/image3"); + TEST_RELATIVE_BACKING(16, backingchain[6], backingchain[7], "../volume/../volume/image4"); + TEST_RELATIVE_BACKING(17, backingchain[7], backingchain[7], "../volume/image4"); + + /* crazy spellings */ + TEST_RELATIVE_BACKING(17, backingchain[9], backingchain[9], "directory/stuff/volumes/garbage/image2"); + TEST_RELATIVE_BACKING(18, backingchain[9], backingchain[10], "directory/stuff/volumes/garbage/../../../image3"); + TEST_RELATIVE_BACKING(19, backingchain[9], backingchain[11], "directory/stuff/volumes/garbage/../../../../blah/image4"); + TEST_RELATIVE_BACKING(20, backingchain[10], backingchain[10], "../../../image3"); + TEST_RELATIVE_BACKING(21, backingchain[10], backingchain[11], "../../../../blah/image4"); + TEST_RELATIVE_BACKING(22, backingchain[11], backingchain[11], "../blah/image4");
And all of these make sense as well. Final summary: ACK. I think I pointed out some places where you could improve comments (such as documenting that the internal function will never be called with trailing '/'), or add attributes or fix spacing; but the overall code is correct, even if it took me longer than I would have liked to reach that conclusion (in part my fault for not reading the .h file sooner, and just assuming incorrectly what relPath meant). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that we changed ordering of the stored metadata so that the backing store is described by the child element the test should reflect this change too. Remove the expected backing store field as it's actually described by the next element in the backing chain, so there's no need for duplication. --- tests/virstoragetest.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 72c7ca2..d3dc007 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -282,7 +282,6 @@ testPrepImages(void) typedef struct _testFileData testFileData; struct _testFileData { - const char *expBackingStore; const char *expBackingStoreRaw; unsigned long long expCapacity; bool expEncrypted; @@ -315,12 +314,11 @@ struct testChainData static const char testStorageChainFormat[] = "chain member: %zu\n" - "store: %s\n" + "path:%s\n" "backingStoreRaw: %s\n" "capacity: %lld\n" "encryption: %d\n" "relPath:%s\n" - "path:%s\n" "relDir:%s\n" "type:%d\n" "format:%d\n"; @@ -387,23 +385,21 @@ testStorageChain(const void *args) : data->files[i]->relDirRel; if (virAsprintf(&expect, testStorageChainFormat, i, - NULLSTR(data->files[i]->expBackingStore), + NULLSTR(data->files[i]->path), NULLSTR(data->files[i]->expBackingStoreRaw), data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(expPath), - NULLSTR(data->files[i]->path), NULLSTR(expRelDir), data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, testStorageChainFormat, i, - NULLSTR(elt->backingStore ? elt->backingStore->path : NULL), + NULLSTR(elt->path), NULLSTR(elt->backingStoreRaw), elt->capacity, !!elt->encryption, NULLSTR(elt->relPath), - NULLSTR(elt->path), NULLSTR(elt->relDir), elt->type, elt->format) < 0) { @@ -766,7 +762,6 @@ mymain(void) /* Qcow2 file with relative raw backing, format provided */ raw.pathAbs = "raw"; testFileData qcow2 = { - .expBackingStore = canonraw, .expBackingStoreRaw = "raw", .expCapacity = 1024, .pathRel = "qcow2", @@ -822,7 +817,6 @@ mymain(void) /* Wrapped file access */ testFileData wrap = { - .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .pathRel = "wrap", @@ -858,7 +852,6 @@ mymain(void) /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { - .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .pathRel = "wrap", @@ -882,7 +875,6 @@ mymain(void) "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = datadir "/bogus"; qcow2.pathRel = "qcow2"; qcow2.relDirRel = "."; @@ -915,7 +907,6 @@ mymain(void) "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2.expBackingStore = "blah"; qcow2.expBackingStoreRaw = "nbd:example.org:6000:exportname=blah"; /* Qcow2 file with backing protocol instead of file */ @@ -936,7 +927,6 @@ mymain(void) /* qed file */ testFileData qed = { - .expBackingStore = canonraw, .expBackingStoreRaw = absraw, .expCapacity = 1024, .pathRel = "qed", @@ -1001,7 +991,6 @@ mymain(void) /* Behavior of symlinks to qcow2 with relative backing files */ testFileData link1 = { - .expBackingStore = canonraw, .expBackingStoreRaw = "../raw", .expCapacity = 1024, .pathRel = "../sub/link1", @@ -1013,7 +1002,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; testFileData link2 = { - .expBackingStore = canonqcow2, .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, .pathRel = "sub/link2", @@ -1041,7 +1029,6 @@ mymain(void) "-F", "qcow2", "-b", "qcow2", "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = "qcow2"; /* Behavior of an infinite loop chain */ -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Now that we changed ordering of the stored metadata so that the backing store is described by the child element the test should reflect this change too.
Remove the expected backing store field as it's actually described by the next element in the backing chain, so there's no need for duplication. --- tests/virstoragetest.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-)
@@ -315,12 +314,11 @@ struct testChainData
static const char testStorageChainFormat[] = "chain member: %zu\n" - "store: %s\n" + "path:%s\n" "backingStoreRaw: %s\n"
Why no space after ':'?
"capacity: %lld\n" "encryption: %d\n" "relPath:%s\n" - "path:%s\n" "relDir:%s\n"
Oh - we have a mix of formats. It might be nice to be consistent; but as this format string is only ever used on failing tests, it doesn't matter in the normal case of all tests passing. ACK. Up to you if you want to tweak that string format. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Due to various refactors and compatibility with the virstoragetest the relPath field of the virStorageSource structure was always filled either with the relative name or the full path in case of abslutely backed storage. Return it's original purpose to store only the relative name of the disk if it is backed relatively and tweak the tests. --- src/storage/storage_driver.c | 4 ---- src/util/virstoragefile.c | 21 +++++++++------------ src/util/virstoragefile.h | 4 ++-- tests/virstoragetest.c | 25 +++---------------------- 4 files changed, 14 insertions(+), 40 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7116185..c6e2936 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2944,10 +2944,6 @@ virStorageFileGetMetadata(virStorageSourcePtr src, if (!(cycle = virHashCreate(5, NULL))) return -1; - if (!src->relPath && - VIR_STRDUP(src->relPath, src->path) < 0) - goto cleanup; - if (!src->relDir && !(src->relDir = mdir_name(src->path))) { virReportOOMError(); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ac3bcba..7b4e46a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -764,11 +764,11 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, { int ret = -1; - VIR_DEBUG("relPath=%s, buf=%p, len=%zu, meta->format=%d", - meta->relPath, buf, len, meta->format); + VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", + meta->path, buf, len, meta->format); if (meta->format == VIR_STORAGE_FILE_AUTO) - meta->format = virStorageFileProbeFormatFromBuf(meta->relPath, buf, len); + meta->format = virStorageFileProbeFormatFromBuf(meta->path, buf, len); if (meta->format <= VIR_STORAGE_FILE_NONE || meta->format >= VIR_STORAGE_FILE_LAST) { @@ -908,9 +908,6 @@ virStorageFileMetadataNew(const char *path, ret->format = format; ret->type = VIR_STORAGE_TYPE_FILE; - if (VIR_STRDUP(ret->relPath, path) < 0) - goto error; - if (VIR_STRDUP(ret->path, path) < 0) goto error; @@ -1378,7 +1375,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain, if (idx == i) break; } else { - if (STREQ_NULLABLE(name, chain->relPath)) + if (STREQ_NULLABLE(name, chain->relPath) || + STREQ(name, chain->path)) break; if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || chain->type == VIR_STORAGE_TYPE_BLOCK)) { @@ -1602,6 +1600,10 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, ret->backingRelative = true; + /* store relative name */ + if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) + goto error; + /* XXX Once we get rid of the need to use canonical names in path, we will be * able to use mdir_name on parent->path instead of using parent->relDir */ if (STRNEQ(parent->relDir, "/")) @@ -1916,11 +1918,6 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent) ret = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw); if (ret) { - if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) { - virStorageSourceFree(ret); - return NULL; - } - /* possibly update local type */ if (ret->type == VIR_STORAGE_TYPE_FILE) { if (stat(ret->path, &st) == 0) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index ff8130d..38d1720 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -247,8 +247,8 @@ struct _virStorageSource { virStorageDriverDataPtr drv; /* metadata about storage image which need separate fields */ - /* Name of the current file as spelled by the user (top level) or - * metadata of the overlay (if this is a backing store). */ + /* Relative path of the backing image from the parent NULL if + * backed by absolute path */ char *relPath; /* Directory to start from if backingStoreRaw is a relative file * name. */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index d3dc007..e2a06f2 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -116,9 +116,6 @@ testStorageFileGetMetadata(const char *path, } } - if (VIR_STRDUP(ret->relPath, path) < 0) - goto error; - if (!(ret->relDir = mdir_name(path))) { virReportOOMError(); goto error; @@ -371,7 +368,6 @@ testStorageChain(const void *args) while (elt) { char *expect = NULL; char *actual = NULL; - const char *expPath; const char *expRelDir; if (i == data->nfiles) { @@ -379,8 +375,6 @@ testStorageChain(const void *args) goto cleanup; } - expPath = isAbs ? data->files[i]->pathAbs - : data->files[i]->pathRel; expRelDir = isAbs ? data->files[i]->relDirAbs : data->files[i]->relDirRel; if (virAsprintf(&expect, @@ -389,7 +383,7 @@ testStorageChain(const void *args) NULLSTR(data->files[i]->expBackingStoreRaw), data->files[i]->expCapacity, data->files[i]->expEncrypted, - NULLSTR(expPath), + NULLSTR(data->files[i]->pathRel), NULLSTR(expRelDir), data->files[i]->type, data->files[i]->format) < 0 || @@ -740,7 +734,6 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { - .pathRel = "raw", .pathAbs = canonraw, .path = canonraw, .relDirRel = ".", @@ -761,10 +754,10 @@ mymain(void) /* Qcow2 file with relative raw backing, format provided */ raw.pathAbs = "raw"; + raw.pathRel = "raw"; testFileData qcow2 = { .expBackingStoreRaw = "raw", .expCapacity = 1024, - .pathRel = "qcow2", .pathAbs = canonqcow2, .path = canonqcow2, .relDirRel = ".", @@ -773,7 +766,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { - .pathRel = "qcow2", .pathAbs = canonqcow2, .path = canonqcow2, .relDirRel = ".", @@ -799,7 +791,7 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = absraw; - raw.pathRel = absraw; + raw.pathRel = NULL; raw.pathAbs = absraw; raw.relDirRel = datadir; @@ -819,7 +811,6 @@ mymain(void) testFileData wrap = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, - .pathRel = "wrap", .pathAbs = abswrap, .path = canonwrap, .relDirRel = ".", @@ -827,7 +818,6 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; - qcow2.pathRel = absqcow2; qcow2.relDirRel = datadir; TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2, &raw), EXP_PASS, @@ -847,14 +837,12 @@ mymain(void) "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_as_raw.pathRel = absqcow2; qcow2_as_raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, - .pathRel = "wrap", .pathAbs = abswrap, .path = canonwrap, .relDirRel = ".", @@ -876,7 +864,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = datadir "/bogus"; - qcow2.pathRel = "qcow2"; qcow2.relDirRel = "."; /* Qcow2 file with missing backing file but specified type */ @@ -911,7 +898,6 @@ mymain(void) /* Qcow2 file with backing protocol instead of file */ testFileData nbd = { - .pathRel = "nbd:example.org:6000:exportname=blah", .pathAbs = "nbd:example.org:6000:exportname=blah", .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, @@ -929,7 +915,6 @@ mymain(void) testFileData qed = { .expBackingStoreRaw = absraw, .expCapacity = 1024, - .pathRel = "qed", .pathAbs = absqed, .path = canonqed, .relDirRel = ".", @@ -938,7 +923,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { - .pathRel = "qed", .pathAbs = absqed, .path = canonqed, .relDirRel = ".", @@ -954,7 +938,6 @@ mymain(void) /* directory */ testFileData dir = { - .pathRel = "dir", .pathAbs = absdir, .path = canondir, .relDirRel = ".", @@ -1004,7 +987,6 @@ mymain(void) testFileData link2 = { .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, - .pathRel = "sub/link2", .pathAbs = abslink2, .path = canonwrap, .relDirRel = "sub", @@ -1051,7 +1033,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = "wrap"; - qcow2.pathRel = absqcow2; qcow2.relDirRel = datadir; /* Behavior of an infinite loop chain */ -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Due to various refactors and compatibility with the virstoragetest the relPath field of the virStorageSource structure was always filled either with the relative name or the full path in case of abslutely backed
s/abslutely/absolutely/
storage. Return it's original purpose to store only the relative name of
s/it's/its/
the disk if it is backed relatively and tweak the tests. --- src/storage/storage_driver.c | 4 ---- src/util/virstoragefile.c | 21 +++++++++------------ src/util/virstoragefile.h | 4 ++-- tests/virstoragetest.c | 25 +++---------------------- 4 files changed, 14 insertions(+), 40 deletions(-)
+++ b/src/util/virstoragefile.h @@ -247,8 +247,8 @@ struct _virStorageSource { virStorageDriverDataPtr drv;
/* metadata about storage image which need separate fields */ - /* Name of the current file as spelled by the user (top level) or - * metadata of the overlay (if this is a backing store). */ + /* Relative path of the backing image from the parent NULL if
s/parent/parent,/
+ * backed by absolute path */
My confusion in 4/19 was based on misinterpreting the purpose of this field. May I suggest: /* Relative name by which this image was opened from its parent, or NULL if this image was opened by absolute name */ ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Separately remove the now unused variable. --- tests/virstoragetest.c | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index e2a06f2..2fd5459 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -283,7 +283,6 @@ struct _testFileData unsigned long long expCapacity; bool expEncrypted; const char *pathRel; - const char *pathAbs; const char *path; const char *relDirRel; const char *relDirAbs; @@ -734,7 +733,6 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { - .pathAbs = canonraw, .path = canonraw, .relDirRel = ".", .relDirAbs = datadir, @@ -753,12 +751,10 @@ mymain(void) (&raw), ALLOW_PROBE | EXP_PASS); /* Qcow2 file with relative raw backing, format provided */ - raw.pathAbs = "raw"; raw.pathRel = "raw"; testFileData qcow2 = { .expBackingStoreRaw = "raw", .expCapacity = 1024, - .pathAbs = canonqcow2, .path = canonqcow2, .relDirRel = ".", .relDirAbs = datadir, @@ -766,7 +762,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { - .pathAbs = canonqcow2, .path = canonqcow2, .relDirRel = ".", .relDirAbs = datadir, @@ -792,7 +787,6 @@ mymain(void) ret = -1; qcow2.expBackingStoreRaw = absraw; raw.pathRel = NULL; - raw.pathAbs = absraw; raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format provided */ @@ -811,7 +805,6 @@ mymain(void) testFileData wrap = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, - .pathAbs = abswrap, .path = canonwrap, .relDirRel = ".", .relDirAbs = datadir, @@ -843,7 +836,6 @@ mymain(void) testFileData wrap_as_raw = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, - .pathAbs = abswrap, .path = canonwrap, .relDirRel = ".", .relDirAbs = datadir, @@ -898,7 +890,6 @@ mymain(void) /* Qcow2 file with backing protocol instead of file */ testFileData nbd = { - .pathAbs = "nbd:example.org:6000:exportname=blah", .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, @@ -915,7 +906,6 @@ mymain(void) testFileData qed = { .expBackingStoreRaw = absraw, .expCapacity = 1024, - .pathAbs = absqed, .path = canonqed, .relDirRel = ".", .relDirAbs = datadir, @@ -923,7 +913,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { - .pathAbs = absqed, .path = canonqed, .relDirRel = ".", .relDirAbs = datadir, @@ -938,7 +927,6 @@ mymain(void) /* directory */ testFileData dir = { - .pathAbs = absdir, .path = canondir, .relDirRel = ".", .relDirAbs = datadir, @@ -977,7 +965,6 @@ mymain(void) .expBackingStoreRaw = "../raw", .expCapacity = 1024, .pathRel = "../sub/link1", - .pathAbs = "../sub/link1", .path = canonqcow2, .relDirRel = "sub/../sub", .relDirAbs = datadir "/sub/../sub", @@ -987,7 +974,6 @@ mymain(void) testFileData link2 = { .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, - .pathAbs = abslink2, .path = canonwrap, .relDirRel = "sub", .relDirAbs = datadir "/sub", @@ -995,7 +981,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; raw.pathRel = "../raw"; - raw.pathAbs = "../raw"; raw.relDirRel = "sub/../sub/.."; raw.relDirAbs = datadir "/sub/../sub/.."; TEST_CHAIN(15, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Separately remove the now unused variable. --- tests/virstoragetest.c | 15 --------------- 1 file changed, 15 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that we store only relative names in virStorageSource's member relPath the backingRelative member is obsolete. Remove it and adapt the code to the removal. --- src/util/virstoragefile.c | 4 +--- src/util/virstoragefile.h | 2 -- tests/virstoragetest.c | 8 +------- 3 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7b4e46a..b527a12 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1598,8 +1598,6 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, if (VIR_ALLOC(ret) < 0) return NULL; - ret->backingRelative = true; - /* store relative name */ if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) goto error; @@ -2168,7 +2166,7 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top, *relpath = NULL; for (next = top; next; next = next->backingStore) { - if (!next->backingRelative || !next->relPath) { + if (!next->relPath) { ret = 1; goto cleanup; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 38d1720..92f30a7 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -256,8 +256,6 @@ struct _virStorageSource { /* Name of the child backing store recorded in metadata of the * current file. */ char *backingStoreRaw; - /* is backing store identified as relative */ - bool backingRelative; }; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 2fd5459..be1ea4d 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -591,13 +591,11 @@ testPathRelativePrepare(void) else backingchain[i].backingStore = NULL; - backingchain[i].backingRelative = true; + backingchain[i].relPath = NULL; } /* normal relative backing chain */ backingchain[0].path = (char *) "/path/to/some/img"; - backingchain[0].relPath = (char *) "/path/to/some/img"; - backingchain[0].backingRelative = false; backingchain[1].path = (char *) "/path/to/some/asdf"; backingchain[1].relPath = (char *) "asdf"; @@ -610,8 +608,6 @@ testPathRelativePrepare(void) /* ovirt's backing chain */ backingchain[4].path = (char *) "/path/to/volume/image1"; - backingchain[4].relPath = (char *) "/path/to/volume/image1"; - backingchain[4].backingRelative = false; backingchain[5].path = (char *) "/path/to/volume/image2"; backingchain[5].relPath = (char *) "../volume/image2"; @@ -624,8 +620,6 @@ testPathRelativePrepare(void) /* some arbitrarily crazy backing chains */ backingchain[8].path = (char *) "/crazy/base/image"; - backingchain[8].relPath = (char *) "/crazy/base/image"; - backingchain[8].backingRelative = false; backingchain[9].path = (char *) "/crazy/base/directory/stuff/volumes/garbage/image2"; backingchain[9].relPath = (char *) "directory/stuff/volumes/garbage/image2"; -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Now that we store only relative names in virStorageSource's member relPath the backingRelative member is obsolete. Remove it and adapt the code to the removal. --- src/util/virstoragefile.c | 4 +--- src/util/virstoragefile.h | 2 -- tests/virstoragetest.c | 8 +------- 3 files changed, 2 insertions(+), 12 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
libvirt always uses an absolute path to address the top image of an image chain. Our storage test tests also the relative path which won't ever be used. Additionally it makes the test more complicated. --- tests/virstoragetest.c | 79 +++++++++++++------------------------------------- 1 file changed, 20 insertions(+), 59 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index be1ea4d..36b82d4 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -707,17 +707,12 @@ mymain(void) #define VIR_FLATTEN_2(...) __VA_ARGS__ #define VIR_FLATTEN_1(_1) VIR_FLATTEN_2 _1 -#define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1, \ - chain2, flags2, chain3, flags3, chain4, flags4) \ +#define TEST_CHAIN(id, path, format, chain1, flags1, chain2, flags2) \ do { \ - TEST_ONE_CHAIN(#id "a", relstart, format, flags1, \ + TEST_ONE_CHAIN(#id "a", path, format, flags1 | ABS_START, \ VIR_FLATTEN_1(chain1)); \ - TEST_ONE_CHAIN(#id "b", relstart, format, flags2, \ + TEST_ONE_CHAIN(#id "b", path, format, flags2 | ABS_START, \ VIR_FLATTEN_1(chain2)); \ - TEST_ONE_CHAIN(#id "c", absstart, format, flags3 | ABS_START,\ - VIR_FLATTEN_1(chain3)); \ - TEST_ONE_CHAIN(#id "d", absstart, format, flags4 | ABS_START,\ - VIR_FLATTEN_1(chain4)); \ } while (0) /* The actual tests, in several groups. */ @@ -733,14 +728,10 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; - TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, - (&raw), EXP_PASS, - (&raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(1, absraw, VIR_STORAGE_FILE_RAW, (&raw), EXP_PASS, (&raw), ALLOW_PROBE | EXP_PASS); - TEST_CHAIN(2, "raw", absraw, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, - (&raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(2, absraw, VIR_STORAGE_FILE_AUTO, (&raw), EXP_PASS, (&raw), ALLOW_PROBE | EXP_PASS); @@ -762,14 +753,10 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; - TEST_CHAIN(3, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2, &raw), EXP_PASS, - (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(3, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); - TEST_CHAIN(4, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - (&qcow2_as_raw), EXP_PASS, - (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(4, absqcow2, VIR_STORAGE_FILE_AUTO, (&qcow2_as_raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -784,14 +771,10 @@ mymain(void) raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format provided */ - TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2, &raw), EXP_PASS, - (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(5, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); - TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - (&qcow2_as_raw), EXP_PASS, - (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(6, absqcow2, VIR_STORAGE_FILE_AUTO, (&qcow2_as_raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -806,9 +789,7 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; qcow2.relDirRel = datadir; - TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap, &qcow2, &raw), EXP_PASS, - (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(7, abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2, &raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -836,9 +817,7 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; - TEST_CHAIN(8, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap_as_raw, &qcow2_as_raw), EXP_PASS, - (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(8, abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap_as_raw, &qcow2_as_raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -853,9 +832,7 @@ mymain(void) qcow2.relDirRel = "."; /* Qcow2 file with missing backing file but specified type */ - TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2), EXP_WARN, - (&qcow2), ALLOW_PROBE | EXP_WARN, + TEST_CHAIN(9, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN, (&qcow2), ALLOW_PROBE | EXP_WARN); @@ -867,9 +844,7 @@ mymain(void) ret = -1; /* Qcow2 file with missing backing file and no specified type */ - TEST_CHAIN(10, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2), EXP_WARN, - (&qcow2), ALLOW_PROBE | EXP_WARN, + TEST_CHAIN(10, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN, (&qcow2), ALLOW_PROBE | EXP_WARN); @@ -890,9 +865,7 @@ mymain(void) .relDirRel = ".", .relDirAbs = ".", }; - TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2, &nbd), EXP_PASS, - (&qcow2, &nbd), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(11, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &nbd), EXP_PASS, (&qcow2, &nbd), ALLOW_PROBE | EXP_PASS); @@ -913,9 +886,7 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; - TEST_CHAIN(12, "qed", absqed, VIR_STORAGE_FILE_AUTO, - (&qed_as_raw), EXP_PASS, - (&qed, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(12, absqed, VIR_STORAGE_FILE_AUTO, (&qed_as_raw), EXP_PASS, (&qed, &raw), ALLOW_PROBE | EXP_PASS); @@ -927,14 +898,10 @@ mymain(void) .type = VIR_STORAGE_TYPE_DIR, .format = VIR_STORAGE_FILE_DIR, }; - TEST_CHAIN(13, "dir", absdir, VIR_STORAGE_FILE_AUTO, - (&dir), EXP_PASS, - (&dir), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(13, absdir, VIR_STORAGE_FILE_AUTO, (&dir), EXP_PASS, (&dir), ALLOW_PROBE | EXP_PASS); - TEST_CHAIN(14, "dir", absdir, VIR_STORAGE_FILE_DIR, - (&dir), EXP_PASS, - (&dir), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(14, absdir, VIR_STORAGE_FILE_DIR, (&dir), EXP_PASS, (&dir), ALLOW_PROBE | EXP_PASS); @@ -977,9 +944,7 @@ mymain(void) raw.pathRel = "../raw"; raw.relDirRel = "sub/../sub/.."; raw.relDirAbs = datadir "/sub/../sub/.."; - TEST_CHAIN(15, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, - (&link2, &link1, &raw), EXP_PASS, - (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(15, abslink2, VIR_STORAGE_FILE_QCOW2, (&link2, &link1, &raw), EXP_PASS, (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS); #endif @@ -993,9 +958,7 @@ mymain(void) qcow2.expBackingStoreRaw = "qcow2"; /* Behavior of an infinite loop chain */ - TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2), EXP_WARN, - (&qcow2), ALLOW_PROBE | EXP_WARN, + TEST_CHAIN(16, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN, (&qcow2), ALLOW_PROBE | EXP_WARN); @@ -1015,9 +978,7 @@ mymain(void) qcow2.relDirRel = datadir; /* Behavior of an infinite loop chain */ - TEST_CHAIN(17, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap, &qcow2), EXP_WARN, - (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN, + TEST_CHAIN(17, abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2), EXP_WARN, (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN); -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
libvirt always uses an absolute path to address the top image of an image chain. Our storage test tests also the relative path which won't ever be used. Additionally it makes the test more complicated.
It covers some interesting corner cases that matter to qemu; but I agree that we won't ever use them locally, and that pruning our code may make it easier to maintain (whereas the user can start qemu-img in any directory of their choice, we are always running libvirtd in / and so relative names don't work, and we really do always start with an absolute name). I think having the relative tests helped us avoid what might otherwise have been some nasty regressions in all our refactoring, but now that we have stabilized on something that is working, I agree with the decision to prune it.
--- tests/virstoragetest.c | 79 +++++++++++++------------------------------------- 1 file changed, 20 insertions(+), 59 deletions(-)
-#define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1, \ - chain2, flags2, chain3, flags3, chain4, flags4) \ +#define TEST_CHAIN(id, path, format, chain1, flags1, chain2, flags2) \ do { \ - TEST_ONE_CHAIN(#id "a", relstart, format, flags1, \ + TEST_ONE_CHAIN(#id "a", path, format, flags1 | ABS_START, \ VIR_FLATTEN_1(chain1)); \ - TEST_ONE_CHAIN(#id "b", relstart, format, flags2, \ + TEST_ONE_CHAIN(#id "b", path, format, flags2 | ABS_START, \ VIR_FLATTEN_1(chain2)); \
Do we even still need ABS_START? But if not, do that as a separate cleanup; this patch is already big enough. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
After we don't test relative paths, remove even more unnecessary cruft from the test code. --- tests/virstoragetest.c | 61 +++++++++++++++----------------------------------- 1 file changed, 18 insertions(+), 43 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 36b82d4..1554a8f 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -284,8 +284,7 @@ struct _testFileData bool expEncrypted; const char *pathRel; const char *path; - const char *relDirRel; - const char *relDirAbs; + const char *relDir; int type; int format; }; @@ -295,7 +294,6 @@ enum { EXP_FAIL = 1, EXP_WARN = 2, ALLOW_PROBE = 4, - ABS_START = 8, }; struct testChainData @@ -328,7 +326,6 @@ testStorageChain(const void *args) virStorageSourcePtr elt; size_t i = 0; char *broken = NULL; - bool isAbs = !!(data->flags & ABS_START); meta = testStorageFileGetMetadata(data->start, data->format, -1, -1, (data->flags & ALLOW_PROBE) != 0); @@ -367,15 +364,12 @@ testStorageChain(const void *args) while (elt) { char *expect = NULL; char *actual = NULL; - const char *expRelDir; if (i == data->nfiles) { fprintf(stderr, "probed chain was too long\n"); goto cleanup; } - expRelDir = isAbs ? data->files[i]->relDirAbs - : data->files[i]->relDirRel; if (virAsprintf(&expect, testStorageChainFormat, i, NULLSTR(data->files[i]->path), @@ -383,7 +377,7 @@ testStorageChain(const void *args) data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(data->files[i]->pathRel), - NULLSTR(expRelDir), + NULLSTR(data->files[i]->relDir), data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, @@ -707,12 +701,10 @@ mymain(void) #define VIR_FLATTEN_2(...) __VA_ARGS__ #define VIR_FLATTEN_1(_1) VIR_FLATTEN_2 _1 -#define TEST_CHAIN(id, path, format, chain1, flags1, chain2, flags2) \ - do { \ - TEST_ONE_CHAIN(#id "a", path, format, flags1 | ABS_START, \ - VIR_FLATTEN_1(chain1)); \ - TEST_ONE_CHAIN(#id "b", path, format, flags2 | ABS_START, \ - VIR_FLATTEN_1(chain2)); \ +#define TEST_CHAIN(id, path, format, chain1, flags1, chain2, flags2) \ + do { \ + TEST_ONE_CHAIN(#id "a", path, format, flags1, VIR_FLATTEN_1(chain1)); \ + TEST_ONE_CHAIN(#id "b", path, format, flags2, VIR_FLATTEN_1(chain2)); \ } while (0) /* The actual tests, in several groups. */ @@ -723,8 +715,7 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { .path = canonraw, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -741,15 +732,13 @@ mymain(void) .expBackingStoreRaw = "raw", .expCapacity = 1024, .path = canonqcow2, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { .path = canonqcow2, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -768,7 +757,6 @@ mymain(void) ret = -1; qcow2.expBackingStoreRaw = absraw; raw.pathRel = NULL; - raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format provided */ TEST_CHAIN(5, absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -783,12 +771,10 @@ mymain(void) .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .path = canonwrap, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; - qcow2.relDirRel = datadir; TEST_CHAIN(7, abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2, &raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -805,15 +791,13 @@ mymain(void) "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_as_raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .path = canonwrap, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -829,7 +813,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = datadir "/bogus"; - qcow2.relDirRel = "."; /* Qcow2 file with missing backing file but specified type */ TEST_CHAIN(9, absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -862,8 +845,7 @@ mymain(void) .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, - .relDirRel = ".", - .relDirAbs = ".", + .relDir = ".", }; TEST_CHAIN(11, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &nbd), EXP_PASS, @@ -874,15 +856,13 @@ mymain(void) .expBackingStoreRaw = absraw, .expCapacity = 1024, .path = canonqed, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { .path = canonqed, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -893,8 +873,7 @@ mymain(void) /* directory */ testFileData dir = { .path = canondir, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_DIR, .format = VIR_STORAGE_FILE_DIR, }; @@ -927,8 +906,7 @@ mymain(void) .expCapacity = 1024, .pathRel = "../sub/link1", .path = canonqcow2, - .relDirRel = "sub/../sub", - .relDirAbs = datadir "/sub/../sub", + .relDir = datadir "/sub/../sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -936,14 +914,12 @@ mymain(void) .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, .path = canonwrap, - .relDirRel = "sub", - .relDirAbs = datadir "/sub", + .relDir = datadir "/sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; raw.pathRel = "../raw"; - raw.relDirRel = "sub/../sub/.."; - raw.relDirAbs = datadir "/sub/../sub/.."; + raw.relDir = datadir "/sub/../sub/.."; TEST_CHAIN(15, abslink2, VIR_STORAGE_FILE_QCOW2, (&link2, &link1, &raw), EXP_PASS, (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS); @@ -975,7 +951,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = "wrap"; - qcow2.relDirRel = datadir; /* Behavior of an infinite loop chain */ TEST_CHAIN(17, abswrap, VIR_STORAGE_FILE_QCOW2, -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
After we don't test relative paths, remove even more unnecessary cruft from the test code. --- tests/virstoragetest.c | 61 +++++++++++++++----------------------------------- 1 file changed, 18 insertions(+), 43 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Store backing chain paths as non-canonical. The canonicalization step will be already taken. This will allow to avoid storing unnecessary amounts of data. --- src/util/virstoragefile.c | 33 ++++++--------------------------- tests/virstoragetest.c | 10 +++++----- 2 files changed, 11 insertions(+), 32 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b527a12..7d52175 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1044,25 +1044,17 @@ virStorageFileGetMetadataFromFD(const char *path, int *backingFormat) { - virStorageSourcePtr ret = NULL; - char *canonPath = NULL; - - if (!(canonPath = canonicalize_file_name(path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); - goto cleanup; - } + virStorageSourcePtr ret; - if (!(ret = virStorageFileMetadataNew(canonPath, format))) - goto cleanup; + if (!(ret = virStorageFileMetadataNew(path, format))) + return NULL; if (virStorageFileGetMetadataFromFDInternal(ret, fd, backingFormat) < 0) { virStorageSourceFree(ret); - ret = NULL; + return NULL; } - cleanup: - VIR_FREE(canonPath); return ret; } @@ -1618,15 +1610,6 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, virReportOOMError(); goto error; } - - /* XXX we don't currently need to store the canonical path but the - * change would break the test suite. Get rid of this later */ - char *tmp = ret->path; - if (!(ret->path = canonicalize_file_name(tmp))) { - ret->path = tmp; - tmp = NULL; - } - VIR_FREE(tmp); } else { ret->type = VIR_STORAGE_TYPE_NETWORK; @@ -1865,12 +1848,8 @@ virStorageSourceNewFromBackingAbsolute(const char *path) goto error; } - /* XXX we don't currently need to store the canonical path but the - * change would break the test suite. Get rid of this later */ - if (!(ret->path = canonicalize_file_name(path))) { - if (VIR_STRDUP(ret->path, path) < 0) - goto error; - } + if (VIR_STRDUP(ret->path, path) < 0) + goto error; } else { ret->type = VIR_STORAGE_TYPE_NETWORK; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 1554a8f..5bc3122 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -121,10 +121,8 @@ testStorageFileGetMetadata(const char *path, goto error; } - if (!(ret->path = canonicalize_file_name(path))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "failed to resolve '%s'", path); + if (VIR_STRDUP(ret->path, path) < 0) goto error; - } if (virStorageFileGetMetadata(ret, uid, gid, allow_probe) < 0) goto error; @@ -905,7 +903,7 @@ mymain(void) .expBackingStoreRaw = "../raw", .expCapacity = 1024, .pathRel = "../sub/link1", - .path = canonqcow2, + .path = datadir "/sub/../sub/link1", .relDir = datadir "/sub/../sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, @@ -913,11 +911,13 @@ mymain(void) testFileData link2 = { .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, - .path = canonwrap, + .path = abslink2, .relDir = datadir "/sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; + + raw.path = datadir "/sub/../sub/../raw"; raw.pathRel = "../raw"; raw.relDir = datadir "/sub/../sub/.."; TEST_CHAIN(15, abslink2, VIR_STORAGE_FILE_QCOW2, -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Store backing chain paths as non-canonical. The canonicalization step will be already taken. This will allow to avoid storing unnecessary amounts of data. --- src/util/virstoragefile.c | 33 ++++++--------------------------- tests/virstoragetest.c | 10 +++++----- 2 files changed, 11 insertions(+), 32 deletions(-)
- - /* XXX we don't currently need to store the canonical path but the - * change would break the test suite. Get rid of this later */
Yay, doing cleanups hinted at earlier in the series. ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The parent directory doesn't necessarily need to be stored after we don't mangle the path stored in the image. Remove it and tweak the code to avoid using it. --- src/storage/storage_driver.c | 11 ++----- src/util/virstoragefile.c | 68 ++++++++++++++++++-------------------------- src/util/virstoragefile.h | 3 -- tests/virstoragetest.c | 21 -------------- 4 files changed, 30 insertions(+), 73 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c6e2936..44855fa 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2837,8 +2837,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, virStorageSourcePtr backingStore = NULL; int backingFormat; - VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, NULLSTR(src->relDir), src->format, + VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", + src->path, src->format, (int)uid, (int)gid, allow_probe); /* exit if we can't load information about the current image */ @@ -2944,19 +2944,12 @@ virStorageFileGetMetadata(virStorageSourcePtr src, if (!(cycle = virHashCreate(5, NULL))) return -1; - if (!src->relDir && - !(src->relDir = mdir_name(src->path))) { - virReportOOMError(); - goto cleanup; - } - if (src->format <= VIR_STORAGE_FILE_NONE) src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; ret = virStorageFileGetMetadataRecurse(src, uid, gid, allow_probe, cycle); - cleanup: VIR_FREE(canonPath); virHashFree(cycle); return ret; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7d52175..831e3bf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1339,9 +1339,10 @@ virStorageFileChainLookup(virStorageSourcePtr chain, unsigned int idx, const char **parent) { + virStorageSourcePtr prev = NULL; const char *start = chain->path; const char *tmp; - const char *parentDir = "."; + char *parentDir = NULL; bool nameIsFile = virStorageIsFile(name); size_t i; @@ -1372,8 +1373,20 @@ virStorageFileChainLookup(virStorageSourcePtr chain, break; if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || chain->type == VIR_STORAGE_TYPE_BLOCK)) { + if (prev) { + if (!(parentDir = mdir_name(prev->path))) { + virReportOOMError(); + goto error; + } + } else { + if (VIR_STRDUP(parentDir, ".") < 0) + goto error; + } + int result = virFileRelLinkPointsTo(parentDir, name, chain->path); + + VIR_FREE(parentDir); if (result < 0) goto error; if (result > 0) @@ -1381,7 +1394,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, } } *parent = chain->path; - parentDir = chain->relDir; + prev = chain; chain = chain->backingStore; i++; } @@ -1525,7 +1538,6 @@ virStorageSourceClearBackingStore(virStorageSourcePtr def) return; VIR_FREE(def->relPath); - VIR_FREE(def->relDir); VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ @@ -1584,7 +1596,6 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, const char *rel) { char *dirname = NULL; - const char *parentdir = ""; virStorageSourcePtr ret; if (VIR_ALLOC(ret) < 0) @@ -1594,23 +1605,20 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) goto error; - /* XXX Once we get rid of the need to use canonical names in path, we will be - * able to use mdir_name on parent->path instead of using parent->relDir */ - if (STRNEQ(parent->relDir, "/")) - parentdir = parent->relDir; - - if (virAsprintf(&ret->path, "%s/%s", parentdir, rel) < 0) + if (!(dirname = mdir_name(parent->path))) { + virReportOOMError(); goto error; + } - if (virStorageSourceGetActualType(parent) != VIR_STORAGE_TYPE_NETWORK) { - ret->type = VIR_STORAGE_TYPE_FILE; - - /* XXX store the relative directory name for test's sake */ - if (!(ret->relDir = mdir_name(ret->path))) { - virReportOOMError(); + if (STRNEQ(dirname, "/")) { + if (virAsprintf(&ret->path, "%s/%s", dirname, rel) < 0) goto error; - } } else { + if (virAsprintf(&ret->path, "/%s", rel) < 0) + goto error; + } + + if (virStorageSourceGetActualType(parent) == VIR_STORAGE_TYPE_NETWORK) { ret->type = VIR_STORAGE_TYPE_NETWORK; /* copy the host network part */ @@ -1621,12 +1629,9 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, if (VIR_STRDUP(ret->volume, parent->volume) < 0) goto error; - - /* XXX store the relative directory name for test's sake */ - if (!(ret->relDir = mdir_name(ret->path))) { - virReportOOMError(); - goto error; - } + } else { + /* set the type to _FILE, the caller shall update it to the actual type */ + ret->type = VIR_STORAGE_TYPE_FILE; } cleanup: @@ -1842,12 +1847,6 @@ virStorageSourceNewFromBackingAbsolute(const char *path) if (virStorageIsFile(path)) { ret->type = VIR_STORAGE_TYPE_FILE; - /* XXX store the relative directory name for test's sake */ - if (!(ret->relDir = mdir_name(path))) { - virReportOOMError(); - goto error; - } - if (VIR_STRDUP(ret->path, path) < 0) goto error; } else { @@ -1861,17 +1860,6 @@ virStorageSourceNewFromBackingAbsolute(const char *path) if (virStorageSourceParseBackingColon(ret, path) < 0) goto error; } - - /* XXX fill relative path so that relative names work with network storage too */ - if (ret->path) { - if (!(ret->relDir = mdir_name(ret->path))) { - virReportOOMError(); - goto error; - } - } else { - if (VIR_STRDUP(ret->relDir, "") < 0) - goto error; - } } return ret; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 92f30a7..cb16720 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -250,9 +250,6 @@ struct _virStorageSource { /* Relative path of the backing image from the parent NULL if * backed by absolute path */ char *relPath; - /* Directory to start from if backingStoreRaw is a relative file - * name. */ - char *relDir; /* Name of the child backing store recorded in metadata of the * current file. */ char *backingStoreRaw; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 5bc3122..bae538f 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -116,11 +116,6 @@ testStorageFileGetMetadata(const char *path, } } - if (!(ret->relDir = mdir_name(path))) { - virReportOOMError(); - goto error; - } - if (VIR_STRDUP(ret->path, path) < 0) goto error; @@ -282,7 +277,6 @@ struct _testFileData bool expEncrypted; const char *pathRel; const char *path; - const char *relDir; int type; int format; }; @@ -311,7 +305,6 @@ static const char testStorageChainFormat[] = "capacity: %lld\n" "encryption: %d\n" "relPath:%s\n" - "relDir:%s\n" "type:%d\n" "format:%d\n"; @@ -375,7 +368,6 @@ testStorageChain(const void *args) data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(data->files[i]->pathRel), - NULLSTR(data->files[i]->relDir), data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, @@ -385,7 +377,6 @@ testStorageChain(const void *args) elt->capacity, !!elt->encryption, NULLSTR(elt->relPath), - NULLSTR(elt->relDir), elt->type, elt->format) < 0) { VIR_FREE(expect); @@ -713,7 +704,6 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { .path = canonraw, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -730,13 +720,11 @@ mymain(void) .expBackingStoreRaw = "raw", .expCapacity = 1024, .path = canonqcow2, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { .path = canonqcow2, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -769,7 +757,6 @@ mymain(void) .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .path = canonwrap, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -795,7 +782,6 @@ mymain(void) .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .path = canonwrap, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -843,7 +829,6 @@ mymain(void) .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, - .relDir = ".", }; TEST_CHAIN(11, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &nbd), EXP_PASS, @@ -854,13 +839,11 @@ mymain(void) .expBackingStoreRaw = absraw, .expCapacity = 1024, .path = canonqed, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { .path = canonqed, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -871,7 +854,6 @@ mymain(void) /* directory */ testFileData dir = { .path = canondir, - .relDir = datadir, .type = VIR_STORAGE_TYPE_DIR, .format = VIR_STORAGE_FILE_DIR, }; @@ -904,7 +886,6 @@ mymain(void) .expCapacity = 1024, .pathRel = "../sub/link1", .path = datadir "/sub/../sub/link1", - .relDir = datadir "/sub/../sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -912,14 +893,12 @@ mymain(void) .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, .path = abslink2, - .relDir = datadir "/sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; raw.path = datadir "/sub/../sub/../raw"; raw.pathRel = "../raw"; - raw.relDir = datadir "/sub/../sub/.."; TEST_CHAIN(15, abslink2, VIR_STORAGE_FILE_QCOW2, (&link2, &link1, &raw), EXP_PASS, (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS); -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
The parent directory doesn't necessarily need to be stored after we don't mangle the path stored in the image. Remove it and tweak the code to avoid using it. --- src/storage/storage_driver.c | 11 ++----- src/util/virstoragefile.c | 68 ++++++++++++++++++-------------------------- src/util/virstoragefile.h | 3 -- tests/virstoragetest.c | 21 -------------- 4 files changed, 30 insertions(+), 73 deletions(-)
@@ -1372,8 +1373,20 @@ virStorageFileChainLookup(virStorageSourcePtr chain, break; if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || chain->type == VIR_STORAGE_TYPE_BLOCK)) { + if (prev) { + if (!(parentDir = mdir_name(prev->path))) { + virReportOOMError(); + goto error; + } + } else { + if (VIR_STRDUP(parentDir, ".") < 0) + goto error; + } + int result = virFileRelLinkPointsTo(parentDir, name, chain->path);
Maybe you could make virFileRelLinkPointsTo gracefully handle parentDir==NULL as meaning to start from the current directory, and then you wouldn't have to strdup(".") here. But if you do that, make it a separate patch. I'm fine with this one as-is. ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/25/14 00:59, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
The parent directory doesn't necessarily need to be stored after we don't mangle the path stored in the image. Remove it and tweak the code to avoid using it. --- src/storage/storage_driver.c | 11 ++----- src/util/virstoragefile.c | 68 ++++++++++++++++++-------------------------- src/util/virstoragefile.h | 3 -- tests/virstoragetest.c | 21 -------------- 4 files changed, 30 insertions(+), 73 deletions(-)
@@ -1372,8 +1373,20 @@ virStorageFileChainLookup(virStorageSourcePtr chain, break; if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || chain->type == VIR_STORAGE_TYPE_BLOCK)) { + if (prev) { + if (!(parentDir = mdir_name(prev->path))) { + virReportOOMError(); + goto error; + } + } else { + if (VIR_STRDUP(parentDir, ".") < 0) + goto error; + } + int result = virFileRelLinkPointsTo(parentDir, name, chain->path);
Maybe you could make virFileRelLinkPointsTo gracefully handle parentDir==NULL as meaning to start from the current directory, and then you wouldn't have to strdup(".") here. But if you do that, make it a separate patch. I'm fine with this one as-is.
ACK
I've tweaked the commits according to your comments and pushed everything up to here. Thank you for the review. Peter
This command allows to change the backing file name recorded in the metadata of a qcow (or other) image. The capability also notifies that the "block-stream" and "block-commit" commands understand the "backing-file" attribute. --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4cbe8d1..df3f529 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -257,6 +257,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "host-pci-multidomain", "msg-timestamp", "active-commit", + "change-backing-file", ); @@ -1410,6 +1411,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "blockdev-snapshot-sync", QEMU_CAPS_DISK_SNAPSHOT }, { "add-fd", QEMU_CAPS_ADD_FD }, { "nbd-server-start", QEMU_CAPS_NBD_SERVER }, + { "change-backing-file", QEMU_CAPS_CHANGE_BACKING_FILE }, }; struct virQEMUCapsStringFlags virQEMUCapsEvents[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 617986d..ea2d9e3 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -207,6 +207,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_HOST_PCI_MULTIDOMAIN = 166, /* support domain > 0 in host pci address */ QEMU_CAPS_MSG_TIMESTAMP = 167, /* -msg timestamp */ QEMU_CAPS_ACTIVE_COMMIT = 168, /* block-commit works without 'top' */ + QEMU_CAPS_CHANGE_BACKING_FILE = 169, /* change name of backing file in metadata */ QEMU_CAPS_LAST, /* this must always be the last item */ }; -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
This command allows to change the backing file name recorded in the metadata of a qcow (or other) image. The capability also notifies that the "block-stream" and "block-commit" commands understand the "backing-file" attribute. --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 insertions(+)
This, of course, is waiting on the same series from Jeff that my active-commit is waiting on; but I agree with making it a separate capability, as someone may backport active-commit optional 'top' to qemu 2.0 while leaving out the more complex backing file stuff. ACK, but wait to push until qemu.git is ready :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow changing the name that is recorded in the overlay of the TOP image used in a block commit operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-commit command. --- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_driver.c | 1 + src/qemu/qemu_monitor.c | 8 +++++--- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 6 +++--- 7 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index df3f529..2b7cfad 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2182,7 +2182,7 @@ virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps, /* Probe for active commit of qemu 2.1 (for now, we are choosing * to ignore the fact that qemu 2.0 can also do active commit) */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCK_COMMIT) && - qemuMonitorBlockCommit(mon, "bogus", NULL, NULL, 0) == -2) + qemuMonitorBlockCommit(mon, "bogus", NULL, NULL, NULL, 0) == -2) virQEMUCapsSet(qemuCaps, QEMU_CAPS_ACTIVE_COMMIT); return 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38c5ecb..41604a1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15639,6 +15639,7 @@ qemuDomainBlockCommit(virDomainPtr dom, ret = qemuMonitorBlockCommit(priv->mon, device, top && !topIndex ? top : topSource->path, base && !baseIndex ? base : baseSource->path, + NULL, bandwidth); qemuDomainObjExitMonitor(driver, vm); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 013a6ad..66fa39a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3234,13 +3234,14 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, + const char *backingName, unsigned long bandwidth) { int ret = -1; unsigned long long speed; - VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, bandwidth=%ld", - mon, device, NULLSTR(top), NULLSTR(base), bandwidth); + VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, backingName=%s, bandwidth=%lu", + mon, device, NULLSTR(top), NULLSTR(base), NULLSTR(backingName), bandwidth); /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is * limited to LLONG_MAX also for unsigned values */ @@ -3254,7 +3255,8 @@ qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, speed <<= 20; if (mon->json) - ret = qemuMonitorJSONBlockCommit(mon, device, top, base, speed); + ret = qemuMonitorJSONBlockCommit(mon, device, top, base, + backingName, speed); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("block-commit requires JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1c7857e..96af5fd 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -662,6 +662,7 @@ int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, + const char *backingName, unsigned long bandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 0e4262d..3dd09b5 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3458,6 +3458,7 @@ qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, + const char *backingName, unsigned long long speed) { int ret = -1; @@ -3469,6 +3470,7 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, "U:speed", speed, "S:top", top, "S:base", base, + "S:backing-file", backingName, NULL); if (!cmd) return -1; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 89e668c..652a4b6 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -261,6 +261,7 @@ int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, + const char *backingName, unsigned long long bandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index faa968f..ac0b92f 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1164,7 +1164,7 @@ GEN_TEST_FUNC(qemuMonitorJSONAddDevice, "some_dummy_devicestr") GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase") GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) -GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", 1024) +GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024) GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb", NULL, NULL) GEN_TEST_FUNC(qemuMonitorJSONScreendump, "/foo/bar") GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) @@ -2021,7 +2021,7 @@ testQemuMonitorJSONqemuMonitorJSONBlockCommit2(const void *data) goto cleanup; if (qemuMonitorBlockCommit(qemuMonitorTestGetMonitor(test), - "bogus", NULL, NULL, 0) != -2) + "bogus", NULL, NULL, NULL, 0) != -2) goto cleanup; if (qemuMonitorTestAddItemParams(test, "block-commit", error2, @@ -2030,7 +2030,7 @@ testQemuMonitorJSONqemuMonitorJSONBlockCommit2(const void *data) goto cleanup; if (qemuMonitorBlockCommit(qemuMonitorTestGetMonitor(test), - "bogus", NULL, NULL, 0) != -3) + "bogus", NULL, NULL, NULL, 0) != -3) goto cleanup; ret = 0; -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
To allow changing the name that is recorded in the overlay of the TOP image used in a block commit operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-commit command.
Yeah, I can see how this one conflicts with my work on active commit. This one is also gated by waiting for qemu.git to have Jeff's work added.
--- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_driver.c | 1 + src/qemu/qemu_monitor.c | 8 +++++--- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 6 +++--- 7 files changed, 14 insertions(+), 7 deletions(-)
This one just adds the parameter placeholder, but never supplies it, so it is safe. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow changing the name that is recorded in the top of the current image chain used in a block pull/rebase operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-stream commad. --- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 6 +++--- src/qemu/qemu_monitor.c | 12 +++++++----- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 15 +++++++++++++++ src/qemu/qemu_monitor_json.h | 1 + 6 files changed, 32 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 41604a1..0f35123 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14885,7 +14885,7 @@ qemuDomainBlockPivot(virConnectPtr conn, /* Probe the status, if needed. */ if (!disk->mirroring) { qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &info, + rc = qemuMonitorBlockJob(priv->mon, device, NULL, NULL, 0, &info, BLOCK_JOB_INFO, true); qemuDomainObjExitMonitor(driver, vm); if (rc < 0) @@ -15103,7 +15103,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJob(priv->mon, device, baseIndex ? baseSource->path : base, - bandwidth, info, mode, async); + NULL, bandwidth, info, mode, async); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) goto endjob; @@ -15149,8 +15149,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virDomainBlockJobInfo dummy; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &dummy, - BLOCK_JOB_INFO, async); + ret = qemuMonitorBlockJob(priv->mon, device, NULL, NULL, 0, + &dummy, BLOCK_JOB_INFO, async); qemuDomainObjExitMonitor(driver, vm); if (ret <= 0) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 842f782..30175f2 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1308,7 +1308,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, _("canceled by client")); goto error; } - mon_ret = qemuMonitorBlockJob(priv->mon, diskAlias, NULL, 0, + mon_ret = qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, &info, BLOCK_JOB_INFO, true); qemuDomainObjExitMonitor(driver, vm); @@ -1360,7 +1360,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, continue; if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) { - if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, 0, + if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, NULL, BLOCK_JOB_ABORT, true) < 0) { VIR_WARN("Unable to cancel block-job on '%s'", diskAlias); } @@ -1426,7 +1426,7 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cleanup; - if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, 0, + if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, NULL, BLOCK_JOB_ABORT, true) < 0) VIR_WARN("Unable to stop block job on %s", diskAlias); qemuDomainObjExitMonitor(driver, vm); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 66fa39a..b84df00 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3353,6 +3353,7 @@ int qemuMonitorScreendump(qemuMonitorPtr mon, int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *device, const char *base, + const char *backingName, unsigned long bandwidth, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, @@ -3361,9 +3362,10 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, int ret = -1; unsigned long long speed; - VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%luM, info=%p, mode=%o, " - "modern=%d", mon, device, NULLSTR(base), bandwidth, info, mode, - modern); + VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%luM, " + "info=%p, mode=%o, modern=%d", + mon, device, NULLSTR(base), NULLSTR(backingName), + bandwidth, info, mode, modern); /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is * limited to LLONG_MAX also for unsigned values */ @@ -3377,8 +3379,8 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, speed <<= 20; if (mon->json) - ret = qemuMonitorJSONBlockJob(mon, device, base, speed, info, mode, - modern); + ret = qemuMonitorJSONBlockJob(mon, device, base, backingName, + speed, info, mode, modern); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("block jobs require JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 96af5fd..01555b8 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -690,7 +690,8 @@ typedef enum { int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *device, - const char *back, + const char *base, + const char *backingName, unsigned long bandwidth, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3dd09b5..2af3056 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3782,6 +3782,7 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *device, const char *base, + const char *backingName, unsigned long long speed, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, @@ -3797,6 +3798,19 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, _("only modern block pull supports base: %s"), base); return -1; } + + if (backingName && mode != BLOCK_JOB_PULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("backing name is supported only for block pull")); + return -1; + } + + if (backingName && !base) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("backing name requires a base image")); + return -1; + } + if (speed && mode == BLOCK_JOB_PULL && !modern) { virReportError(VIR_ERR_INTERNAL_ERROR, _("only modern block pull supports speed: %llu"), @@ -3831,6 +3845,7 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, "s:device", device, "P:speed", speed, "S:base", base, + "S:backing-file", backingName, NULL); break; } diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 652a4b6..385211c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -283,6 +283,7 @@ int qemuMonitorJSONScreendump(qemuMonitorPtr mon, int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *device, const char *base, + const char *backingName, unsigned long long speed, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
To allow changing the name that is recorded in the top of the current image chain used in a block pull/rebase operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-stream commad.
s/commad/command/ Gated by acceptance of Jeff's patches into qemu.git.
--- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 6 +++--- src/qemu/qemu_monitor.c | 12 +++++++----- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 15 +++++++++++++++ src/qemu/qemu_monitor_json.h | 1 + 6 files changed, 32 insertions(+), 13 deletions(-)
ACK. This patch just wires up the parameter, but doesn't use it, so it's safe. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Introduce flag for the block commit API to allow the commit operation to leave the chain relatively addressed. Also adds a virsh switch to enable this behavior. --- include/libvirt/libvirt.h.in | 4 ++++ src/libvirt.c | 5 +++++ tools/virsh-domain.c | 7 +++++++ tools/virsh.pod | 5 +++-- 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c83b20d..d165fea 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2599,6 +2599,10 @@ typedef enum { have been committed */ VIR_DOMAIN_BLOCK_COMMIT_ACTIVE = 1 << 2, /* Allow a two-phase commit when top is the active layer */ + VIR_DOMAIN_BLOCK_COMMIT_RELATIVE = 1 << 3, /* try to keep the backing chain + relative if the components + removed by the commit are + already relative */ } virDomainBlockCommitFlags; int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, diff --git a/src/libvirt.c b/src/libvirt.c index 83b7437..eef014e 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19879,6 +19879,11 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, * VIR_DOMAIN_BLOCK_COMMIT_DELETE, then this command will unlink all files * that were invalidated, after the commit successfully completes. * + * If @flags contains VIR_DOMAIN_BLOCK_COMMIT_RELATIVE, the name recorded + * into the overlay of the @top image as path to the new backing file + * will be kept relative to other images in case the backing chain was + * using relative names. + * * By default, if @base is NULL, the commit target will be the bottom of * the backing chain; if @flags contains VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, * then the immediate backing file of @top will be used instead. If @top diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0ae1538..1d99cce 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1496,6 +1496,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, vshCommandOptBool(cmd, "pivot") || vshCommandOptBool(cmd, "keep-overlay")) flags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; + if (vshCommandOptBool(cmd, "keep-relative")) + flags |= VIR_DOMAIN_BLOCK_COMMIT_RELATIVE; ret = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); break; case VSH_CMD_BLOCK_JOB_COPY: @@ -1629,6 +1631,11 @@ static const vshCmdOptDef opts_block_commit[] = { .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish") }, + {.name = "keep-relative", + .type = VSH_OT_BOOL, + .help = N_("keep the backing chain relative if it was relatively " + "referenced if it was before") + }, {.name = NULL} }; diff --git a/tools/virsh.pod b/tools/virsh.pod index 2c0f18a..40163b2 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -782,7 +782,7 @@ address of virtual interface (such as I<detach-interface> or I<domif-setlink>) will accept the MAC address printed by this command. =item B<blockcommit> I<domain> I<path> [I<bandwidth>] -[I<base>] [I<--shallow>] [I<top>] [I<--delete>] +[I<base>] [I<--shallow>] [I<top>] [I<--delete>] [I<--keep-relative>] [I<--wait> [I<--async>] [I<--verbose>]] [I<--timeout> B<seconds>] [I<--active>] [{I<--pivot> | I<--keep-overlay>}] @@ -795,7 +795,8 @@ I<--shallow> can be used instead of I<base> to specify the immediate backing file of the resulting top image to be committed. The files being committed are rendered invalid, possibly as soon as the operation starts; using the I<--delete> flag will attempt to remove these invalidated -files at the successful completion of the commit operation. +files at the successful completion of the commit operation. When the +I<--keep-relative> flag is used, the backing file paths will be kept relative. When I<top> is omitted or specified as the active image, it is also possible to specify I<--active> to trigger a two-phase active commit. In -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Introduce flag for the block commit API to allow the commit operation to leave the chain relatively addressed. Also adds a virsh switch to enable this behavior. --- include/libvirt/libvirt.h.in | 4 ++++ src/libvirt.c | 5 +++++ tools/virsh-domain.c | 7 +++++++ tools/virsh.pod | 5 +++-- 4 files changed, 19 insertions(+), 2 deletions(-)
This patch can go in now, in order to make the freeze for 1.2.6. That is, we can commit to this API even if we can't implement it in qemu until Jeff's patches are in.
+++ b/src/libvirt.c @@ -19879,6 +19879,11 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, * VIR_DOMAIN_BLOCK_COMMIT_DELETE, then this command will unlink all files * that were invalidated, after the commit successfully completes. * + * If @flags contains VIR_DOMAIN_BLOCK_COMMIT_RELATIVE, the name recorded + * into the overlay of the @top image as path to the new backing file
s/as path/as the path/
+ * will be kept relative to other images in case the backing chain was + * using relative names.
Should we also mention what happens if there is no overlay of @top (that is, when @top is NULL or explicitly mentions the active layer)? Is this flag then silently ignored, or an explicit error?
+ {.name = "keep-relative", + .type = VSH_OT_BOOL, + .help = N_("keep the backing chain relative if it was relatively " + "referenced if it was before")
s/if it was before/before/
+files at the successful completion of the commit operation. When the +I<--keep-relative> flag is used, the backing file paths will be kept relative.
s/relative./relative, if possible./ So even though I'd like to commit to this API, it's probably enough changes to post a v6 to make sure the grammar still reads well. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Introduce flag for the block rebase API to allow the rebase operation to leave the chain relatively addressed. Also adds a virsh switch to enable this behavior. --- include/libvirt/libvirt.h.in | 2 ++ src/libvirt.c | 5 +++++ tools/virsh-domain.c | 22 +++++++++++++++++++--- tools/virsh.pod | 4 ++++ 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index d165fea..eac639e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2580,6 +2580,8 @@ typedef enum { file for a copy */ VIR_DOMAIN_BLOCK_REBASE_COPY_RAW = 1 << 2, /* Make destination file raw */ VIR_DOMAIN_BLOCK_REBASE_COPY = 1 << 3, /* Start a copy job */ + VIR_DOMAIN_BLOCK_REBASE_RELATIVE = 1 << 4, /* Keep backing chain relative + if possible */ } virDomainBlockRebaseFlags; int virDomainBlockRebase(virDomainPtr dom, const char *disk, diff --git a/src/libvirt.c b/src/libvirt.c index eef014e..c45797f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19716,6 +19716,11 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * exists. If the job is aborted, a new one can be started later to * resume from the same point. * + * If @flags contains VIR_DOMAIN_BLOCK_REBASE_RELATIVE, the name recorded + * into the overlay of the @base image as path to the new backing file + * will be kept relative to other images in case the backing chain was + * using relative names. + * * When @flags includes VIR_DOMAIN_BLOCK_REBASE_COPY, this starts a copy, * where @base must be the name of a new file to copy the chain to. By * default, the copy will pull the entire source chain into the destination diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1d99cce..3f864ec 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1479,10 +1479,14 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, case VSH_CMD_BLOCK_JOB_PULL: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) goto cleanup; - if (base) - ret = virDomainBlockRebase(dom, path, base, bandwidth, 0); - else + if (base) { + if (vshCommandOptBool(cmd, "keep-relative")) + flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE; + + ret = virDomainBlockRebase(dom, path, base, bandwidth, flags); + } else { ret = virDomainBlockPull(dom, path, bandwidth, 0); + } break; case VSH_CMD_BLOCK_JOB_COMMIT: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0 || @@ -2119,6 +2123,11 @@ static const vshCmdOptDef opts_block_pull[] = { .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish") }, + {.name = "keep-relative", + .type = VSH_OT_BOOL, + .help = N_("keep the backing chain relative if it was relatively " + "referenced if it was before") + }, {.name = NULL} }; @@ -2139,6 +2148,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0; + if (vshCommandOptBool(cmd, "keep-relative") && + !vshCommandOptBool(cmd, "base")) { + vshError(ctl, "%s", _("--keep-relative is supported only with partial " + "pull operations with --base specified")); + return false; + } + if (blocking) { if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index 40163b2..6d2ce83 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -873,6 +873,7 @@ I<bandwidth> specifies copying bandwidth limit in MiB/s. =item B<blockpull> I<domain> I<path> [I<bandwidth>] [I<base>] [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] +[I<--keep-relative>] Populate a disk from its backing image chain. By default, this command flattens the entire chain; but if I<base> is specified, containing the @@ -892,6 +893,9 @@ is triggered, I<--async> will return control to the user as fast as possible, otherwise the command may continue to block a little while longer until the job is done cleaning up. +Using the I<--keep-relative> flag will try to keep the backing chain names +relative (if they were relative before). + I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Introduce flag for the block rebase API to allow the rebase operation to leave the chain relatively addressed. Also adds a virsh switch to enable this behavior. --- include/libvirt/libvirt.h.in | 2 ++ src/libvirt.c | 5 +++++ tools/virsh-domain.c | 22 +++++++++++++++++++--- tools/virsh.pod | 4 ++++ 4 files changed, 30 insertions(+), 3 deletions(-)
Similar comments to 16/19 about being gated on qemu.git.
+++ b/src/libvirt.c @@ -19716,6 +19716,11 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * exists. If the job is aborted, a new one can be started later to * resume from the same point. * + * If @flags contains VIR_DOMAIN_BLOCK_REBASE_RELATIVE, the name recorded + * into the overlay of the @base image as path to the new backing file + * will be kept relative to other images in case the backing chain was + * using relative names.
Quite wordy since the overlay of @base is always the active layer (given the current limitations of blockpull); how about: If @flags contains VIR_DOMAIN_BLOCK_REBASE_RELATIVE, the name recorded into the active disk as the location for @base will be kept relative, if the backing chain was using relative names. Also needs to mention what happens if this flag is set bug @base is omitted (silently ignored, or explicit error?)
+++ b/tools/virsh-domain.c @@ -1479,10 +1479,14 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, case VSH_CMD_BLOCK_JOB_PULL: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) goto cleanup; - if (base) - ret = virDomainBlockRebase(dom, path, base, bandwidth, 0); - else + if (base) { + if (vshCommandOptBool(cmd, "keep-relative")) + flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE;
Here, you silently ignore the flag if base is omitted. Is it worth calling the new API when the flag is specified but base is NULL, in order to let virsh serve as a test for what happens if the flag is set in error?
+ + ret = virDomainBlockRebase(dom, path, base, bandwidth, flags); + } else { ret = virDomainBlockPull(dom, path, bandwidth, 0); + }
In fact, I think you want to modify flags in advance, and then do if (base || flags) virDomainBlockRebase(); else virDomainBlockPull()
+ {.name = "keep-relative", + .type = VSH_OT_BOOL, + .help = N_("keep the backing chain relative if it was relatively " + "referenced if it was before")
s/if it was before/before/
@@ -2139,6 +2148,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0;
+ if (vshCommandOptBool(cmd, "keep-relative") && + !vshCommandOptBool(cmd, "base")) { + vshError(ctl, "%s", _("--keep-relative is supported only with partial " + "pull operations with --base specified")); + return false; + }
Again, if virsh does less validation up front, then we can ensure that lower in the stack behaves sanely with unusual requests. I'm not sure this condition is worth having in virsh.
+++ b/tools/virsh.pod
+Using the I<--keep-relative> flag will try to keep the backing chain names +relative (if they were relative before).
Hmm, this wording is a bit nicer compared to the sentence you added in 16/19; might be worth trying to make them similar. Looking forward to v6. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu. --- src/qemu/qemu_driver.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f35123..338ec05 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15496,11 +15496,14 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *top_parent = NULL; bool clean_access = false; virStorageSourcePtr mirror = NULL; - + char *topPath = NULL; + char *basePath = NULL; + char *backingPath = NULL; /* XXX Add support for COMMIT_DELETE */ virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | - VIR_DOMAIN_BLOCK_COMMIT_ACTIVE, -1); + VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | + VIR_DOMAIN_BLOCK_COMMIT_RELATIVE, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -15630,6 +15633,31 @@ qemuDomainBlockCommit(virDomainPtr dom, mirror->format = baseSource->format; } + if (qemuGetDriveSourceString(topSource, NULL, &topPath) < 0) + goto endjob; + + if (qemuGetDriveSourceString(baseSource, NULL, &basePath) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && + topSource != disk->src) { + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support relative blockpull")); + goto endjob; + } + + if (virStorageFileGetRelativeBackingPath(topSource, baseSource, + &backingPath) < 0) + goto endjob; + + if (!backingPath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't keep relative backing relationship.")); + goto endjob; + } + } + /* Start the commit operation. Pass the user's original spelling, * if any, through to qemu, since qemu may behave differently * depending on whether the input was specified as relative or @@ -15637,9 +15665,7 @@ qemuDomainBlockCommit(virDomainPtr dom, * thing if the user specified a relative name). */ qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockCommit(priv->mon, device, - top && !topIndex ? top : topSource->path, - base && !baseIndex ? base : baseSource->path, - NULL, + topPath, basePath, backingPath, bandwidth); qemuDomainObjExitMonitor(driver, vm); @@ -15664,6 +15690,9 @@ qemuDomainBlockCommit(virDomainPtr dom, vm = NULL; cleanup: + VIR_FREE(topPath); + VIR_FREE(basePath); + VIR_FREE(backingPath); VIR_FREE(device); if (vm) virObjectUnlock(vm); -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu.
Eventually, we'll want to use qemu's node-name functionality, also being added (but possibly in qemu 2.2 instead of 2.1, depends on how Jeff's series goes). But for the simpler case of all files being local or all files being network from the same pool (that is, no mixed-mode chains), then this does appear to work at getting a decent name into qemu, at which point qemu can indeed commit to the right target.
--- src/qemu/qemu_driver.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)
+ + if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && + topSource != disk->src) {
So you are silently ignoring the flag if topSource is the active layer? That's okay, but reflect it in the documentation earlier in the series.
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support relative blockpull")); + goto endjob; + } + + if (virStorageFileGetRelativeBackingPath(topSource, baseSource, + &backingPath) < 0) + goto endjob; + + if (!backingPath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't keep relative backing relationship."));
No '.' at end of the message. Wait - the earlier patches said that relative names would be preserved if possible, implying that an absolute name would still be used if a relative name was not possible. But this errors out if a relative name was not possible. Which is nicer to the end user, treating the flag as advisory or mandatory? I'm hoping Adam can answer which he'd prefer, as one of the first clients of this new feature. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 25/06/14 10:27 -0600, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu.
Eventually, we'll want to use qemu's node-name functionality, also being added (but possibly in qemu 2.2 instead of 2.1, depends on how Jeff's series goes). But for the simpler case of all files being local or all files being network from the same pool (that is, no mixed-mode chains), then this does appear to work at getting a decent name into qemu, at which point qemu can indeed commit to the right target.
--- src/qemu/qemu_driver.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)
+ + if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && + topSource != disk->src) {
So you are silently ignoring the flag if topSource is the active layer? That's okay, but reflect it in the documentation earlier in the series.
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support relative blockpull")); + goto endjob; + } + + if (virStorageFileGetRelativeBackingPath(topSource, baseSource, + &backingPath) < 0) + goto endjob; + + if (!backingPath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't keep relative backing relationship."));
No '.' at end of the message.
Wait - the earlier patches said that relative names would be preserved if possible, implying that an absolute name would still be used if a relative name was not possible. But this errors out if a relative name was not possible. Which is nicer to the end user, treating the flag as advisory or mandatory? I'm hoping Adam can answer which he'd prefer, as one of the first clients of this new feature.
Thanks Eric. If the flag was specified we need it to fail if a relative backing path is not possible. Otherwise the backing chain could be rewritten such that the VM can not be started on a different host in the future. For us, not honoring the flag is a corruption. For those applications that don't mind (or might handle abs paths differently than relative ones, they could retry the operation without the flag. Perhaps we'll want a specific error code for this scenario to make it easy to handle? -- Adam Litke
On 06/25/2014 12:13 PM, Adam Litke wrote:
On 25/06/14 10:27 -0600, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu.
Eventually, we'll want to use qemu's node-name functionality, also being added (but possibly in qemu 2.2 instead of 2.1, depends on how Jeff's series goes). But for the simpler case of all files being local or all files being network from the same pool (that is, no mixed-mode chains), then this does appear to work at getting a decent name into qemu, at which point qemu can indeed commit to the right target.
Wait - the earlier patches said that relative names would be preserved if possible, implying that an absolute name would still be used if a relative name was not possible. But this errors out if a relative name was not possible. Which is nicer to the end user, treating the flag as advisory or mandatory? I'm hoping Adam can answer which he'd prefer, as one of the first clients of this new feature.
Thanks Eric. If the flag was specified we need it to fail if a relative backing path is not possible. Otherwise the backing chain could be rewritten such that the VM can not be started on a different host in the future. For us, not honoring the flag is a corruption.
Okay, let's go with mandatory semantics on the respin of this series. If the flag is present, we fail unless we were able to write a relative name into the affected file (which implies that using the flag while the chain already had absolute names is a guaranteed failure).
For those applications that don't mind (or might handle abs paths differently than relative ones, they could retry the operation without the flag. Perhaps we'll want a specific error code for this scenario to make it easy to handle?
I wouldn't bother with a special error code unless someone specifically asks for it in their use case. We can always add it later, if needed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu. --- src/qemu/qemu_driver.c | 45 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 338ec05..1ee14b0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15033,6 +15033,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; unsigned int baseIndex = 0; + char *basePath = NULL; + char *backingPath = NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -15040,6 +15042,13 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto cleanup; } + if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE is valid only " + " with non-null base ")); + goto cleanup; + } + priv = vm->privateData; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { async = true; @@ -15100,10 +15109,35 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, base, baseIndex, NULL)))) goto endjob; + if (baseSource) { + if (qemuGetDriveSourceString(baseSource, NULL, &basePath) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) { + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this QEMU binary doesn't support relative " + "block pull/rebase")); + goto endjob; + } + + if (virStorageFileGetRelativeBackingPath(disk->src->backingStore, + baseSource, + &backingPath) < 0) + goto endjob; + + + if (!backingPath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't keep relative backing relationship.")); + goto endjob; + } + } + } + qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJob(priv->mon, device, - baseIndex ? baseSource->path : base, - NULL, bandwidth, info, mode, async); + ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, + bandwidth, info, mode, async); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) goto endjob; @@ -15179,6 +15213,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, } cleanup: + VIR_FREE(basePath); + VIR_FREE(backingPath); VIR_FREE(device); if (vm) virObjectUnlock(vm); @@ -15431,7 +15467,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | VIR_DOMAIN_BLOCK_REBASE_COPY | - VIR_DOMAIN_BLOCK_REBASE_COPY_RAW, -1); + VIR_DOMAIN_BLOCK_REBASE_COPY_RAW | + VIR_DOMAIN_BLOCK_REBASE_RELATIVE, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; -- 1.9.3
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu. --- src/qemu/qemu_driver.c | 45 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-)
Same caveats as in 18/19 about not necessarily working in mixed-source chains (for that, we'd need to use node-names); but as it is definitely more powerful than what libvirt previously supported, it's still worth including under the incremental improvement umbrella.
@@ -15040,6 +15042,13 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto cleanup; }
+ if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE is valid only " + " with non-null base "));
Trailing space in the error message. This treats relative name with no base as a hard error, which is okay but should be documented.
+ + if (!backingPath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't keep relative backing relationship."));
No trailing '.'. Once again, back to the question of whether it is nicer for the flag to be advisory (best effort to use relative, but absolute fallback is okay) or mandatory (fail if the request cannot be honored). At this point, I'm leaning towards mandatory (it's easier to relax mandatory to advisory later than it is to give advisory now and tighten it up later; and I like to know if my explicit request cannot be honored). But the documentation needs to match what we choose, and it would help to have Adam's insight as a client of this flag. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 25/06/14 10:34 -0600, Eric Blake wrote:
On 06/19/2014 07:59 AM, Peter Krempa wrote:
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu. --- src/qemu/qemu_driver.c | 45 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-)
Same caveats as in 18/19 about not necessarily working in mixed-source chains (for that, we'd need to use node-names); but as it is definitely more powerful than what libvirt previously supported, it's still worth including under the incremental improvement umbrella.
@@ -15040,6 +15042,13 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto cleanup; }
+ if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE is valid only " + " with non-null base "));
Trailing space in the error message. This treats relative name with no base as a hard error, which is okay but should be documented.
+ + if (!backingPath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Can't keep relative backing relationship."));
No trailing '.'. Once again, back to the question of whether it is nicer for the flag to be advisory (best effort to use relative, but absolute fallback is okay) or mandatory (fail if the request cannot be honored).
At this point, I'm leaning towards mandatory (it's easier to relax mandatory to advisory later than it is to give advisory now and tighten it up later; and I like to know if my explicit request cannot be honored). But the documentation needs to match what we choose, and it would help to have Adam's insight as a client of this flag.
See response to 18... Mandatory is our strict requirement. -- Adam Litke
participants (3)
-
Adam Litke -
Eric Blake -
Peter Krempa