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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]