5: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(-)
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;
}
--
2.7.3
7:54 p.m.
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...)
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.
- 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;
}
6:24 a.m.
On Tue, Apr 12, 2016 at 08:54:59PM -0400, 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).
I proposed moving the offending caller to virnetdev.c and mocking it
instead of virCommand run in:
cover.1460536848.git.jtomko(a)redhat.com
[libvirt] [PATCH 0/2] Fix VIR_TEST_REGENERATE_OUTPUT in tests
Jan
7:37 a.m.
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;
> }
3147
days inactive
3148
days old
3 comments
3 participants
participants (3)
-
Cole Robinson -
Ján Tomko -
Laine Stump