tests: Fix VIR_TEST_REGENERATE_OUTPUT

Message ID 78053b9f554b2b172933f5e1fbf77a4073df0112.1460500437.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson April 12, 2016, 10:33 p.m.
commit 950a90d489 mocked some virCommand handling for the qemu tests,
but we were using that in the test suite to call test-wrap-argv.pl for
regenerating test output.

Switch the generator code to just use system() instead
---
 tests/testutils.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

-- 
2.7.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson April 13, 2016, 12:37 p.m. | #1
On 04/12/2016 08:54 PM, Laine Stump wrote:
> On 04/12/2016 06:33 PM, Cole Robinson wrote:

>> commit 950a90d489 mocked some virCommand handling for the qemu tests,

>> but we were using that in the test suite to call test-wrap-argv.pl for

>> regenerating test output.

>>

>> Switch the generator code to just use system() instead

>> ---

> 

> I dislike giving up the potential utility of virCommand*() in tests, but this

> does fix the problem (without creating the inefficiency of an extra function

> in the callstack for every virCommandRun() call by every libvirtd in the

> world). So ACK unless someone has a better idea / different opinion. (But

> there's one thing to fix, noted below).

> 

> (if this was in a binary run outside the build environment, I would NACK use

> of system(), but since it's only used by a test...)

> 


Agreed, I wouldn't advocate system() use in end user code. FWIW there's
already a usage of system() in testutils.c, though I suspect it predates
virCommand

> 

>>   tests/testutils.c | 19 +++++--------------

>>   1 file changed, 5 insertions(+), 14 deletions(-)

>>

>> diff --git a/tests/testutils.c b/tests/testutils.c

>> index a0ce4b6..4f3e67b 100644

>> --- a/tests/testutils.c

>> +++ b/tests/testutils.c

>> @@ -434,24 +434,15 @@ virTestRewrapFile(const char *filename)

>>   {

>>       int ret = -1;

>>       char *outbuf = NULL;

> 

> outbuf is no longer used, so it should be removed.

> 

>> -    char *script = NULL;

>> -    virCommandPtr cmd = NULL;

>> +    char *cmd = NULL;

>>   -    if (virAsprintf(&script, "%s/test-wrap-argv.pl", abs_srcdir) < 0)

>> +    if (virAsprintf(&cmd, "echo \"$(%s/test-wrap-argv.pl %s)\" > %s",

>> +                    abs_srcdir, filename, filename) < 0)

>>           goto cleanup;

> 

> For that matter, cmd would be NULL if virAsprintf failed, so all that's really

> needed here is a "return -1;", meaning that the cleanup label can also be

> removed.

> 


Yes that simplifies things a lot. Made the changes locally, I'll give it a day
and see if anyone else comments, if not I'll push.

Thanks,
Cole

>>   -    cmd = virCommandNewArgList(script, filename, NULL);

>> -    virCommandSetOutputBuffer(cmd, &outbuf);

>> -    if (virCommandRun(cmd, NULL) < 0)

>> -        goto cleanup;

>> -

>> -    if (virFileWriteStr(filename, outbuf, 0666) < 0)

>> -        goto cleanup;

>> -

>> -    ret = 0;

>> +    ret = system(cmd);

>>    cleanup:

>> -    VIR_FREE(script);

>> -    virCommandFree(cmd);

>> +    VIR_FREE(cmd);

>>       VIR_FREE(outbuf);

>>       return ret;

>>   }

> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/tests/testutils.c b/tests/testutils.c
index a0ce4b6..4f3e67b 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -434,24 +434,15 @@  virTestRewrapFile(const char *filename)
 {
     int ret = -1;
     char *outbuf = NULL;
-    char *script = NULL;
-    virCommandPtr cmd = NULL;
+    char *cmd = NULL;
 
-    if (virAsprintf(&script, "%s/test-wrap-argv.pl", abs_srcdir) < 0)
+    if (virAsprintf(&cmd, "echo \"$(%s/test-wrap-argv.pl %s)\" > %s",
+                    abs_srcdir, filename, filename) < 0)
         goto cleanup;
 
-    cmd = virCommandNewArgList(script, filename, NULL);
-    virCommandSetOutputBuffer(cmd, &outbuf);
-    if (virCommandRun(cmd, NULL) < 0)
-        goto cleanup;
-
-    if (virFileWriteStr(filename, outbuf, 0666) < 0)
-        goto cleanup;
-
-    ret = 0;
+    ret = system(cmd);
  cleanup:
-    VIR_FREE(script);
-    virCommandFree(cmd);
+    VIR_FREE(cmd);
     VIR_FREE(outbuf);
     return ret;
 }