8:46 p.m.
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