| Message ID | 78053b9f554b2b172933f5e1fbf77a4073df0112.1460500437.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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; }