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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list