[libvirt] [PATCH 0/6] another virstoragefile cleanup
Today's cleanup: add tests for backing chain lookups, and remove another redundant field. Eric Blake (6): conf: test backing chain lookup util: new virFileRelLinkPointsTo function conf: report error on chain lookup failure conf: drop redundant parameter to chain lookup conf: tweak chain lookup internals conf: delete internal directory field src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 13 +-- src/util/virfile.c | 26 +++++- src/util/virfile.h | 3 + src/util/virstoragefile.c | 96 +++++++++++------------ src/util/virstoragefile.h | 2 - src/xen/xm_internal.c | 11 ++- tests/virstoragetest.c | 196 +++++++++++++++++++++++++++++++++++++++------- 8 files changed, 251 insertions(+), 97 deletions(-) -- 1.9.0
I realized that we had no good test coverage of looking up a name from within a backing chain, even though code like block-commit is relying on it. * tests/virstoragetest.c (testStorageLookup): New function. (mymain): New tests. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 029561c..2f181a2 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -378,12 +378,81 @@ testStorageChain(const void *args) return ret; } +struct testLookupData +{ + virStorageFileMetadataPtr chain; + const char *start; + const char *name; + const char *expResult; + virStorageFileMetadataPtr expMeta; + const char *expParent; +}; + +static int +testStorageLookup(const void *args) +{ + const struct testLookupData *data = args; + int ret = 0; + const char *actualResult; + virStorageFileMetadataPtr actualMeta; + const char *actualParent; + + /* This function is documented as giving results within chain, but + * as the same string may be duplicated into more than one field, + * we rely on STREQ rather than pointer equality. Test twice to + * ensure optional parameters don't cause NULL deref. */ + actualResult = virStorageFileChainLookup(data->chain, data->start, + data->name, NULL, NULL); + + if (!data->expResult) { + if (!virGetLastError()) { + fprintf(stderr, "call should have failed\n"); + ret = -1; + } + virResetLastError(); + } else { + if (virGetLastError()) { + fprintf(stderr, "call should not have warned\n"); + ret = -1; + } + } + + if (STRNEQ_NULLABLE(data->expResult, actualResult)) { + fprintf(stderr, "result 1: expected %s, got %s\n", + NULLSTR(data->expResult), NULLSTR(actualResult)); + ret = -1; + } + + actualResult = virStorageFileChainLookup(data->chain, data->start, + data->name, &actualMeta, + &actualParent); + if (STRNEQ_NULLABLE(data->expResult, actualResult)) { + fprintf(stderr, "result 2: expected %s, got %s\n", + NULLSTR(data->expResult), NULLSTR(actualResult)); + ret = -1; + } + if (data->expMeta != actualMeta) { + fprintf(stderr, "meta: expected %p, got %p\n", + data->expMeta, actualMeta); + ret = -1; + } + if (STRNEQ_NULLABLE(data->expParent, actualParent)) { + fprintf(stderr, "parent: expected %s, got %s\n", + NULLSTR(data->expParent), NULLSTR(actualParent)); + ret = -1; + } + + return ret; +} + static int mymain(void) { int ret; virCommandPtr cmd = NULL; struct testChainData data; + virStorageFileMetadataPtr chain = NULL; + const char *start = NULL; /* Prep some files with qemu-img; if that is not found on PATH, or * if it lacks support for qcow2 and qed, skip this test. */ @@ -771,7 +840,105 @@ mymain(void) (&wrap, &qcow2), EXP_WARN, (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN); + /* Rewrite wrap and qcow2 back to 3-deep chain, absolute backing */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", absraw, "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Test behavior of chain lookups, absolute backing */ + start = "wrap"; + chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, + -1, -1, false); + if (!chain) + return EXIT_FAILURE; + +#define TEST_LOOKUP(id, name, result, meta, parent) \ + do { \ + struct testLookupData data2 = { chain, start, name, \ + result, meta, parent, }; \ + if (virtTestRun("Chain lookup " #id, \ + testStorageLookup, &data2) < 0) \ + ret = -1; \ + } while (0) + + TEST_LOOKUP(0, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(1, "wrap", start, chain, NULL); + TEST_LOOKUP(2, abswrap, start, chain, NULL); + TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(4, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(5, "raw", chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(6, absraw, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(7, NULL, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + + /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "raw", "-b", "raw", "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", "qcow2", "wrap", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Test behavior of chain lookups, relative backing */ + start = abswrap; + virStorageFileFreeMetadata(chain); + chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, + -1, -1, false); + if (!chain) + return EXIT_FAILURE; + + TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(9, "wrap", start, chain, NULL); + TEST_LOOKUP(10, abswrap, start, chain, NULL); + TEST_LOOKUP(11, "qcow2", chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(12, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(13, "raw", chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(14, absraw, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(15, NULL, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + + /* Use link to wrap with cross-directory relative backing */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", "../qcow2", "wrap", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Test behavior of chain lookups, relative backing */ + start = "sub/link2"; + virStorageFileFreeMetadata(chain); + chain = virStorageFileGetMetadata(start, VIR_STORAGE_FILE_QCOW2, + -1, -1, false); + if (!chain) + return EXIT_FAILURE; + + TEST_LOOKUP(16, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(17, "sub/link2", start, chain, NULL); + TEST_LOOKUP(18, "wrap", start, chain, NULL); + TEST_LOOKUP(19, abswrap, start, chain, NULL); + TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(24, absraw, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(25, NULL, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + /* Final cleanup */ + virStorageFileFreeMetadata(chain); testCleanupImages(); virCommandFree(cmd); -- 1.9.0
On 04/11/2014 12:21 AM, Eric Blake wrote:
I realized that we had no good test coverage of looking up a name from within a backing chain, even though code like block-commit is relying on it.
* tests/virstoragetest.c (testStorageLookup): New function. (mymain): New tests.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstoragetest.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+)
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 029561c..2f181a2 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -378,12 +378,81 @@ testStorageChain(const void *args) return ret; }
+struct testLookupData +{ + virStorageFileMetadataPtr chain; + const char *start; + const char *name; + const char *expResult; + virStorageFileMetadataPtr expMeta; + const char *expParent; +}; + +static int +testStorageLookup(const void *args) +{ + const struct testLookupData *data = args; + int ret = 0; + const char *actualResult; + virStorageFileMetadataPtr actualMeta; + const char *actualParent; + + /* This function is documented as giving results within chain, but + * as the same string may be duplicated into more than one field, + * we rely on STREQ rather than pointer equality. Test twice to + * ensure optional parameters don't cause NULL deref. */ + actualResult = virStorageFileChainLookup(data->chain, data->start, + data->name, NULL, NULL); + + if (!data->expResult) { + if (!virGetLastError()) { + fprintf(stderr, "call should have failed\n"); + ret = -1; + } + virResetLastError(); + } else { + if (virGetLastError()) { + fprintf(stderr, "call should not have warned\n"); + ret = -1;
Should ResetLast be called here? So as to not generate future false positive/failure scenario. Only when running the test directly (e.g. not via make check or make -C tests ...) with VIR_TEST_DEBUG (or VERBOSE) did I see the error messages printed (for 0, 8, 16, & 21)
+ } + } + + if (STRNEQ_NULLABLE(data->expResult, actualResult)) { + fprintf(stderr, "result 1: expected %s, got %s\n", + NULLSTR(data->expResult), NULLSTR(actualResult)); + ret = -1; + } + + actualResult = virStorageFileChainLookup(data->chain, data->start, + data->name, &actualMeta, + &actualParent);
So no LastError checking/clearing here? Just checking... I haven't followed all the functionality changes before this that closely.
+ if (STRNEQ_NULLABLE(data->expResult, actualResult)) { + fprintf(stderr, "result 2: expected %s, got %s\n", + NULLSTR(data->expResult), NULLSTR(actualResult)); + ret = -1; + } + if (data->expMeta != actualMeta) { + fprintf(stderr, "meta: expected %p, got %p\n", + data->expMeta, actualMeta); + ret = -1; + } + if (STRNEQ_NULLABLE(data->expParent, actualParent)) { + fprintf(stderr, "parent: expected %s, got %s\n", + NULLSTR(data->expParent), NULLSTR(actualParent)); + ret = -1; + } + + return ret; +} + static int mymain(void) { int ret; virCommandPtr cmd = NULL; struct testChainData data; + virStorageFileMetadataPtr chain = NULL; + const char *start = NULL;
/* Prep some files with qemu-img; if that is not found on PATH, or * if it lacks support for qcow2 and qed, skip this test. */ @@ -771,7 +840,105 @@ mymain(void) (&wrap, &qcow2), EXP_WARN, (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN);
+ /* Rewrite wrap and qcow2 back to 3-deep chain, absolute backing */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", absraw, "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Test behavior of chain lookups, absolute backing */ + start = "wrap"; + chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, + -1, -1, false); + if (!chain) + return EXIT_FAILURE;
Whether it matters or not cmd is leaked...
+ +#define TEST_LOOKUP(id, name, result, meta, parent) \ + do { \ + struct testLookupData data2 = { chain, start, name, \ + result, meta, parent, }; \ + if (virtTestRun("Chain lookup " #id, \ + testStorageLookup, &data2) < 0) \ + ret = -1; \ + } while (0) + + TEST_LOOKUP(0, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(1, "wrap", start, chain, NULL); + TEST_LOOKUP(2, abswrap, start, chain, NULL); + TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(4, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(5, "raw", chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(6, absraw, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(7, NULL, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + + /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "raw", "-b", "raw", "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", "qcow2", "wrap", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Test behavior of chain lookups, relative backing */ + start = abswrap; + virStorageFileFreeMetadata(chain); + chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, + -1, -1, false); + if (!chain) + return EXIT_FAILURE;
Whether it matters or not cmd is leaked...
+ + TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(9, "wrap", start, chain, NULL); + TEST_LOOKUP(10, abswrap, start, chain, NULL); + TEST_LOOKUP(11, "qcow2", chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(12, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(13, "raw", chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(14, absraw, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(15, NULL, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + + /* Use link to wrap with cross-directory relative backing */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", "../qcow2", "wrap", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Test behavior of chain lookups, relative backing */ + start = "sub/link2"; + virStorageFileFreeMetadata(chain); + chain = virStorageFileGetMetadata(start, VIR_STORAGE_FILE_QCOW2, + -1, -1, false); + if (!chain) + return EXIT_FAILURE; + + TEST_LOOKUP(16, "bogus", NULL, NULL, NULL); + TEST_LOOKUP(17, "sub/link2", start, chain, NULL); + TEST_LOOKUP(18, "wrap", start, chain, NULL); + TEST_LOOKUP(19, abswrap, start, chain, NULL); + TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(24, absraw, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + TEST_LOOKUP(25, NULL, chain->backingMeta->backingStore, + chain->backingMeta->backingMeta, chain->backingStore); + /* Final cleanup */ + virStorageFileFreeMetadata(chain); testCleanupImages(); virCommandFree(cmd);
ACK - in general John
On 04/11/2014 01:11 PM, John Ferlan wrote:
On 04/11/2014 12:21 AM, Eric Blake wrote:
I realized that we had no good test coverage of looking up a name from within a backing chain, even though code like block-commit is relying on it.
* tests/virstoragetest.c (testStorageLookup): New function. (mymain): New tests.
+ } else { + if (virGetLastError()) { + fprintf(stderr, "call should not have warned\n"); + ret = -1;
Should ResetLast be called here? So as to not generate future false positive/failure scenario. Only when running the test directly (e.g. not via make check or make -C tests ...) with VIR_TEST_DEBUG (or VERBOSE) did I see the error messages printed (for 0, 8, 16, & 21)
Here, no. The test is going to fail if the error was reported here, so we might as well print the error rather than resetting it to make it easier to diagnose the failure.
+ } + } + + if (STRNEQ_NULLABLE(data->expResult, actualResult)) { + fprintf(stderr, "result 1: expected %s, got %s\n", + NULLSTR(data->expResult), NULLSTR(actualResult)); + ret = -1; + } + + actualResult = virStorageFileChainLookup(data->chain, data->start, + data->name, &actualMeta, + &actualParent);
So no LastError checking/clearing here? Just checking... I haven't followed all the functionality changes before this that closely.
Oh, you're right - I _do_ need to clear the error here, if actualResult is NULL, to silence the error messages you saw on 0, 8, 16, and 21.
+ /* Test behavior of chain lookups, absolute backing */ + start = "wrap"; + chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, + -1, -1, false); + if (!chain) + return EXIT_FAILURE;
Whether it matters or not cmd is leaked...
We're about to exit, so the memory leak isn't important. But the file system was left dirty, which may not be so good. I'll tweak things to ensure cleanup occurs. Oh, and I should also tweak the cleanup to leave files behind if a particular env-var is set, the way several other tests do (ah, but that's a separate patch, because we aren't consistent on which namespace of env-var to use: LIBVIRT_SKIP_CLEANUP vs. VIR_TEST_DEBUG).
ACK - in general
Thanks; squashing this, and pushing. diff --git i/tests/virstoragetest.c w/tests/virstoragetest.c index 2f181a2..37edac9 100644 --- i/tests/virstoragetest.c +++ w/tests/virstoragetest.c @@ -426,6 +426,8 @@ testStorageLookup(const void *args) actualResult = virStorageFileChainLookup(data->chain, data->start, data->name, &actualMeta, &actualParent); + if (!data->expResult) + virResetLastError(); if (STRNEQ_NULLABLE(data->expResult, actualResult)) { fprintf(stderr, "result 2: expected %s, got %s\n", NULLSTR(data->expResult), NULLSTR(actualResult)); @@ -851,8 +853,10 @@ mymain(void) start = "wrap"; chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, -1, -1, false); - if (!chain) - return EXIT_FAILURE; + if (!chain) { + ret = -1; + goto cleanup; + } #define TEST_LOOKUP(id, name, result, meta, parent) \ do { \ @@ -893,8 +897,10 @@ mymain(void) virStorageFileFreeMetadata(chain); chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, -1, -1, false); - if (!chain) - return EXIT_FAILURE; + if (!chain) { + ret = -1; + goto cleanup; + } TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); TEST_LOOKUP(9, "wrap", start, chain, NULL); @@ -920,8 +926,10 @@ mymain(void) virStorageFileFreeMetadata(chain); chain = virStorageFileGetMetadata(start, VIR_STORAGE_FILE_QCOW2, -1, -1, false); - if (!chain) - return EXIT_FAILURE; + if (!chain) { + ret = -1; + goto cleanup; + } TEST_LOOKUP(16, "bogus", NULL, NULL, NULL); TEST_LOOKUP(17, "sub/link2", start, chain, NULL); @@ -937,6 +945,7 @@ mymain(void) TEST_LOOKUP(25, NULL, chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); + cleanup: /* Final cleanup */ virStorageFileFreeMetadata(chain); testCleanupImages(); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When checking if two filenames point to the same inode (whether by hardlink or symlink), sometimes one of the names might be relative. This convenience function makes it easier to check. * src/util/virfile.h (virFileRelLinkPointsTo): New prototype. * src/util/virfile.c (virFileRelLinkPointsTo): New function. * src/libvirt_private.syms (virfile.h): Export it. * src/xen/xm_internal.c (xenXMDomainGetAutostart): Use it. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 26 +++++++++++++++++++++++++- src/util/virfile.h | 3 +++ src/xen/xm_internal.c | 11 +++++------ 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cd43335..c2bce13 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1255,6 +1255,7 @@ virFilePrintf; virFileReadAll; virFileReadHeaderFD; virFileReadLimFD; +virFileRelLinkPointsTo; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; diff --git a/src/util/virfile.c b/src/util/virfile.c index a28cbf1..10c4337 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1368,7 +1368,8 @@ virFileHasSuffix(const char *str, && (Stat_buf_1).st_dev == (Stat_buf_2).st_dev) /* Return nonzero if checkLink and checkDest - refer to the same file. Otherwise, return 0. */ + * refer to the same file. Otherwise, return 0. + */ int virFileLinkPointsTo(const char *checkLink, const char *checkDest) @@ -1382,6 +1383,29 @@ virFileLinkPointsTo(const char *checkLink, } +/* Return positive if checkLink (residing within directory if not + * absolute) and checkDest refer to the same file. Otherwise, return + * -1 on allocation failure (error reported), or 0 if not the same + * (silent). + */ +int +virFileRelLinkPointsTo(const char *directory, + const char *checkLink, + const char *checkDest) +{ + char *candidate; + int ret; + + if (*checkLink == '/') + return virFileLinkPointsTo(checkLink, checkDest); + if (virAsprintf(&candidate, "%s/%s", directory, checkLink) < 0) + return -1; + ret = virFileLinkPointsTo(candidate, checkDest); + VIR_FREE(candidate); + return ret; +} + + static int virFileResolveLinkHelper(const char *linkpath, bool intermediatePaths, diff --git a/src/util/virfile.h b/src/util/virfile.h index 638378a..168eb0d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -144,6 +144,9 @@ int virFileStripSuffix(char *str, int virFileLinkPointsTo(const char *checkLink, const char *checkDest); +int virFileRelLinkPointsTo(const char *directory, + const char *checkLink, + const char *checkDest); int virFileResolveLink(const char *linkpath, char **resultpath) ATTRIBUTE_RETURN_CHECK; diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 52d2a1e..f25a7df 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1427,25 +1427,24 @@ int xenXMDomainGetAutostart(virDomainDefPtr def, int *autostart) { - char *linkname = xenXMAutostartLinkName(def); char *config = xenXMDomainConfigName(def); int ret = -1; - if (!linkname || !config) + if (!config) goto cleanup; - *autostart = virFileLinkPointsTo(linkname, config); + *autostart = virFileRelLinkPointsTo("/etc/xen/auto/", def->name, config); if (*autostart < 0) { virReportSystemError(errno, - _("cannot check link %s points to config %s"), - linkname, config); + _("cannot check link /etc/xen/auto/%s points " + "to config %s"), + def->name, config); goto cleanup; } ret = 0; cleanup: - VIR_FREE(linkname); VIR_FREE(config); return ret; } -- 1.9.0
On 04/11/2014 12:21 AM, Eric Blake wrote:
When checking if two filenames point to the same inode (whether by hardlink or symlink), sometimes one of the names might be relative. This convenience function makes it easier to check.
* src/util/virfile.h (virFileRelLinkPointsTo): New prototype. * src/util/virfile.c (virFileRelLinkPointsTo): New function. * src/libvirt_private.syms (virfile.h): Export it. * src/xen/xm_internal.c (xenXMDomainGetAutostart): Use it.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 26 +++++++++++++++++++++++++- src/util/virfile.h | 3 +++ src/xen/xm_internal.c | 11 +++++------ 4 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cd43335..c2bce13 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1255,6 +1255,7 @@ virFilePrintf; virFileReadAll; virFileReadHeaderFD; virFileReadLimFD; +virFileRelLinkPointsTo; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; diff --git a/src/util/virfile.c b/src/util/virfile.c index a28cbf1..10c4337 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1368,7 +1368,8 @@ virFileHasSuffix(const char *str, && (Stat_buf_1).st_dev == (Stat_buf_2).st_dev)
/* Return nonzero if checkLink and checkDest - refer to the same file. Otherwise, return 0. */ + * refer to the same file. Otherwise, return 0. + */ int virFileLinkPointsTo(const char *checkLink, const char *checkDest) @@ -1382,6 +1383,29 @@ virFileLinkPointsTo(const char *checkLink, }
+/* Return positive if checkLink (residing within directory if not + * absolute) and checkDest refer to the same file. Otherwise, return + * -1 on allocation failure (error reported), or 0 if not the same + * (silent). + */ +int +virFileRelLinkPointsTo(const char *directory, + const char *checkLink, + const char *checkDest) +{ + char *candidate; + int ret; + + if (*checkLink == '/') + return virFileLinkPointsTo(checkLink, checkDest); + if (virAsprintf(&candidate, "%s/%s", directory, checkLink) < 0) + return -1; + ret = virFileLinkPointsTo(candidate, checkDest); + VIR_FREE(candidate); + return ret; +} + + static int virFileResolveLinkHelper(const char *linkpath, bool intermediatePaths, diff --git a/src/util/virfile.h b/src/util/virfile.h index 638378a..168eb0d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -144,6 +144,9 @@ int virFileStripSuffix(char *str,
int virFileLinkPointsTo(const char *checkLink, const char *checkDest); +int virFileRelLinkPointsTo(const char *directory, + const char *checkLink, + const char *checkDest);
Should there be a ATTRIBUTE_NONNULL(1,2,3) here?? Probably same for PointsTo argument 1...
int virFileResolveLink(const char *linkpath, char **resultpath) ATTRIBUTE_RETURN_CHECK; diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 52d2a1e..f25a7df 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1427,25 +1427,24 @@ int xenXMDomainGetAutostart(virDomainDefPtr def, int *autostart) { - char *linkname = xenXMAutostartLinkName(def); char *config = xenXMDomainConfigName(def); int ret = -1;
- if (!linkname || !config) + if (!config) goto cleanup;
- *autostart = virFileLinkPointsTo(linkname, config); + *autostart = virFileRelLinkPointsTo("/etc/xen/auto/", def->name, config); if (*autostart < 0) { virReportSystemError(errno, - _("cannot check link %s points to config %s"), - linkname, config); + _("cannot check link /etc/xen/auto/%s points " + "to config %s"), + def->name, config); goto cleanup; }
ret = 0;
cleanup: - VIR_FREE(linkname); VIR_FREE(config); return ret; }
ACK in general though John
On 04/11/2014 01:11 PM, John Ferlan wrote:
+++ b/src/util/virfile.h @@ -144,6 +144,9 @@ int virFileStripSuffix(char *str,
int virFileLinkPointsTo(const char *checkLink, const char *checkDest); +int virFileRelLinkPointsTo(const char *directory, + const char *checkLink, + const char *checkDest);
Should there be a ATTRIBUTE_NONNULL(1,2,3) here?? Probably same for PointsTo argument 1...
Just copying existing style, but you are correct that it makes sense for 2 and 3. For 1, NULL is permitted if checkLink is absolute.
ACK in general though
Squashing this and pushing: diff --git i/src/util/virfile.c w/src/util/virfile.c index 10c4337..3eb2703 100644 --- i/src/util/virfile.c +++ w/src/util/virfile.c @@ -1398,6 +1398,12 @@ virFileRelLinkPointsTo(const char *directory, if (*checkLink == '/') return virFileLinkPointsTo(checkLink, checkDest); + if (!directory) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot resolve '%s' without starting directory"), + checkLink); + return -1; + } if (virAsprintf(&candidate, "%s/%s", directory, checkLink) < 0) return -1; ret = virFileLinkPointsTo(candidate, checkDest); diff --git i/src/util/virfile.h w/src/util/virfile.h index 168eb0d..46ef781 100644 --- i/src/util/virfile.h +++ w/src/util/virfile.h @@ -143,10 +143,12 @@ int virFileStripSuffix(char *str, const char *suffix) ATTRIBUTE_RETURN_CHECK; int virFileLinkPointsTo(const char *checkLink, - const char *checkDest); + const char *checkDest) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virFileRelLinkPointsTo(const char *directory, const char *checkLink, - const char *checkDest); + const char *checkDest) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int virFileResolveLink(const char *linkpath, char **resultpath) ATTRIBUTE_RETURN_CHECK; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The chain lookup function was inconsistent on whether it left a message in the log when looking up a name that is not found on the chain (leaving a message for OOM or if name was relative but not part of the chain), and could litter the log even when successful (when name was relative but deep in the chain, use of virFindBackingFile early in the chain would complain about a file not found). It's easier to make the function consistently emit a message exactly once on failure, and to let all callers rely on the clean semantics. * src/util/virstoragefile.c (virStorageFileChainLookup): Always report error on failure. Simplify relative lookups. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Avoid overwriting error. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 10 +--------- src/util/virstoragefile.c | 17 +++++++++-------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b0c6a74..b8cfe27 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15346,9 +15346,6 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, disk->src.path, top, &top_meta, &top_parent))) { - virReportError(VIR_ERR_INVALID_ARG, - _("could not find top '%s' in chain for '%s'"), - top, path); goto endjob; } if (!top_meta || !top_meta->backingStore) { @@ -15360,13 +15357,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) { base_canon = top_meta->backingStore; } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, - base, NULL, NULL))) { - virReportError(VIR_ERR_INVALID_ARG, - _("could not find base '%s' below '%s' in chain " - "for '%s'"), - base ? base : "(default)", top_canon, path); + base, NULL, NULL))) goto endjob; - } /* Note that this code exploits the fact that * virStorageFileChainLookup guarantees a simple pointer * comparison will work, rather than needing full-blown STREQ. */ diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2013914..66a6ab1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1537,7 +1537,8 @@ int virStorageFileGetSCSIKey(const char *path, * backing element is not a file). If PARENT is not NULL, set *PARENT * to the preferred name of the parent (or to NULL if NAME matches * START). Since the results point within CHAIN, they must not be - * independently freed. */ + * independently freed. Reports an error and returns NULL if NAME is + * not found. */ const char * virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, const char *name, virStorageFileMetadataPtr *meta, @@ -1570,15 +1571,12 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, STREQ(name, owner->backingStore)) { break; } else if (virStorageIsFile(owner->backingStore)) { - char *absName = NULL; - if (virFindBackingFile(owner->directory, name, - NULL, &absName) < 0) + int result = virFileRelLinkPointsTo(owner->directory, name, + owner->backingStore); + if (result < 0) goto error; - if (absName && STREQ(absName, owner->backingStore)) { - VIR_FREE(absName); + if (result > 0) break; - } - VIR_FREE(absName); } *parent = owner->backingStore; owner = owner->backingMeta; @@ -1590,6 +1588,9 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, return owner->backingStore; error: + virReportError(VIR_ERR_INVALID_ARG, + _("could not find image '%s' in chain for '%s'"), + name, start); *parent = NULL; if (meta) *meta = NULL; -- 1.9.0
On 04/11/2014 12:21 AM, Eric Blake wrote:
The chain lookup function was inconsistent on whether it left a message in the log when looking up a name that is not found on the chain (leaving a message for OOM or if name was relative but not part of the chain), and could litter the log even when successful (when name was relative but deep in the chain, use of virFindBackingFile early in the chain would complain about a file not found). It's easier to make the function consistently emit a message exactly once on failure, and to let all callers rely on the clean semantics.
* src/util/virstoragefile.c (virStorageFileChainLookup): Always report error on failure. Simplify relative lookups. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Avoid overwriting error.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 10 +--------- src/util/virstoragefile.c | 17 +++++++++-------- 2 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b0c6a74..b8cfe27 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15346,9 +15346,6 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, disk->src.path, top, &top_meta, &top_parent))) { - virReportError(VIR_ERR_INVALID_ARG, - _("could not find top '%s' in chain for '%s'"), - top, path); goto endjob; } if (!top_meta || !top_meta->backingStore) { @@ -15360,13 +15357,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) { base_canon = top_meta->backingStore; } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, - base, NULL, NULL))) { - virReportError(VIR_ERR_INVALID_ARG, - _("could not find base '%s' below '%s' in chain " - "for '%s'"), - base ? base : "(default)", top_canon, path); + base, NULL, NULL))) goto endjob; - }
Does removal of {} violate the one line "rule of thumb"... In either case the "if" portion should be made consistent as well as this now has if (xxx) { oneline } else onelonglinebecauseofarguments Probably would be nice to have an extra line after endjob; and before following comment - just a readability thing for me at least.
/* Note that this code exploits the fact that * virStorageFileChainLookup guarantees a simple pointer * comparison will work, rather than needing full-blown STREQ. */ diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2013914..66a6ab1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1537,7 +1537,8 @@ int virStorageFileGetSCSIKey(const char *path, * backing element is not a file). If PARENT is not NULL, set *PARENT * to the preferred name of the parent (or to NULL if NAME matches * START). Since the results point within CHAIN, they must not be - * independently freed. */ + * independently freed. Reports an error and returns NULL if NAME is + * not found. */ const char * virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, const char *name, virStorageFileMetadataPtr *meta, @@ -1570,15 +1571,12 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, STREQ(name, owner->backingStore)) { break; } else if (virStorageIsFile(owner->backingStore)) { - char *absName = NULL; - if (virFindBackingFile(owner->directory, name, - NULL, &absName) < 0) + int result = virFileRelLinkPointsTo(owner->directory, name, + owner->backingStore); + if (result < 0) goto error; - if (absName && STREQ(absName, owner->backingStore)) { - VIR_FREE(absName); + if (result > 0) break; - } - VIR_FREE(absName); } *parent = owner->backingStore; owner = owner->backingMeta; @@ -1590,6 +1588,9 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, return owner->backingStore;
error: + virReportError(VIR_ERR_INVALID_ARG, + _("could not find image '%s' in chain for '%s'"), + name, start);
Looking at the context of the code expanded out a bit... there's a few checks for !name = can we get here with name == NULL? Looking ahead to patch 4 this will be more important...
*parent = NULL; if (meta) *meta = NULL;
ACK - seems reasonable, just address the possible NULL name... John
On 04/11/2014 01:12 PM, John Ferlan wrote:
On 04/11/2014 12:21 AM, Eric Blake wrote:
The chain lookup function was inconsistent on whether it left a message in the log when looking up a name that is not found on the chain (leaving a message for OOM or if name was relative but not part of the chain), and could litter the log even when successful (when name was relative but deep in the chain, use of virFindBackingFile early in the chain would complain about a file not found). It's easier to make the function consistently emit a message exactly once on failure, and to let all callers rely on the clean semantics.
base_canon = top_meta->backingStore; } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, - base, NULL, NULL))) { - virReportError(VIR_ERR_INVALID_ARG, - _("could not find base '%s' below '%s' in chain " - "for '%s'"), - base ? base : "(default)", top_canon, path); + base, NULL, NULL))) goto endjob; - }
Does removal of {} violate the one line "rule of thumb"...
Eww, I did indeed break the double {} rule.
error: + virReportError(VIR_ERR_INVALID_ARG, + _("could not find image '%s' in chain for '%s'"), + name, start);
Looking at the context of the code expanded out a bit... there's a few checks for !name = can we get here with name == NULL? Looking ahead to patch 4 this will be more important...
Indeed, callers can pass NULL; I'll improve it.
ACK - seems reasonable, just address the possible NULL name...
Pushing with this added: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index b8cfe27..fed7d1c 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -15354,11 +15354,12 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, top_canon, path); goto endjob; } - if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) { + if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) base_canon = top_meta->backingStore; - } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, - base, NULL, NULL))) + else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, + base, NULL, NULL))) goto endjob; + /* Note that this code exploits the fact that * virStorageFileChainLookup guarantees a simple pointer * comparison will work, rather than needing full-blown STREQ. */ diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index 66a6ab1..77cc878 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1588,9 +1588,14 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, return owner->backingStore; error: - virReportError(VIR_ERR_INVALID_ARG, - _("could not find image '%s' in chain for '%s'"), - name, start); + if (name) + virReportError(VIR_ERR_INVALID_ARG, + _("could not find image '%s' in chain for '%s'"), + name, start); + else + virReportError(VIR_ERR_INVALID_ARG, + _("could not find base image in chain for '%s'"), + start); *parent = NULL; if (meta) *meta = NULL; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The original chain lookup code had to pass in the starting name, because it was not available in the chain. But now that we have added fields to the struct, this parameter is redundant. * src/util/virstoragefile.h (virStorageFileChainLookup): Alter signature. * src/util/virstoragefile.c (virStorageFileChainLookup): Adjust handling of top of chain. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Adjust caller. * tests/virstoragetest.c (testStorageLookup, mymain): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- src/util/virstoragefile.c | 18 +++++++-------- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 57 ++++++++++++++++++++++++----------------------- 4 files changed, 39 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b8cfe27..7cb24e6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15343,7 +15343,6 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, top_canon = disk->src.path; top_meta = disk->backingChain; } else if (!(top_canon = virStorageFileChainLookup(disk->backingChain, - disk->src.path, top, &top_meta, &top_parent))) { goto endjob; @@ -15356,7 +15355,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, } if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) { base_canon = top_meta->backingStore; - } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, + } else if (!(base_canon = virStorageFileChainLookup(top_meta, base, NULL, NULL))) goto endjob; /* Note that this code exploits the fact that diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 66a6ab1..dcde9e5 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1529,21 +1529,21 @@ int virStorageFileGetSCSIKey(const char *path, } #endif -/* Given a CHAIN that starts at the named file START, return a string - * pointing to either START or within CHAIN that gives the preferred - * name for the backing file NAME within that chain. Pass NULL for - * NAME to find the base of the chain. If META is not NULL, set *META - * to the point in the chain that describes NAME (or to NULL if the - * backing element is not a file). If PARENT is not NULL, set *PARENT - * to the preferred name of the parent (or to NULL if NAME matches - * START). Since the results point within CHAIN, they must not be +/* Given a CHAIN, look for the backing file NAME within the chain and + * return its canonical name. Pass NULL for NAME to find the base of + * the chain. If META is not NULL, set *META to the point in the + * chain that describes NAME (or to NULL if the backing element is not + * a file). If PARENT is not NULL, set *PARENT to the preferred name + * of the parent (or to NULL if NAME matches the start of the chain). + * Since the results point within CHAIN, they must not be * independently freed. Reports an error and returns NULL if NAME is * not found. */ const char * -virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, +virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *name, virStorageFileMetadataPtr *meta, const char **parent) { + const char *start = chain->canonPath; virStorageFileMetadataPtr owner; const char *tmp; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c747f20..caf1d2f 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -303,7 +303,6 @@ int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, char **broken_file); const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, - const char *start, const char *name, virStorageFileMetadataPtr *meta, const char **parent) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 2f181a2..73bcb0b 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -381,7 +381,6 @@ testStorageChain(const void *args) struct testLookupData { virStorageFileMetadataPtr chain; - const char *start; const char *name; const char *expResult; virStorageFileMetadataPtr expMeta; @@ -401,8 +400,8 @@ testStorageLookup(const void *args) * as the same string may be duplicated into more than one field, * we rely on STREQ rather than pointer equality. Test twice to * ensure optional parameters don't cause NULL deref. */ - actualResult = virStorageFileChainLookup(data->chain, data->start, - data->name, NULL, NULL); + actualResult = virStorageFileChainLookup(data->chain, data->name, + NULL, NULL); if (!data->expResult) { if (!virGetLastError()) { @@ -423,9 +422,8 @@ testStorageLookup(const void *args) ret = -1; } - actualResult = virStorageFileChainLookup(data->chain, data->start, - data->name, &actualMeta, - &actualParent); + actualResult = virStorageFileChainLookup(data->chain, data->name, + &actualMeta, &actualParent); if (STRNEQ_NULLABLE(data->expResult, actualResult)) { fprintf(stderr, "result 2: expected %s, got %s\n", NULLSTR(data->expResult), NULLSTR(actualResult)); @@ -452,7 +450,6 @@ mymain(void) virCommandPtr cmd = NULL; struct testChainData data; virStorageFileMetadataPtr chain = NULL; - const char *start = NULL; /* Prep some files with qemu-img; if that is not found on PATH, or * if it lacks support for qcow2 and qed, skip this test. */ @@ -847,8 +844,7 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; - /* Test behavior of chain lookups, absolute backing */ - start = "wrap"; + /* Test behavior of chain lookups, absolute backing from relative start */ chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) @@ -856,7 +852,7 @@ mymain(void) #define TEST_LOOKUP(id, name, result, meta, parent) \ do { \ - struct testLookupData data2 = { chain, start, name, \ + struct testLookupData data2 = { chain, name, \ result, meta, parent, }; \ if (virtTestRun("Chain lookup " #id, \ testStorageLookup, &data2) < 0) \ @@ -864,10 +860,12 @@ mymain(void) } while (0) TEST_LOOKUP(0, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(1, "wrap", start, chain, NULL); - TEST_LOOKUP(2, abswrap, start, chain, NULL); - TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, start); - TEST_LOOKUP(4, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(1, "wrap", chain->canonPath, chain, NULL); + TEST_LOOKUP(2, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, + chain->canonPath); + TEST_LOOKUP(4, absqcow2, chain->backingStore, chain->backingMeta, + chain->canonPath); TEST_LOOKUP(5, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(6, absraw, chain->backingMeta->backingStore, @@ -888,8 +886,7 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; - /* Test behavior of chain lookups, relative backing */ - start = abswrap; + /* Test behavior of chain lookups, relative backing from absolute start */ virStorageFileFreeMetadata(chain); chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, -1, -1, false); @@ -897,10 +894,12 @@ mymain(void) return EXIT_FAILURE; TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(9, "wrap", start, chain, NULL); - TEST_LOOKUP(10, abswrap, start, chain, NULL); - TEST_LOOKUP(11, "qcow2", chain->backingStore, chain->backingMeta, start); - TEST_LOOKUP(12, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(9, "wrap", chain->canonPath, chain, NULL); + TEST_LOOKUP(10, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(11, "qcow2", chain->backingStore, chain->backingMeta, + chain->canonPath); + TEST_LOOKUP(12, absqcow2, chain->backingStore, chain->backingMeta, + chain->canonPath); TEST_LOOKUP(13, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(14, absraw, chain->backingMeta->backingStore, @@ -916,20 +915,22 @@ mymain(void) ret = -1; /* Test behavior of chain lookups, relative backing */ - start = "sub/link2"; virStorageFileFreeMetadata(chain); - chain = virStorageFileGetMetadata(start, VIR_STORAGE_FILE_QCOW2, + chain = virStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) return EXIT_FAILURE; TEST_LOOKUP(16, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(17, "sub/link2", start, chain, NULL); - TEST_LOOKUP(18, "wrap", start, chain, NULL); - TEST_LOOKUP(19, abswrap, start, chain, NULL); - TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, start); - TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta, start); - TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(17, "sub/link2", chain->canonPath, chain, NULL); + TEST_LOOKUP(18, "wrap", chain->canonPath, chain, NULL); + TEST_LOOKUP(19, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, + chain->canonPath); + TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta, + chain->canonPath); + TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, + chain->canonPath); TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(24, absraw, chain->backingMeta->backingStore, -- 1.9.0
On 04/11/2014 12:21 AM, Eric Blake wrote:
The original chain lookup code had to pass in the starting name, because it was not available in the chain. But now that we have added fields to the struct, this parameter is redundant.
* src/util/virstoragefile.h (virStorageFileChainLookup): Alter signature. * src/util/virstoragefile.c (virStorageFileChainLookup): Adjust handling of top of chain. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Adjust caller. * tests/virstoragetest.c (testStorageLookup, mymain): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- src/util/virstoragefile.c | 18 +++++++-------- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 57 ++++++++++++++++++++++++----------------------- 4 files changed, 39 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b8cfe27..7cb24e6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15343,7 +15343,6 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, top_canon = disk->src.path; top_meta = disk->backingChain; } else if (!(top_canon = virStorageFileChainLookup(disk->backingChain, - disk->src.path, top, &top_meta, &top_parent))) { goto endjob; @@ -15356,7 +15355,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, } if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) { base_canon = top_meta->backingStore; - } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon, + } else if (!(base_canon = virStorageFileChainLookup(top_meta, base, NULL, NULL))) goto endjob; /* Note that this code exploits the fact that diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 66a6ab1..dcde9e5 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1529,21 +1529,21 @@ int virStorageFileGetSCSIKey(const char *path, } #endif
-/* Given a CHAIN that starts at the named file START, return a string - * pointing to either START or within CHAIN that gives the preferred - * name for the backing file NAME within that chain. Pass NULL for - * NAME to find the base of the chain. If META is not NULL, set *META - * to the point in the chain that describes NAME (or to NULL if the - * backing element is not a file). If PARENT is not NULL, set *PARENT - * to the preferred name of the parent (or to NULL if NAME matches - * START). Since the results point within CHAIN, they must not be +/* Given a CHAIN, look for the backing file NAME within the chain and + * return its canonical name. Pass NULL for NAME to find the base of + * the chain. If META is not NULL, set *META to the point in the + * chain that describes NAME (or to NULL if the backing element is not + * a file). If PARENT is not NULL, set *PARENT to the preferred name + * of the parent (or to NULL if NAME matches the start of the chain). + * Since the results point within CHAIN, they must not be * independently freed. Reports an error and returns NULL if NAME is * not found. */ const char * -virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, +virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *name, virStorageFileMetadataPtr *meta, const char **parent) { + const char *start = chain->canonPath; virStorageFileMetadataPtr owner; const char *tmp;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c747f20..caf1d2f 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -303,7 +303,6 @@ int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain, char **broken_file);
const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, - const char *start, const char *name, virStorageFileMetadataPtr *meta, const char **parent)
Expanded out a bit more shows: ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); not being changed - so on input previous chain and start could not be NULL - now 'name' would be NONNULL which covers a previous concern, but probably isn't correct...
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 2f181a2..73bcb0b 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -381,7 +381,6 @@ testStorageChain(const void *args) struct testLookupData { virStorageFileMetadataPtr chain; - const char *start; const char *name; const char *expResult; virStorageFileMetadataPtr expMeta; @@ -401,8 +400,8 @@ testStorageLookup(const void *args) * as the same string may be duplicated into more than one field, * we rely on STREQ rather than pointer equality. Test twice to * ensure optional parameters don't cause NULL deref. */ - actualResult = virStorageFileChainLookup(data->chain, data->start, - data->name, NULL, NULL); + actualResult = virStorageFileChainLookup(data->chain, data->name, + NULL, NULL);
if (!data->expResult) { if (!virGetLastError()) { @@ -423,9 +422,8 @@ testStorageLookup(const void *args) ret = -1; }
- actualResult = virStorageFileChainLookup(data->chain, data->start, - data->name, &actualMeta, - &actualParent); + actualResult = virStorageFileChainLookup(data->chain, data->name, + &actualMeta, &actualParent); if (STRNEQ_NULLABLE(data->expResult, actualResult)) { fprintf(stderr, "result 2: expected %s, got %s\n", NULLSTR(data->expResult), NULLSTR(actualResult)); @@ -452,7 +450,6 @@ mymain(void) virCommandPtr cmd = NULL; struct testChainData data; virStorageFileMetadataPtr chain = NULL; - const char *start = NULL;
/* Prep some files with qemu-img; if that is not found on PATH, or * if it lacks support for qcow2 and qed, skip this test. */ @@ -847,8 +844,7 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1;
- /* Test behavior of chain lookups, absolute backing */ - start = "wrap"; + /* Test behavior of chain lookups, absolute backing from relative start */ chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) @@ -856,7 +852,7 @@ mymain(void)
#define TEST_LOOKUP(id, name, result, meta, parent) \ do { \ - struct testLookupData data2 = { chain, start, name, \ + struct testLookupData data2 = { chain, name, \ result, meta, parent, }; \ if (virtTestRun("Chain lookup " #id, \ testStorageLookup, &data2) < 0) \ @@ -864,10 +860,12 @@ mymain(void) } while (0)
TEST_LOOKUP(0, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(1, "wrap", start, chain, NULL); - TEST_LOOKUP(2, abswrap, start, chain, NULL); - TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, start); - TEST_LOOKUP(4, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(1, "wrap", chain->canonPath, chain, NULL); + TEST_LOOKUP(2, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta, + chain->canonPath); + TEST_LOOKUP(4, absqcow2, chain->backingStore, chain->backingMeta, + chain->canonPath); TEST_LOOKUP(5, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(6, absraw, chain->backingMeta->backingStore, @@ -888,8 +886,7 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1;
- /* Test behavior of chain lookups, relative backing */ - start = abswrap; + /* Test behavior of chain lookups, relative backing from absolute start */ virStorageFileFreeMetadata(chain); chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, -1, -1, false); @@ -897,10 +894,12 @@ mymain(void) return EXIT_FAILURE;
TEST_LOOKUP(8, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(9, "wrap", start, chain, NULL); - TEST_LOOKUP(10, abswrap, start, chain, NULL); - TEST_LOOKUP(11, "qcow2", chain->backingStore, chain->backingMeta, start); - TEST_LOOKUP(12, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(9, "wrap", chain->canonPath, chain, NULL); + TEST_LOOKUP(10, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(11, "qcow2", chain->backingStore, chain->backingMeta, + chain->canonPath); + TEST_LOOKUP(12, absqcow2, chain->backingStore, chain->backingMeta, + chain->canonPath); TEST_LOOKUP(13, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(14, absraw, chain->backingMeta->backingStore, @@ -916,20 +915,22 @@ mymain(void) ret = -1;
/* Test behavior of chain lookups, relative backing */ - start = "sub/link2"; virStorageFileFreeMetadata(chain); - chain = virStorageFileGetMetadata(start, VIR_STORAGE_FILE_QCOW2, + chain = virStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2, -1, -1, false); if (!chain) return EXIT_FAILURE;
TEST_LOOKUP(16, "bogus", NULL, NULL, NULL); - TEST_LOOKUP(17, "sub/link2", start, chain, NULL); - TEST_LOOKUP(18, "wrap", start, chain, NULL); - TEST_LOOKUP(19, abswrap, start, chain, NULL); - TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, start); - TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta, start); - TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, start); + TEST_LOOKUP(17, "sub/link2", chain->canonPath, chain, NULL); + TEST_LOOKUP(18, "wrap", chain->canonPath, chain, NULL); + TEST_LOOKUP(19, abswrap, chain->canonPath, chain, NULL); + TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, + chain->canonPath); + TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta, + chain->canonPath); + TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, + chain->canonPath); TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore, chain->backingMeta->backingMeta, chain->backingStore); TEST_LOOKUP(24, absraw, chain->backingMeta->backingStore,
ACK - just remove the NONNULL(2) it seems. John
On 04/11/2014 01:18 PM, John Ferlan wrote:
On 04/11/2014 12:21 AM, Eric Blake wrote:
The original chain lookup code had to pass in the starting name, because it was not available in the chain. But now that we have added fields to the struct, this parameter is redundant.
* src/util/virstoragefile.h (virStorageFileChainLookup): Alter signature. * src/util/virstoragefile.c (virStorageFileChainLookup): Adjust handling of top of chain. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Adjust caller. * tests/virstoragetest.c (testStorageLookup, mymain): Likewise.
const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, - const char *start, const char *name, virStorageFileMetadataPtr *meta, const char **parent)
Expanded out a bit more shows:
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
not being changed - so on input previous chain and start could not be NULL - now 'name' would be NONNULL which covers a previous concern, but probably isn't correct...
Indeed, name can be NULL, per the documentation comment, and I just overlooked the attributes when removing a parameter.
ACK - just remove the NONNULL(2) it seems.
Fixed with this and pushed. diff --git i/src/util/virstoragefile.h w/src/util/virstoragefile.h index caf1d2f..959937c 100644 --- i/src/util/virstoragefile.h +++ w/src/util/virstoragefile.h @@ -306,7 +306,7 @@ const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *name, virStorageFileMetadataPtr *meta, const char **parent) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_NONNULL(1); void virStorageFileFreeMetadata(virStorageFileMetadataPtr meta); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Thanks to the testsuite, I feel quite confident that this rewrite still gives the same results for all cases except for one, and I can make the argument that _that_ case was a pre-existing bug. When looking up relative names, the lookup is supposed to be pegged to the directory that contains the parent qcow2 file. * src/util/virstoragefile.c (virStorageFileChainLookup): Depend on new rather than old fields. * tests/virstoragetest.c (mymain): Adjust test to match fix. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 49 +++++++++++++++++++---------------------------- tests/virstoragetest.c | 3 +-- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dcde9e5..e33f6ef 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1544,48 +1544,39 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char **parent) { const char *start = chain->canonPath; - virStorageFileMetadataPtr owner; const char *tmp; + const char *parentDir = "."; + bool nameIsFile = virStorageIsFile(name); if (!parent) parent = &tmp; *parent = NULL; - if (name ? STREQ(start, name) || virFileLinkPointsTo(start, name) : - !chain->backingStore) { - if (meta) - *meta = chain; - return start; - } - - owner = chain; - *parent = start; - while (owner) { - if (!owner->backingStore) - goto error; + while (chain) { if (!name) { - if (!owner->backingMeta || - !owner->backingMeta->backingStore) + if (!chain->backingMeta) break; - } else if (STREQ_NULLABLE(name, owner->backingStoreRaw) || - STREQ(name, owner->backingStore)) { - break; - } else if (virStorageIsFile(owner->backingStore)) { - int result = virFileRelLinkPointsTo(owner->directory, name, - owner->backingStore); - if (result < 0) - goto error; - if (result > 0) + } else { + if (STREQ(name, chain->path)) break; + if (nameIsFile && chain->type < VIR_STORAGE_TYPE_DIR) { + int result = virFileRelLinkPointsTo(parentDir, name, + chain->canonPath); + if (result < 0) + goto error; + if (result > 0) + break; + } } - *parent = owner->backingStore; - owner = owner->backingMeta; + *parent = chain->canonPath; + parentDir = chain->relDir; + chain = chain->backingMeta; } - if (!owner) + if (!chain) goto error; if (meta) - *meta = owner->backingMeta; - return owner->backingStore; + *meta = chain; + return chain->canonPath; error: virReportError(VIR_ERR_INVALID_ARG, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 73bcb0b..37f41bd 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -927,8 +927,7 @@ mymain(void) TEST_LOOKUP(19, abswrap, chain->canonPath, chain, NULL); TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, chain->canonPath); - TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta, - chain->canonPath); + TEST_LOOKUP(21, "qcow2", NULL, NULL, NULL); TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, chain->canonPath); TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore, -- 1.9.0
On 04/11/2014 12:21 AM, Eric Blake wrote:
Thanks to the testsuite, I feel quite confident that this rewrite still gives the same results for all cases except for one, and I can make the argument that _that_ case was a pre-existing bug. When looking up relative names, the lookup is supposed to be pegged to the directory that contains the parent qcow2 file.
* src/util/virstoragefile.c (virStorageFileChainLookup): Depend on new rather than old fields. * tests/virstoragetest.c (mymain): Adjust test to match fix.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 49 +++++++++++++++++++---------------------------- tests/virstoragetest.c | 3 +-- 2 files changed, 21 insertions(+), 31 deletions(-)
ACK Although I'll admit I don't know enough about all the backing chain code to fully understand the "pre-existing" bug comment. I suppose the only odd part I found was the comparison < VIR_STORAGE_TYPE_DIR - leaving currently DIR, NETWORK, and VOLUME out of the comparison. My thoughts went to what if something new comes along and what on earth was being compared beforehand... John
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dcde9e5..e33f6ef 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1544,48 +1544,39 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char **parent) { const char *start = chain->canonPath; - virStorageFileMetadataPtr owner; const char *tmp; + const char *parentDir = "."; + bool nameIsFile = virStorageIsFile(name);
if (!parent) parent = &tmp;
*parent = NULL; - if (name ? STREQ(start, name) || virFileLinkPointsTo(start, name) : - !chain->backingStore) { - if (meta) - *meta = chain; - return start; - } - - owner = chain; - *parent = start; - while (owner) { - if (!owner->backingStore) - goto error; + while (chain) { if (!name) { - if (!owner->backingMeta || - !owner->backingMeta->backingStore) + if (!chain->backingMeta) break; - } else if (STREQ_NULLABLE(name, owner->backingStoreRaw) || - STREQ(name, owner->backingStore)) { - break; - } else if (virStorageIsFile(owner->backingStore)) { - int result = virFileRelLinkPointsTo(owner->directory, name, - owner->backingStore); - if (result < 0) - goto error; - if (result > 0) + } else { + if (STREQ(name, chain->path)) break; + if (nameIsFile && chain->type < VIR_STORAGE_TYPE_DIR) { + int result = virFileRelLinkPointsTo(parentDir, name, + chain->canonPath); + if (result < 0) + goto error; + if (result > 0) + break; + } } - *parent = owner->backingStore; - owner = owner->backingMeta; + *parent = chain->canonPath; + parentDir = chain->relDir; + chain = chain->backingMeta; } - if (!owner) + if (!chain) goto error; if (meta) - *meta = owner->backingMeta; - return owner->backingStore; + *meta = chain; + return chain->canonPath;
error: virReportError(VIR_ERR_INVALID_ARG, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 73bcb0b..37f41bd 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -927,8 +927,7 @@ mymain(void) TEST_LOOKUP(19, abswrap, chain->canonPath, chain, NULL); TEST_LOOKUP(20, "../qcow2", chain->backingStore, chain->backingMeta, chain->canonPath); - TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta, - chain->canonPath); + TEST_LOOKUP(21, "qcow2", NULL, NULL, NULL); TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta, chain->canonPath); TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore,
On 04/11/2014 08:46 PM, John Ferlan wrote:
On 04/11/2014 12:21 AM, Eric Blake wrote:
Thanks to the testsuite, I feel quite confident that this rewrite still gives the same results for all cases except for one, and I can make the argument that _that_ case was a pre-existing bug. When looking up relative names, the lookup is supposed to be pegged to the directory that contains the parent qcow2 file.
* src/util/virstoragefile.c (virStorageFileChainLookup): Depend on new rather than old fields. * tests/virstoragetest.c (mymain): Adjust test to match fix.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 49 +++++++++++++++++++---------------------------- tests/virstoragetest.c | 3 +-- 2 files changed, 21 insertions(+), 31 deletions(-)
ACK
Although I'll admit I don't know enough about all the backing chain code to fully understand the "pre-existing" bug comment.
This is the comment I added earlier in the series explaining the pre-existing bug: /* FIXME: 21 is questionable, since there is no 'sub/qcow2' and * since relative backing files should be looked up in the context * of the directory containing the parent. */ (oops, I already removed the comment in 4/6, even though the bug wasn't addressed until 5/6). I'll describe it in more detail in the commit message.
I suppose the only odd part I found was the comparison < VIR_STORAGE_TYPE_DIR - leaving currently DIR, NETWORK, and VOLUME out of the comparison. My thoughts went to what if something new comes along and what on earth was being compared beforehand...
I think I'll rewrite that to use explicit comparison against VIR_STORAGE_TYPE_FILE and VIR_STORAGE_TYPE_BLOCK instead. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/11/2014 10:13 PM, Eric Blake wrote:
ACK
I suppose the only odd part I found was the comparison < VIR_STORAGE_TYPE_DIR - leaving currently DIR, NETWORK, and VOLUME out of the comparison. My thoughts went to what if something new comes along and what on earth was being compared beforehand...
I think I'll rewrite that to use explicit comparison against VIR_STORAGE_TYPE_FILE and VIR_STORAGE_TYPE_BLOCK instead.
Now pushed, with this squashed in. diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c index 275be3e..94dddbc 100644 --- i/src/util/virstoragefile.c +++ w/src/util/virstoragefile.c @@ -1563,7 +1563,8 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, } else { if (STREQ(name, chain->path)) break; - if (nameIsFile && chain->type < VIR_STORAGE_TYPE_DIR) { + if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || + chain->type == VIR_STORAGE_TYPE_BLOCK)) { int result = virFileRelLinkPointsTo(parentDir, name, chain->canonPath); if (result < 0)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Another field no longer needed, getting us one step closer to merging virStorageFileMetadata and virStorageSource. * src/util/virstoragefile.h (_virStorageFileMetadata): Drop field. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataFromFDInternal): Alter signature. (virStorageFileFreeMetadata, virStorageFileGetMetadataFromBuf) (virStorageFileGetMetadataFromFD): Adjust clients. * tests/virstoragetest.c (_testFileData, testStorageChain) (mymain): Simplify test. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 22 +++++++++++++--------- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 29 ++--------------------------- 3 files changed, 15 insertions(+), 37 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e33f6ef..0c03797 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -799,7 +799,8 @@ virStorageFileGetMetadataInternal(const char *path, int format, virStorageFileMetadataPtr meta, char **backingStore, - int *backingFormat) + int *backingFormat, + char **backingDirectory) { int ret = -1; @@ -877,7 +878,7 @@ virStorageFileGetMetadataInternal(const char *path, meta->backingStoreRaw = meta->backingStore; meta->backingStore = NULL; if (virFindBackingFile(directory, backing, - &meta->directory, + backingDirectory, &meta->backingStore) < 0) { /* the backing file is (currently) unavailable, treat this * file as standalone: @@ -1017,7 +1018,7 @@ virStorageFileGetMetadataFromBuf(const char *path, if (virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len, format, ret, backing, - backingFormat) < 0) { + backingFormat, NULL) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; } @@ -1036,7 +1037,8 @@ virStorageFileGetMetadataFromFDInternal(const char *path, int fd, int format, virStorageFileMetadataPtr meta, - int *backingFormat) + int *backingFormat, + char **backingDirectory) { char *buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; @@ -1079,7 +1081,7 @@ virStorageFileGetMetadataFromFDInternal(const char *path, ret = virStorageFileGetMetadataInternal(path, canonPath, directory, buf, len, format, meta, &meta->backingStoreRaw, - backingFormat); + backingFormat, backingDirectory); if (ret == 0) { if (S_ISREG(sb.st_mode)) @@ -1121,7 +1123,8 @@ virStorageFileGetMetadataFromFD(const char *path, if (VIR_ALLOC(ret) < 0) goto cleanup; if (virStorageFileGetMetadataFromFDInternal(path, canonPath, ".", - fd, format, ret, NULL) < 0) { + fd, format, ret, + NULL, NULL) < 0) { virStorageFileFreeMetadata(ret); ret = NULL; } @@ -1143,6 +1146,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, int fd; int ret = -1; int backingFormat; + char *backingDirectory = NULL; VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", path, canonPath, NULLSTR(directory), format, @@ -1166,7 +1170,8 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, directory, fd, format, meta, - &backingFormat); + &backingFormat, + &backingDirectory); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); @@ -1193,7 +1198,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath, if (VIR_ALLOC(backing) < 0 || virStorageFileGetMetadataRecurse(meta->backingStoreRaw, meta->backingStore, - meta->directory, backingFormat, + backingDirectory, backingFormat, uid, gid, allow_probe, cycle, backing) < 0) { /* If we failed to get backing data, mark the chain broken */ @@ -1332,7 +1337,6 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); VIR_FREE(meta->compat); - VIR_FREE(meta->directory); virBitmapFree(meta->features); virStorageEncryptionFree(meta->encryption); VIR_FREE(meta); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index caf1d2f..8294e62 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -146,7 +146,6 @@ struct _virStorageFileMetadata { * query the parent metadata for details about the backing * store. */ char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canonPath */ - char *directory; /* The directory containing basename of backingStoreRaw. Should be same as backingMeta->relDir */ }; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 37f41bd..bb5c173 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -233,8 +233,6 @@ struct _testFileData { const char *expBackingStore; const char *expBackingStoreRaw; - const char *expBackingDirRel; - const char *expBackingDirAbs; unsigned long long expCapacity; bool expEncrypted; const char *pathRel; @@ -311,7 +309,6 @@ testStorageChain(const void *args) while (elt) { char *expect = NULL; char *actual = NULL; - const char *expBackingDirectory; const char *expPath; const char *expRelDir; @@ -320,18 +317,15 @@ testStorageChain(const void *args) goto cleanup; } - expBackingDirectory = isAbs ? data->files[i]->expBackingDirAbs - : data->files[i]->expBackingDirRel; expPath = isAbs ? data->files[i]->pathAbs : data->files[i]->pathRel; expRelDir = isAbs ? data->files[i]->relDirAbs : data->files[i]->relDirRel; if (virAsprintf(&expect, - "store:%s\nraw:%s\ndirectory:%s\nother:%lld %d\n" + "store:%s\nraw:%s\nother:%lld %d\n" "path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), - NULLSTR(expBackingDirectory), data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(expPath), @@ -340,11 +334,10 @@ testStorageChain(const void *args) data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, - "store:%s\nraw:%s\ndirectory:%s\nother:%lld %d\n" + "store:%s\nraw:%s\nother:%lld %d\n" "path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n", NULLSTR(elt->backingStore), NULLSTR(elt->backingStoreRaw), - NULLSTR(elt->directory), elt->capacity, !!elt->encryption, NULLSTR(elt->path), NULLSTR(elt->canonPath), @@ -518,8 +511,6 @@ mymain(void) testFileData qcow2 = { .expBackingStore = canonraw, .expBackingStoreRaw = "raw", - .expBackingDirRel = ".", - .expBackingDirAbs = datadir, .expCapacity = 1024, .pathRel = "qcow2", .pathAbs = canonqcow2, @@ -556,7 +547,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = absraw; - qcow2.expBackingDirRel = datadir; raw.pathRel = absraw; raw.pathAbs = absraw; raw.relDirRel = datadir; @@ -577,8 +567,6 @@ mymain(void) testFileData wrap = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, - .expBackingDirRel = datadir, - .expBackingDirAbs = datadir, .expCapacity = 1024, .pathRel = "wrap", .pathAbs = abswrap, @@ -615,8 +603,6 @@ mymain(void) testFileData wrap_as_raw = { .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, - .expBackingDirRel = datadir, - .expBackingDirAbs = datadir, .expCapacity = 1024, .pathRel = "wrap", .pathAbs = abswrap, @@ -674,8 +660,6 @@ mymain(void) ret = -1; qcow2.expBackingStore = "nbd:example.org:6000"; qcow2.expBackingStoreRaw = "nbd:example.org:6000"; - qcow2.expBackingDirRel = NULL; - qcow2.expBackingDirAbs = NULL; /* Qcow2 file with backing protocol instead of file */ testFileData nbd = { @@ -695,8 +679,6 @@ mymain(void) testFileData qed = { .expBackingStore = canonraw, .expBackingStoreRaw = absraw, - .expBackingDirRel = datadir, - .expBackingDirAbs = datadir, .expCapacity = 1024, .pathRel = "qed", .pathAbs = absqed, @@ -759,8 +741,6 @@ mymain(void) testFileData link1 = { .expBackingStore = canonraw, .expBackingStoreRaw = "../raw", - .expBackingDirRel = "sub/../sub/..", - .expBackingDirAbs = datadir "/sub/../sub/..", .expCapacity = 1024, .pathRel = "../sub/link1", .pathAbs = "../sub/link1", @@ -773,8 +753,6 @@ mymain(void) testFileData link2 = { .expBackingStore = canonqcow2, .expBackingStoreRaw = "../sub/link1", - .expBackingDirRel = "sub/../sub", - .expBackingDirAbs = datadir "/sub/../sub", .expCapacity = 1024, .pathRel = "sub/link2", .pathAbs = abslink2, @@ -803,8 +781,6 @@ mymain(void) ret = -1; qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = "qcow2"; - qcow2.expBackingDirRel = "."; - qcow2.expBackingDirAbs = datadir; /* Behavior of an infinite loop chain */ TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -826,7 +802,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = "wrap"; - qcow2.expBackingDirRel = datadir; qcow2.pathRel = absqcow2; qcow2.relDirRel = datadir; -- 1.9.0
On 04/11/2014 12:21 AM, Eric Blake wrote:
Another field no longer needed, getting us one step closer to merging virStorageFileMetadata and virStorageSource.
* src/util/virstoragefile.h (_virStorageFileMetadata): Drop field. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataFromFDInternal): Alter signature. (virStorageFileFreeMetadata, virStorageFileGetMetadataFromBuf) (virStorageFileGetMetadataFromFD): Adjust clients. * tests/virstoragetest.c (_testFileData, testStorageChain) (mymain): Simplify test.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 22 +++++++++++++--------- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 29 ++--------------------------- 3 files changed, 15 insertions(+), 37 deletions(-)
ACK - John
On 04/11/2014 08:56 PM, John Ferlan wrote:
On 04/11/2014 12:21 AM, Eric Blake wrote:
Another field no longer needed, getting us one step closer to merging virStorageFileMetadata and virStorageSource.
* src/util/virstoragefile.h (_virStorageFileMetadata): Drop field. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal) (virStorageFileGetMetadataFromFDInternal): Alter signature. (virStorageFileFreeMetadata, virStorageFileGetMetadataFromBuf) (virStorageFileGetMetadataFromFD): Adjust clients. * tests/virstoragetest.c (_testFileData, testStorageChain) (mymain): Simplify test.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 22 +++++++++++++--------- src/util/virstoragefile.h | 1 - tests/virstoragetest.c | 29 ++--------------------------- 3 files changed, 15 insertions(+), 37 deletions(-)
ACK -
Thanks; pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
John Ferlan