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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list