Re: [PATCH] tests: Fix VIR_TEST_REGENERATE_OUTPUT

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

 



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



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