Re: [PATCH 1/6] conf: test backing chain lookup

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

 




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@xxxxxxxxxx>
> ---
>  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

--
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]