Re: [libvirt] [PATCH 4/6] conf: drop redundant parameter to chain lookup

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(a)redhat.com&gt; --- 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,