8:59 a.m.
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 01/19] util: string: Add helper to free non-NULL terminated string arrays
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
5:41 p.m.
New subject: [libvirt] [PATCHv5 01/19] util: string: Add helper to free non-NULL terminated string arrays
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
2:59 a.m.
New subject: [libvirt] [PATCHv5 01/19] util: string: Add helper to free non-NULL terminated string arrays
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
11:29 a.m.
New subject: [libvirt] [PATCHv5 01/19] util: string: Add helper to free non-NULL terminated string arrays
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 02/19] util: storagefile: Introduce universal function to canonicalize paths
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
9:35 a.m.
New subject: [libvirt] [PATCHv5 02/19] util: storagefile: Introduce universal function to canonicalize paths
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
3:28 a.m.
New subject: [libvirt] [PATCHv5 02/19] util: storagefile: Introduce universal function to canonicalize paths
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
10:12 a.m.
New subject: [libvirt] [PATCHv5 02/19] util: storagefile: Introduce universal function to canonicalize paths
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 03/19] storage: gluster: Add backend to return unique storage file path
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
10:35 p.m.
New subject: [libvirt] [PATCHv5 03/19] storage: gluster: Add backend to return unique storage file path
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
3:55 a.m.
New subject: [libvirt] [PATCHv5 03/19] storage: gluster: Add backend to return unique storage file path
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 04/19] util: storage: Add helper to resolve relative path difference
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
3:38 p.m.
New subject: [libvirt] [PATCHv5 04/19] util: storage: Add helper to resolve relative path difference
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 05/19] tests: virstoragetest: Remove "expBackingStore" field
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
3:45 p.m.
New subject: [libvirt] [PATCHv5 05/19] tests: virstoragetest: Remove "expBackingStore" field
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 06/19] storage: Store relative path only for relatively backed storage
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
3:58 p.m.
New subject: [libvirt] [PATCHv5 06/19] storage: Store relative path only for relatively backed storage
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 07/19] tests: virstoragetest: Remove now unused pathAbs
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
3:58 p.m.
New subject: [libvirt] [PATCHv5 07/19] tests: virstoragetest: Remove now unused pathAbs
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 08/19] util: storage: Remove now redundant backingRelative from virStorageSource
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
4 p.m.
New subject: [libvirt] [PATCHv5 08/19] util: storage: Remove now redundant backingRelative from virStorageSource
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 09/19] tests: virstoragetest: Don't test relative start of backing chains
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
4:05 p.m.
New subject: [libvirt] [PATCHv5 09/19] tests: virstoragetest: Don't test relative start of backing chains
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 10/19] tests: virstoragetest: Remove unneeded relative test plumbing
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
4:10 p.m.
New subject: [libvirt] [PATCHv5 10/19] tests: virstoragetest: Remove unneeded relative test plumbing
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 11/19] storage: Don't canonicalize paths unnecessarily
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
4:16 p.m.
New subject: [libvirt] [PATCHv5 11/19] storage: Don't canonicalize paths unnecessarily
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 12/19] storage: Don't store parent directory of an image explicitly
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
5:59 p.m.
New subject: [libvirt] [PATCHv5 12/19] storage: Don't store parent directory of an image explicitly
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
3:10 a.m.
New subject: [libvirt] [PATCHv5 12/19] storage: Don't store parent directory of an image explicitly
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 13/19] qemu: caps: Add capability for change-backing-file command
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
6:03 p.m.
New subject: [libvirt] [PATCHv5 13/19] qemu: caps: Add capability for change-backing-file command
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 14/19] qemu: monitor: Add argument for specifying backing name for block commit
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
6:02 p.m.
New subject: [libvirt] [PATCHv5 14/19] qemu: monitor: Add argument for specifying backing name for block commit
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 15/19] qemu: monitor: Add support for backing name specification for block-stream
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
11:01 a.m.
New subject: [libvirt] [PATCHv5 15/19] qemu: monitor: Add support for backing name specification for block-stream
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 16/19] lib: Introduce flag VIR_DOMAIN_BLOCK_COMMIT_RELATIVE
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
11:05 a.m.
New subject: [libvirt] [PATCHv5 16/19] lib: Introduce flag VIR_DOMAIN_BLOCK_COMMIT_RELATIVE
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 17/19] lib: Introduce flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE
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
11:18 a.m.
New subject: [libvirt] [PATCHv5 17/19] lib: Introduce flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 18/19] qemu: Add support for networked disks for block commit
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
11:27 a.m.
New subject: [libvirt] [PATCHv5 18/19] qemu: Add support for networked disks for block commit
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
1:13 p.m.
New subject: [libvirt] [PATCHv5 18/19] qemu: Add support for networked disks for block commit
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
2:13 p.m.
New subject: [libvirt] [PATCHv5 18/19] qemu: Add support for networked disks for block commit
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
8:59 a.m.
New subject: [libvirt] [PATCHv5 19/19] qemu: Add support for networked disks for block pull/block rebase
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
11:34 a.m.
New subject: [libvirt] [PATCHv5 19/19] qemu: Add support for networked disks for block pull/block rebase
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
1:15 p.m.
New subject: [libvirt] [PATCHv5 19/19] qemu: Add support for networked disks for block pull/block rebase
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
3806
days inactive
3812
days old
47 comments
3 participants
participants (3)
-
Adam Litke -
Eric Blake -
Peter Krempa