[libvirt] [PATCH] tests: Use system() for test-wrap-argv.pl
virCommand doesn't buy us anything here, and this simplifies the code. There's another usage of system() in testutils.c already FWIW, though it predates virCommand --- Had this sitting in a branch from an earlier discussion... I figure system() is safe in the test suite but I'm not going to argue if someone feels strongly the other way tests/testutils.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index 79d0763..7664b06 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -432,28 +432,14 @@ virtTestCaptureProgramOutput(const char *const argv[] ATTRIBUTE_UNUSED, static int virTestRewrapFile(const char *filename) { - int ret = -1; - char *outbuf = NULL; - char *script = NULL; - virCommandPtr cmd = NULL; - - if (virAsprintf(&script, "%s/test-wrap-argv.pl", abs_srcdir) < 0) - goto cleanup; - - cmd = virCommandNewArgList(script, filename, NULL); - virCommandSetOutputBuffer(cmd, &outbuf); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + char *cmd; - if (virFileWriteStr(filename, outbuf, 0666) < 0) - goto cleanup; + /* The 'echo' syntax lets us read and write the file in one shot */ + if (virAsprintf(&cmd, "echo \"$(%s/test-wrap-argv.pl %s)\" > %s", + abs_srcdir, filename, filename) < 0) + return -1; - ret = 0; - cleanup: - VIR_FREE(script); - virCommandFree(cmd); - VIR_FREE(outbuf); - return ret; + return system(cmd); } /** -- 2.7.3
On Wed, Apr 20, 2016 at 01:33:00PM -0400, Cole Robinson wrote:
virCommand doesn't buy us anything here, and this simplifies the code. There's another usage of system() in testutils.c already FWIW, though it predates virCommand --- Had this sitting in a branch from an earlier discussion... I figure system() is safe in the test suite but I'm not going to argue if someone feels strongly the other way
I have no preference in this, but it needed to be changed due to some mocking problem or something IIRC, didn't it? Is that dealt with already? Anyway, ACK if nobody else is against it.
tests/testutils.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c index 79d0763..7664b06 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -432,28 +432,14 @@ virtTestCaptureProgramOutput(const char *const argv[] ATTRIBUTE_UNUSED, static int virTestRewrapFile(const char *filename) { - int ret = -1; - char *outbuf = NULL; - char *script = NULL; - virCommandPtr cmd = NULL; - - if (virAsprintf(&script, "%s/test-wrap-argv.pl", abs_srcdir) < 0) - goto cleanup; - - cmd = virCommandNewArgList(script, filename, NULL); - virCommandSetOutputBuffer(cmd, &outbuf); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + char *cmd;
- if (virFileWriteStr(filename, outbuf, 0666) < 0) - goto cleanup; + /* The 'echo' syntax lets us read and write the file in one shot */ + if (virAsprintf(&cmd, "echo \"$(%s/test-wrap-argv.pl %s)\" > %s", + abs_srcdir, filename, filename) < 0) + return -1;
- ret = 0; - cleanup: - VIR_FREE(script); - virCommandFree(cmd); - VIR_FREE(outbuf); - return ret; + return system(cmd); }
/** -- 2.7.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Apr 20, 2016 at 01:33:00PM -0400, Cole Robinson wrote:
virCommand doesn't buy us anything here, and this simplifies the code. There's another usage of system() in testutils.c already FWIW, though it predates virCommand --- Had this sitting in a branch from an earlier discussion... I figure system() is safe in the test suite but I'm not going to argue if someone feels strongly the other way
tests/testutils.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c index 79d0763..7664b06 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -432,28 +432,14 @@ virtTestCaptureProgramOutput(const char *const argv[] ATTRIBUTE_UNUSED, static int virTestRewrapFile(const char *filename) { - int ret = -1; - char *outbuf = NULL; - char *script = NULL; - virCommandPtr cmd = NULL; - - if (virAsprintf(&script, "%s/test-wrap-argv.pl", abs_srcdir) < 0) - goto cleanup; - - cmd = virCommandNewArgList(script, filename, NULL); - virCommandSetOutputBuffer(cmd, &outbuf); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + char *cmd;
- if (virFileWriteStr(filename, outbuf, 0666) < 0) - goto cleanup; + /* The 'echo' syntax lets us read and write the file in one shot */ + if (virAsprintf(&cmd, "echo \"$(%s/test-wrap-argv.pl %s)\" > %s", + abs_srcdir, filename, filename) < 0)
Eww, Jan
On Thu, Apr 21, 2016 at 09:24:41 +0200, Ján Tomko wrote:
On Wed, Apr 20, 2016 at 01:33:00PM -0400, Cole Robinson wrote:
virCommand doesn't buy us anything here, and this simplifies the code. There's another usage of system() in testutils.c already FWIW, though it predates virCommand --- Had this sitting in a branch from an earlier discussion... I figure system() is safe in the test suite but I'm not going to argue if someone feels strongly the other way
tests/testutils.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c index 79d0763..7664b06 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -432,28 +432,14 @@ virtTestCaptureProgramOutput(const char *const argv[] ATTRIBUTE_UNUSED, static int virTestRewrapFile(const char *filename) { - int ret = -1; - char *outbuf = NULL; - char *script = NULL; - virCommandPtr cmd = NULL; - - if (virAsprintf(&script, "%s/test-wrap-argv.pl", abs_srcdir) < 0) - goto cleanup; - - cmd = virCommandNewArgList(script, filename, NULL); - virCommandSetOutputBuffer(cmd, &outbuf); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + char *cmd;
- if (virFileWriteStr(filename, outbuf, 0666) < 0) - goto cleanup; + /* The 'echo' syntax lets us read and write the file in one shot */ + if (virAsprintf(&cmd, "echo \"$(%s/test-wrap-argv.pl %s)\" > %s", + abs_srcdir, filename, filename) < 0)
Eww,
I concur. NACK
Dear Cole, Cole Robinson <crobinso@redhat.com> writes: [tests/testutils.c:virTestRewrapFile()]
+ /* The 'echo' syntax lets us read and write the file in one shot */ + if (virAsprintf(&cmd, "echo \"$(%s/test-wrap-argv.pl %s)\" > %s", + abs_srcdir, filename, filename) < 0) + return -1; [...] + return system(cmd);
Doesn't the shell truncate the file before test-wrap-argv.pl gets a chance to read the original content? Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
On 04/21/2016 04:04 AM, Sascha Silbe wrote:
Dear Cole,
Cole Robinson <crobinso@redhat.com> writes:
[tests/testutils.c:virTestRewrapFile()]
+ /* The 'echo' syntax lets us read and write the file in one shot */ + if (virAsprintf(&cmd, "echo \"$(%s/test-wrap-argv.pl %s)\" > %s", + abs_srcdir, filename, filename) < 0) + return -1; [...] + return system(cmd);
Doesn't the shell truncate the file before test-wrap-argv.pl gets a chance to read the original content?
That's what the echo syntax works around; the test-wrap-argv command is fully run before the output file is truncated But I've dropped this patch anyways - Cole
participants (5)
-
Cole Robinson -
Ján Tomko -
Martin Kletzander -
Peter Krempa -
Sascha Silbe