docs/internals/command.html.in | 33 +++++- src/libvirt_private.syms | 4 +- src/qemu/qemu_process.c | 8 +- src/util/vircommand.c | 151 ++++++++++++---------------- src/util/vircommand.h | 18 +++- src/util/vircommandpriv.h | 19 ++-- tests/networkxml2firewalltest.c | 7 +- tests/nwfilterebiptablestest.c | 32 +++--- tests/nwfilterxml2firewalltest.c | 6 +- tests/testutils.c | 210 +++++++++++++++++++++++++++++++++++++++ tests/testutils.h | 2 + tests/virfirewalltest.c | 71 ++++++------- tests/viriscsitest.c | 120 ++++++++++++++++------ tests/virkmodtest.c | 10 +- tests/virnetdevbandwidthtest.c | 6 +- 15 files changed, 482 insertions(+), 215 deletions(-)
Since dry-run of cmd is just for tests now, it would be better to remove
dry-run code and move them to testutils. Then the code flow in virCommand*
could be more general. There are 3 steps in this patch:
1. Introduce a new global hook (of virExecHook type) which will be called
in code flow just before the cmd->hook. The global hook is also called in
child process. If it returns 1, the child process will exit with status
in advance and the parent will process io and wait for the child normally.
It prepares for registering dry-run and anything else.
The virCommandSetPreExecHook is modified for registering both types of hooks.
2. Implement virTestSetDryRun with dry-run code in "testutils.c".
It substitutes for virCommandSetDryRun. The virTestSetDryRun invokes
virCommandSetPreExecHook to register a function on the global hook which
will fill in cmdline buffer and call callback for tests.
3. Remove all dryrun code in "vircommand.c" and remove virCommandSetDryRun API.
Diffs from old dry-run:
The new global hook is called in child process. So dryrun-callback for tests
should write to stdout/stderr when they need output something.
Now the opaque argument for dryrun-callback cannot be inout. In "tests/viriscsitest.c",
iface_created in opaque of callback is an inout argument. There's a bit
complicated to transfer it between the child and its parent. So this patch use
a temporary file to do that.
Signed-off-by: Shi Lei <shi_lei@massclouds.com>
---
docs/internals/command.html.in | 33 +++++-
src/libvirt_private.syms | 4 +-
src/qemu/qemu_process.c | 8 +-
src/util/vircommand.c | 151 ++++++++++++----------------
src/util/vircommand.h | 18 +++-
src/util/vircommandpriv.h | 19 ++--
tests/networkxml2firewalltest.c | 7 +-
tests/nwfilterebiptablestest.c | 32 +++---
tests/nwfilterxml2firewalltest.c | 6 +-
tests/testutils.c | 210 +++++++++++++++++++++++++++++++++++++++
tests/testutils.h | 2 +
tests/virfirewalltest.c | 71 ++++++-------
tests/viriscsitest.c | 120 ++++++++++++++++------
tests/virkmodtest.c | 10 +-
tests/virnetdevbandwidthtest.c | 6 +-
15 files changed, 482 insertions(+), 215 deletions(-)
diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
index 43f51a4..44478f0 100644
--- a/docs/internals/command.html.in
+++ b/docs/internals/command.html.in
@@ -398,17 +398,40 @@ virCommandSetWorkingDirectory(cmd, LOCALSTATEDIR);
<h3><a id="hooks">Any additional hooks</a></h3>
<p>
- If anything else is needed, it is possible to request a hook
- function that is called in the child after the fork, as the
+ If anything else is needed, it is possible to request hook
+ functions that are called in the child after the fork, as the
last thing before changing directories, dropping capabilities,
- and executing the new process. If hook(opaque) returns
- non-zero, then the child process will not be run.
+ and executing the new process.
+
+ There are two types of hooks:
+ <ul>
+ <li>Global hook takes effect to all command.
+ Setting global hook if passing NULL to @cmd.
+ </li>
+ <li>Command hook takes effect only to the command.
+ Passing the pointer of the command to @cmd.
+ </li>
+ </ul>
+
+ If hook(opaque) returns -1, then the child process will not be
+ run for error. And if it returns 1, the child process will exit
+ with status in advance and this mode is used for command dryrun.
+
+ Since @hook runs in the child, it should be careful to avoid
+ any functions that are not async-signal-safe.
+
+ For global hook, Passing NULL for @cmd, @hook and @opaque to
+ cancel the effect.
</p>
<pre>
-virCommandSetPreExecHook(cmd, hook, opaque);
+virCommandSetPreExecHook(NULL, hook, opaque); /* global hook */
+virCommandSetPreExecHook(cmd, hook, opaque); /* command hook */
+
+virCommandSetPreExecHook(NULL, NULL, NULL); /* cancel global hook */
</pre>
+
<h3><a id="logging">Logging commands</a></h3>
<p>
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 47ea35f..f4aca6f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1602,7 +1602,10 @@ virCommandDaemonize;
virCommandDoAsyncIO;
virCommandExec;
virCommandFree;
+virCommandGetArgs;
+virCommandGetEnv;
virCommandGetGID;
+virCommandGetInputBuffer;
virCommandGetUID;
virCommandHandshakeNotify;
virCommandHandshakeWait;
@@ -1621,7 +1624,6 @@ virCommandRunAsync;
virCommandRunNul;
virCommandRunRegex;
virCommandSetAppArmorProfile;
-virCommandSetDryRun;
virCommandSetErrorBuffer;
virCommandSetErrorFD;
virCommandSetGID;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ab74938..f7a5dbd 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2656,7 +2656,9 @@ qemuProcessKillManagedPRDaemon(virDomainObjPtr vm)
static int
-qemuProcessStartPRDaemonHook(void *opaque)
+qemuProcessStartPRDaemonHook(void *opaque,
+ virCommandPtr ptr ATTRIBUTE_UNUSED,
+ int *status ATTRIBUTE_UNUSED)
{
virDomainObjPtr vm = opaque;
size_t i, nfds = 0;
@@ -2972,7 +2974,9 @@ struct qemuProcessHookData {
virQEMUDriverConfigPtr cfg;
};
-static int qemuProcessHook(void *data)
+static int qemuProcessHook(void *data,
+ virCommandPtr ptr ATTRIBUTE_UNUSED,
+ int *status ATTRIBUTE_UNUSED)
{
struct qemuProcessHookData *h = data;
qemuDomainObjPrivatePtr priv = h->vm->privateData;
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 8be3fdf..53aee7a 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -138,11 +138,9 @@ struct _virCommand {
int mask;
};
-/* See virCommandSetDryRun for description for this variable */
-static virBufferPtr dryRunBuffer;
-static virCommandDryRunCallback dryRunCallback;
-static void *dryRunOpaque;
-static int dryRunStatus;
+/* See virCommandSetPreExecHook for these variable */
+static virExecHook preExecHook;
+static void *preExecOpaque;
/*
* virCommandFDIsSet:
@@ -717,12 +715,32 @@ virExec(virCommandPtr cmd)
virProcessSetMaxCoreSize(0, cmd->maxCore) < 0)
goto fork_error;
+ if (preExecHook) {
+ int status = 0;
+ ret = preExecHook(preExecOpaque, cmd, &status);
+ if (ret < 0)
+ goto fork_error;
+
+ if (ret == 1) {
+ /* Child skips followed steps and exits with status in advance. */
+ ret = status;
+ goto fork_error;
+ }
+ }
+
if (cmd->hook) {
+ int status = 0;
VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque);
- ret = cmd->hook(cmd->opaque);
+ ret = cmd->hook(cmd->opaque, cmd, &status);
VIR_DEBUG("Done hook %d", ret);
if (ret < 0)
goto fork_error;
+
+ if (ret == 1) {
+ /* Child skips followed steps and exits with status in advance. */
+ ret = status;
+ goto fork_error;
+ }
}
# if defined(WITH_SECDRIVER_SELINUX)
@@ -1733,6 +1751,13 @@ virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf)
}
+const char *
+virCommandGetInputBuffer(virCommandPtr cmd)
+{
+ return (const char *) cmd->inbuf;
+}
+
+
/**
* virCommandSetOutputBuffer:
* @cmd: the command to modify
@@ -1879,7 +1904,9 @@ virCommandSetErrorFD(virCommandPtr cmd, int *errfd)
/**
* virCommandSetPreExecHook:
- * @cmd: the command to modify
+ * @cmd: the command pointer or NULL.
+ * NonNull, means that it registers a hook for the command
+ * Null, means that it registers the global PreExecHook
* @hook: the hook to run
* @opaque: argument to pass to the hook
*
@@ -1887,13 +1914,30 @@ virCommandSetErrorFD(virCommandPtr cmd, int *errfd)
* directories, dropping capabilities, and executing the new process.
* Force the child to fail if HOOK does not return zero.
*
+ * There are two types of hooks:
+ * 1. Global hook take effect to all command.
+ * Setting global hook if passing NULL to @cmd.
+ * 2. Command hook take effect only to the command.
+ * Passing the pointer of the command to @cmd.
+ *
* Since @hook runs in the child, it should be careful to avoid
* any functions that are not async-signal-safe.
+
+ * For global hook, Passing NULL for @cmd, @hook and @opaque to cancel
+ * the effect.
*/
void
virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque)
{
- if (!cmd || cmd->has_error)
+ if (!cmd) {
+ /* Global hook */
+ preExecHook = hook;
+ preExecOpaque = opaque;
+ return;
+ }
+
+ /* Command hook */
+ if (cmd->has_error)
return;
if (cmd->hook) {
@@ -2017,11 +2061,6 @@ virCommandProcessIO(virCommandPtr cmd)
size_t inoff = 0;
int ret = 0;
- if (dryRunBuffer || dryRunCallback) {
- VIR_DEBUG("Dry run requested, skipping I/O processing");
- return 0;
- }
-
/* With an input buffer, feed data to child
* via pipe */
if (cmd->inbuf)
@@ -2439,31 +2478,6 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
goto cleanup;
}
- str = virCommandToString(cmd);
- if (dryRunBuffer || dryRunCallback) {
- dryRunStatus = 0;
- if (!str) {
- /* error already reported by virCommandToString */
- goto cleanup;
- }
-
- if (dryRunBuffer) {
- VIR_DEBUG("Dry run requested, appending stringified "
- "command to dryRunBuffer=%p", dryRunBuffer);
- virBufferAdd(dryRunBuffer, str, -1);
- virBufferAddChar(dryRunBuffer, '\n');
- }
- if (dryRunCallback) {
- dryRunCallback((const char *const*)cmd->args,
- (const char *const*)cmd->env,
- cmd->inbuf, cmd->outbuf, cmd->errbuf,
- &dryRunStatus, dryRunOpaque);
- }
- ret = 0;
- goto cleanup;
- }
-
- VIR_DEBUG("About to run %s", str ? str : cmd->args[0]);
ret = virExec(cmd);
VIR_DEBUG("Command result %d, with PID %d",
ret, (int)cmd->pid);
@@ -2537,16 +2551,6 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
return -1;
}
- if (dryRunBuffer || dryRunCallback) {
- VIR_DEBUG("Dry run requested, returning status %d",
- dryRunStatus);
- if (exitstatus)
- *exitstatus = dryRunStatus;
- else if (dryRunStatus)
- return -1;
- return 0;
- }
-
if (cmd->pid == -1) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("command is not yet running"));
@@ -2878,48 +2882,21 @@ virCommandDoAsyncIO(virCommandPtr cmd)
cmd->flags |= VIR_EXEC_ASYNC_IO | VIR_EXEC_NONBLOCK;
}
-/**
- * virCommandSetDryRun:
- * @buf: buffer to store stringified commands
- * @callback: callback to process input/output/args
- *
- * Sometimes it's desired to not actually run given command, but
- * see its string representation without having to change the
- * callee. Unit testing serves as a great example. In such cases,
- * the callee constructs the command and calls it via
- * virCommandRun* API. The virCommandSetDryRun allows you to
- * modify this behavior: once called, every call to
- * virCommandRun* results in command string representation being
- * appended to @buf instead of being executed. If @callback is
- * provided, then it is invoked with the argv, env and stdin
- * data string for the command. It is expected to fill the stdout
- * and stderr data strings and exit status variables.
- *
- * The strings stored in @buf are escaped for a shell and
- * separated by a newline. For example:
- *
- * virBuffer buffer = VIR_BUFFER_INITIALIZER;
- * virCommandSetDryRun(&buffer);
- *
- * virCommandPtr echocmd = virCommandNewArgList("/bin/echo", "Hello world", NULL);
- * virCommandRun(echocmd, NULL);
- *
- * After this, the @buffer should contain:
- *
- * /bin/echo 'Hello world'\n
- *
- * To cancel this effect pass NULL for @buf and @callback.
- */
-void
-virCommandSetDryRun(virBufferPtr buf,
- virCommandDryRunCallback cb,
- void *opaque)
+
+const char *const*
+virCommandGetArgs(virCommandPtr cmd)
+{
+ return (const char *const*) cmd->args;
+}
+
+
+const char *const*
+virCommandGetEnv(virCommandPtr cmd)
{
- dryRunBuffer = buf;
- dryRunCallback = cb;
- dryRunOpaque = opaque;
+ return (const char *const*) cmd->env;
}
+
#ifndef WIN32
/**
* virCommandRunRegex:
diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index 90bcc6c..9f322d8 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -29,10 +29,22 @@
typedef struct _virCommand virCommand;
typedef virCommand *virCommandPtr;
-/* This will execute in the context of the first child
+/*
+ * virExecHook:
+ * @data: opaque for this hook
+ * @cmd: pointer to virCommand
+ * @status: get status for exit status of the child,
+ * valid only if return code is 1.
+ *
+ * This will execute in the context of the first child
* after fork() but before execve(). As such, it is unsafe to
- * call any function that is not async-signal-safe. */
-typedef int (*virExecHook)(void *data);
+ * call any function that is not async-signal-safe.
+ *
+ * Returns: 0 on success,
+ * -1 on usage error (errno is ENOMEM on OOM),
+ * 1 to notify child exit with status from child in advance
+ */
+typedef int (*virExecHook)(void *data, virCommandPtr cmd, int *status);
pid_t virFork(void) ATTRIBUTE_RETURN_CHECK;
diff --git a/src/util/vircommandpriv.h b/src/util/vircommandpriv.h
index 8930299..f2b6063 100644
--- a/src/util/vircommandpriv.h
+++ b/src/util/vircommandpriv.h
@@ -20,7 +20,7 @@
*/
#ifndef __VIR_COMMAND_PRIV_H_ALLOW__
-# error "vircommandpriv.h may only be included by vircommand.c or test suites"
+# error "vircommandpriv.h may only be included by vircommand.c or testutils.c"
#endif
#ifndef __VIR_COMMAND_PRIV_H__
@@ -28,16 +28,13 @@
# include "vircommand.h"
-typedef void (*virCommandDryRunCallback)(const char *const*args,
- const char *const*env,
- const char *input,
- char **output,
- char **error,
- int *status,
- void *opaque);
+const char *const*
+virCommandGetArgs(virCommandPtr cmd);
-void virCommandSetDryRun(virBufferPtr buf,
- virCommandDryRunCallback cb,
- void *opaque);
+const char *const*
+virCommandGetEnv(virCommandPtr cmd);
+
+const char *
+virCommandGetInputBuffer(virCommandPtr cmd);
#endif /* __VIR_COMMAND_PRIV_H__ */
diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
index 242b645..196c0b1 100644
--- a/tests/networkxml2firewalltest.c
+++ b/tests/networkxml2firewalltest.c
@@ -31,9 +31,6 @@
# define __VIR_FIREWALL_PRIV_H_ALLOW__
# include "virfirewallpriv.h"
-# define __VIR_COMMAND_PRIV_H_ALLOW__
-# include "vircommandpriv.h"
-
# define VIR_FROM_THIS VIR_FROM_NONE
static const char *abs_top_srcdir;
@@ -53,7 +50,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
virNetworkDefPtr def = NULL;
int ret = -1;
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (!(def = virNetworkDefParseFile(xml)))
goto cleanup;
@@ -66,7 +63,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
actualargv = virBufferContentAndReset(&buf);
virTestClearCommandPath(actualargv);
- virCommandSetDryRun(NULL, NULL, NULL);
if (virTestCompareToFile(actualargv, cmdline) < 0)
goto cleanup;
@@ -74,6 +70,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
ret = 0;
cleanup:
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(expectargv);
VIR_FREE(actualargv);
diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c
index 6073423..186349c 100644
--- a/tests/nwfilterebiptablestest.c
+++ b/tests/nwfilterebiptablestest.c
@@ -29,12 +29,8 @@
#define __VIR_FIREWALL_PRIV_H_ALLOW__
#include "virfirewallpriv.h"
-#define __VIR_COMMAND_PRIV_H_ALLOW__
-#include "vircommandpriv.h"
-
#define VIR_FROM_THIS VIR_FROM_NONE
-
#define VIR_NWFILTER_NEW_RULES_TEARDOWN \
"iptables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FP-vnet0\n" \
"iptables -D libvirt-out -m physdev --physdev-out vnet0 -g FP-vnet0\n" \
@@ -104,7 +100,7 @@ testNWFilterEBIPTablesAllTeardown(const void *opaque ATTRIBUTE_UNUSED)
char *actual = NULL;
int ret = -1;
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (ebiptables_driver.allTeardown("vnet0") < 0)
goto cleanup;
@@ -122,7 +118,7 @@ testNWFilterEBIPTablesAllTeardown(const void *opaque ATTRIBUTE_UNUSED)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(actual);
return ret;
@@ -175,7 +171,7 @@ testNWFilterEBIPTablesTearOldRules(const void *opaque ATTRIBUTE_UNUSED)
char *actual = NULL;
int ret = -1;
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (ebiptables_driver.tearOldRules("vnet0") < 0)
goto cleanup;
@@ -193,7 +189,7 @@ testNWFilterEBIPTablesTearOldRules(const void *opaque ATTRIBUTE_UNUSED)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(actual);
return ret;
@@ -224,7 +220,7 @@ testNWFilterEBIPTablesRemoveBasicRules(const void *opaque ATTRIBUTE_UNUSED)
char *actual = NULL;
int ret = -1;
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (ebiptables_driver.removeBasicRules("vnet0") < 0)
goto cleanup;
@@ -242,7 +238,7 @@ testNWFilterEBIPTablesRemoveBasicRules(const void *opaque ATTRIBUTE_UNUSED)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(actual);
return ret;
@@ -258,7 +254,7 @@ testNWFilterEBIPTablesTearNewRules(const void *opaque ATTRIBUTE_UNUSED)
char *actual = NULL;
int ret = -1;
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (ebiptables_driver.tearNewRules("vnet0") < 0)
goto cleanup;
@@ -276,7 +272,7 @@ testNWFilterEBIPTablesTearNewRules(const void *opaque ATTRIBUTE_UNUSED)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(actual);
return ret;
@@ -330,7 +326,7 @@ testNWFilterEBIPTablesApplyBasicRules(const void *opaque ATTRIBUTE_UNUSED)
int ret = -1;
virMacAddr mac = { .addr = { 0x10, 0x20, 0x30, 0x40, 0x50, 0x60 } };
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (ebiptables_driver.applyBasicRules("vnet0", &mac) < 0)
goto cleanup;
@@ -348,7 +344,7 @@ testNWFilterEBIPTablesApplyBasicRules(const void *opaque ATTRIBUTE_UNUSED)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(actual);
return ret;
@@ -420,7 +416,7 @@ testNWFilterEBIPTablesApplyDHCPOnlyRules(const void *opaque ATTRIBUTE_UNUSED)
}
};
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (ebiptables_driver.applyDHCPOnlyRules("vnet0", &mac, &val, false) < 0)
goto cleanup;
@@ -438,7 +434,7 @@ testNWFilterEBIPTablesApplyDHCPOnlyRules(const void *opaque ATTRIBUTE_UNUSED)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(actual);
return ret;
@@ -493,7 +489,7 @@ testNWFilterEBIPTablesApplyDropAllRules(const void *opaque ATTRIBUTE_UNUSED)
char *actual = NULL;
int ret = -1;
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (ebiptables_driver.applyDropAllRules("vnet0") < 0)
goto cleanup;
@@ -511,7 +507,7 @@ testNWFilterEBIPTablesApplyDropAllRules(const void *opaque ATTRIBUTE_UNUSED)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(actual);
return ret;
diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c
index 043b7d1..6ae0ef7 100644
--- a/tests/nwfilterxml2firewalltest.c
+++ b/tests/nwfilterxml2firewalltest.c
@@ -30,8 +30,6 @@
# define __VIR_FIREWALL_PRIV_H_ALLOW__
# include "virfirewallpriv.h"
-# define __VIR_COMMAND_PRIV_H_ALLOW__
-# include "vircommandpriv.h"
# define VIR_FROM_THIS VIR_FROM_NONE
@@ -380,7 +378,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
memset(&inst, 0, sizeof(inst));
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (!vars)
goto cleanup;
@@ -401,7 +399,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
actualargv = virBufferContentAndReset(&buf);
virTestClearCommandPath(actualargv);
- virCommandSetDryRun(NULL, NULL, NULL);
testRemoveCommonRules(actualargv);
@@ -411,6 +408,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
ret = 0;
cleanup:
+ virTestSetDryRun(NULL, NULL, NULL);
virBufferFreeAndReset(&buf);
VIR_FREE(actualargv);
virNWFilterInstReset(&inst);
diff --git a/tests/testutils.c b/tests/testutils.c
index ab938c1..01d9edc 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -48,6 +48,9 @@
#include "virprocess.h"
#include "virstring.h"
+#define __VIR_COMMAND_PRIV_H_ALLOW__
+#include "vircommandpriv.h"
+
#ifdef TEST_OOM
# ifdef TEST_OOM_TRACE
# include <dlfcn.h>
@@ -1414,3 +1417,210 @@ const char
return virtTestCounterStr;
}
+
+
+#define VIR_TEST_DRYRUN_PIPE "/tmp/vir_test_pipe"
+
+/* According to old virCommandDryRunCallback */
+typedef int (*dryRunCallback)(const char *const*args,
+ const char *const*env,
+ const char *input,
+ const void *opaque);
+
+typedef struct _dryRunHookData dryRunHookData;
+typedef dryRunHookData *dryRunHookDataPtr;
+
+struct _dryRunHookData
+{
+ dryRunCallback callback;
+ void *opaque;
+ virBufferPtr buffer;
+ virThreadPtr thread;
+ int readfd;
+};
+
+static dryRunHookData hookdata = { NULL, NULL, NULL, NULL, -1 };
+
+
+/* of virExecHook type */
+static int
+dryRunHookForTest(void *opaque, virCommandPtr cmd, int *status)
+{
+ dryRunHookDataPtr data = opaque;
+
+ virThreadCancel(data->thread);
+ virThreadJoin(data->thread);
+ VIR_FREE(data->thread);
+ if (data->readfd != -1)
+ VIR_FORCE_CLOSE(data->readfd);
+
+ if (!status)
+ return -1;
+
+ *status = 0;
+
+ if (data->buffer) {
+ int fd;
+ int len = 0;
+ int pos = 0;
+ VIR_AUTOFREE(char *) str = NULL;
+ str = virCommandToString(cmd);
+ if (!str)
+ return -1;
+
+ len = strlen(str);
+ if (len == 0)
+ return -1;
+
+ if ((fd = open(VIR_TEST_DRYRUN_PIPE, O_WRONLY)) < 0)
+ return -1;
+
+ while (len > 0) {
+ int size = len > PIPE_BUF ? PIPE_BUF : len;
+ if (safewrite(fd, str + pos, size) != size) {
+ VIR_FORCE_CLOSE(fd);
+ return -1;
+ }
+ pos += size;
+ len -= size;
+ }
+
+ if (safewrite(fd, "\n", 1) != 1) {
+ VIR_FORCE_CLOSE(fd);
+ return -1;
+ }
+
+ VIR_FORCE_CLOSE(fd);
+ }
+
+ if (data->callback) {
+ *status = data->callback(virCommandGetArgs(cmd),
+ virCommandGetEnv(cmd),
+ virCommandGetInputBuffer(cmd),
+ data->opaque);
+ }
+
+ /* For dry-run:
+ * It returns 1 to exit from child in advance */
+ return 1;
+}
+
+
+static void
+dryRunWaitHelper(void *opaque)
+{
+ int *fd = (int *) opaque;
+ while (true) {
+ if ((*fd = open(VIR_TEST_DRYRUN_PIPE, O_RDONLY)) < 0) {
+ fprintf(stderr, "cannot open dryrun pipe for read\n");
+ return;
+ }
+
+ while (true) {
+ int ret;
+ char str[PIPE_BUF];
+ memset(str, 0, sizeof(str));
+ ret = read(*fd, str, PIPE_BUF);
+ if (ret < 0) {
+ fprintf(stderr, "read dryrun pipe error: %d\n", errno);
+ VIR_FORCE_CLOSE(*fd);
+ return;
+ }
+
+ if (ret == 0)
+ break;
+
+ virBufferAdd(hookdata.buffer, str, ret);
+ }
+
+ VIR_FORCE_CLOSE(*fd);
+ }
+
+ /* Never reach here */
+}
+
+
+/**
+ * virTestSetDryRun: (substitute for old virCommandSetDryRun)
+ * @buf: buffer to store stringified commands
+ * @callback: callback to process input/output/args
+ * @opaque: argument passed to callback
+ *
+ * In unit testing, it's desired to not actually run given command,
+ * but see its string representation without having to change the
+ * callee. The callee constructs the command and calls it via
+ * virCommandRun* API. The virTestSetDryRun allows you to
+ * modify this behavior: once called, every call to
+ * virCommandRun* results in command string representation being
+ * appended to @buf instead of being executed. If @callback is
+ * provided, then it is invoked with the argv, env and stdin
+ * data string for the command. It is expected to fill the stdout
+ * and stderr data strings and exit status variables.
+ *
+ * The strings stored in @buf are escaped for a shell and
+ * separated by a newline. For example:
+ *
+ * virBuffer buffer = VIR_BUFFER_INITIALIZER;
+ * virTestSetDryRun(&buffer, NULL, NULL);
+ *
+ * virCommandPtr echocmd = virCommandNewArgList("/bin/echo", "Hello world", NULL);
+ * virCommandRun(echocmd, NULL);
+ *
+ * After this, the @buffer should contain:
+ *
+ * /bin/echo 'Hello world'\n
+ *
+ * To cancel this effect pass NULL for @buf, @callback and @opaque.
+ * virTestSetDryRun(NULL, NULL, NULL);
+ *
+ */
+void
+virTestSetDryRun(virBufferPtr buf, void *callback, void *opaque)
+{
+ if (!buf && !callback && !opaque) {
+ /* Passing NULL for @buf, @callback and @opaque to cancel dry-run */
+ if (hookdata.thread) {
+ virThreadCancel(hookdata.thread);
+ virThreadJoin(hookdata.thread);
+ VIR_FREE(hookdata.thread);
+ }
+
+ hookdata.buffer = NULL;
+ hookdata.callback = NULL;
+ hookdata.opaque = NULL;
+ if (hookdata.readfd != -1)
+ VIR_FORCE_CLOSE(hookdata.readfd);
+
+ virCommandSetPreExecHook(NULL, NULL, NULL);
+ unlink(VIR_TEST_DRYRUN_PIPE);
+ return;
+ }
+
+ if (hookdata.buffer || hookdata.callback || hookdata.opaque || hookdata.thread) {
+ fprintf(stderr, "cannot set dryrun twice buf(%p) cb(%p) opaque(%p) thread(%p)\n",
+ hookdata.buffer, hookdata.callback, hookdata.opaque, hookdata.thread);
+ return;
+ }
+
+ if (hookdata.readfd != -1)
+ VIR_FORCE_CLOSE(hookdata.readfd);
+
+ if (!virFileExists(VIR_TEST_DRYRUN_PIPE)) {
+ if (mkfifo(VIR_TEST_DRYRUN_PIPE, 0600) < 0) {
+ fprintf(stderr, "cannot create dryrun pipe. err: %d\n", errno);
+ return;
+ }
+ }
+
+ if (VIR_ALLOC(hookdata.thread) < 0 ||
+ virThreadCreate(hookdata.thread, true, dryRunWaitHelper, &hookdata.readfd) < 0) {
+ fprintf(stderr, "cannot create thread for dryrun. err: %d\n", errno);
+ VIR_FREE(hookdata.thread);
+ return;
+ }
+
+ hookdata.buffer = buf;
+ hookdata.callback = callback;
+ hookdata.opaque = opaque;
+ virCommandSetPreExecHook(NULL, dryRunHookForTest, &hookdata);
+}
diff --git a/tests/testutils.h b/tests/testutils.h
index 3bd7bf1..82bfa0a 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -167,4 +167,6 @@ int testCompareDomXML2XMLFiles(virCapsPtr caps,
unsigned int parseFlags,
testCompareDomXML2XMLResult expectResult);
+void virTestSetDryRun(virBufferPtr buf, void *callback, void *opaque);
+
#endif /* __VIR_TEST_UTILS_H__ */
diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index e6d6874..1bf41c7 100644
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -21,14 +21,12 @@
#include <config.h>
#define __VIR_FIREWALL_PRIV_H_ALLOW__
-#define __VIR_COMMAND_PRIV_H_ALLOW__
#include "testutils.h"
#if defined(__linux__)
# include "virbuffer.h"
-# include "vircommandpriv.h"
# include "virfirewallpriv.h"
# include "virmock.h"
# include "virdbuspriv.h"
@@ -213,7 +211,7 @@ testFirewallSingleGroup(const void *opaque)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT)
- virCommandSetDryRun(&cmdbuf, NULL, NULL);
+ virTestSetDryRun(&cmdbuf, NULL, NULL);
else
fwBuf = &cmdbuf;
@@ -249,7 +247,7 @@ testFirewallSingleGroup(const void *opaque)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
@@ -273,7 +271,7 @@ testFirewallRemoveRule(const void *opaque)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT)
- virCommandSetDryRun(&cmdbuf, NULL, NULL);
+ virTestSetDryRun(&cmdbuf, NULL, NULL);
else
fwBuf = &cmdbuf;
@@ -315,7 +313,7 @@ testFirewallRemoveRule(const void *opaque)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
@@ -340,7 +338,7 @@ testFirewallManyGroups(const void *opaque ATTRIBUTE_UNUSED)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT)
- virCommandSetDryRun(&cmdbuf, NULL, NULL);
+ virTestSetDryRun(&cmdbuf, NULL, NULL);
else
fwBuf = &cmdbuf;
@@ -388,19 +386,16 @@ testFirewallManyGroups(const void *opaque ATTRIBUTE_UNUSED)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
-static void
+static int
testFirewallRollbackHook(const char *const*args,
const char *const*env ATTRIBUTE_UNUSED,
const char *input ATTRIBUTE_UNUSED,
- char **output ATTRIBUTE_UNUSED,
- char **error ATTRIBUTE_UNUSED,
- int *status,
- void *opaque ATTRIBUTE_UNUSED)
+ const void *opaque ATTRIBUTE_UNUSED)
{
bool isAdd = false;
while (*args) {
@@ -408,11 +403,11 @@ testFirewallRollbackHook(const char *const*args,
if (STREQ(*args, "-A")) {
isAdd = true;
} else if (isAdd && STREQ(*args, "192.168.122.255")) {
- *status = 127;
- break;
+ return 127;
}
args++;
}
+ return 0;
}
static int
@@ -434,7 +429,7 @@ testFirewallIgnoreFailGroup(const void *opaque ATTRIBUTE_UNUSED)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT) {
- virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
+ virTestSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
} else {
fwBuf = &cmdbuf;
fwError = true;
@@ -484,7 +479,7 @@ testFirewallIgnoreFailGroup(const void *opaque ATTRIBUTE_UNUSED)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
@@ -509,7 +504,7 @@ testFirewallIgnoreFailRule(const void *opaque ATTRIBUTE_UNUSED)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT) {
- virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
+ virTestSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
} else {
fwBuf = &cmdbuf;
fwError = true;
@@ -558,7 +553,7 @@ testFirewallIgnoreFailRule(const void *opaque ATTRIBUTE_UNUSED)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
@@ -581,7 +576,7 @@ testFirewallNoRollback(const void *opaque ATTRIBUTE_UNUSED)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT) {
- virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
+ virTestSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
} else {
fwBuf = &cmdbuf;
fwError = true;
@@ -629,7 +624,7 @@ testFirewallNoRollback(const void *opaque ATTRIBUTE_UNUSED)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
@@ -654,7 +649,7 @@ testFirewallSingleRollback(const void *opaque ATTRIBUTE_UNUSED)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT) {
- virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
+ virTestSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
} else {
fwError = true;
fwBuf = &cmdbuf;
@@ -719,7 +714,7 @@ testFirewallSingleRollback(const void *opaque ATTRIBUTE_UNUSED)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
@@ -743,7 +738,7 @@ testFirewallManyRollback(const void *opaque ATTRIBUTE_UNUSED)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT) {
- virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
+ virTestSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
} else {
fwBuf = &cmdbuf;
fwError = true;
@@ -812,7 +807,7 @@ testFirewallManyRollback(const void *opaque ATTRIBUTE_UNUSED)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
@@ -840,7 +835,7 @@ testFirewallChainedRollback(const void *opaque ATTRIBUTE_UNUSED)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT) {
- virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
+ virTestSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
} else {
fwBuf = &cmdbuf;
fwError = true;
@@ -935,7 +930,7 @@ testFirewallChainedRollback(const void *opaque ATTRIBUTE_UNUSED)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
@@ -967,26 +962,26 @@ static const char *expectedLines[] = {
static size_t expectedLineNum;
static bool expectedLineError;
-static void
+static int
testFirewallQueryHook(const char *const*args,
const char *const*env ATTRIBUTE_UNUSED,
const char *input ATTRIBUTE_UNUSED,
- char **output,
- char **error ATTRIBUTE_UNUSED,
- int *status,
- void *opaque ATTRIBUTE_UNUSED)
+ const void *opaque ATTRIBUTE_UNUSED)
{
if (STREQ(args[0], IPTABLES_PATH) &&
STREQ(args[1], "-L")) {
- if (VIR_STRDUP(*output, TEST_FILTER_TABLE_LIST) < 0)
- *status = 127;
+ if (safewrite(STDOUT_FILENO, TEST_FILTER_TABLE_LIST,
+ strlen(TEST_FILTER_TABLE_LIST)) < 0)
+ return 127;
} else if (STREQ(args[0], IPTABLES_PATH) &&
STREQ(args[1], "-t") &&
STREQ(args[2], "nat") &&
STREQ(args[3], "-L")) {
- if (VIR_STRDUP(*output, TEST_NAT_TABLE_LIST) < 0)
- *status = 127;
+ if (safewrite(STDOUT_FILENO, TEST_NAT_TABLE_LIST,
+ strlen(TEST_NAT_TABLE_LIST)) < 0)
+ return 127;
}
+ return 0;
}
@@ -1044,7 +1039,7 @@ testFirewallQuery(const void *opaque ATTRIBUTE_UNUSED)
goto cleanup;
if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT) {
- virCommandSetDryRun(&cmdbuf, testFirewallQueryHook, NULL);
+ virTestSetDryRun(&cmdbuf, testFirewallQueryHook, NULL);
} else {
fwBuf = &cmdbuf;
fwError = true;
@@ -1118,7 +1113,7 @@ testFirewallQuery(const void *opaque ATTRIBUTE_UNUSED)
cleanup:
virBufferFreeAndReset(&cmdbuf);
fwBuf = NULL;
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virFirewallFree(fw);
return ret;
}
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
index 4fd7ff6..11a2a54 100644
--- a/tests/viriscsitest.c
+++ b/tests/viriscsitest.c
@@ -19,6 +19,7 @@
*/
#include <config.h>
+#include <fcntl.h>
#include "testutils.h"
@@ -29,9 +30,6 @@ main(void)
return EXIT_AM_SKIP;
}
#else
-# define __VIR_COMMAND_PRIV_H_ALLOW__
-
-# include "vircommandpriv.h"
# include "viriscsi.h"
# define VIR_FROM_THIS VIR_FROM_NONE
@@ -72,27 +70,40 @@ const char *iscsiadmIfaceIfaceOutput =
struct testIscsiadmCbData {
bool output_version;
- bool iface_created;
+ char *result_path;
};
-static void testIscsiadmCb(const char *const*args,
- const char *const*env ATTRIBUTE_UNUSED,
- const char *input ATTRIBUTE_UNUSED,
- char **output,
- char **error ATTRIBUTE_UNUSED,
- int *status,
- void *opaque)
+static int testIscsiadmCb(const char *const*args,
+ const char *const*env ATTRIBUTE_UNUSED,
+ const char *input ATTRIBUTE_UNUSED,
+ const void *opaque)
{
- struct testIscsiadmCbData *data = opaque;
+ int fd;
+ int iface_created = 0;
+ const struct testIscsiadmCbData *data = opaque;
+
+ if (data) {
+ fd = open(data->result_path, O_RDONLY);
+ if (fd < 0)
+ return -1;
+
+ if (read(fd, &iface_created, sizeof(int)) != sizeof(int)) {
+ VIR_FORCE_CLOSE(fd);
+ return -1;
+ }
+ VIR_FORCE_CLOSE(fd);
+ }
if (args[0] && STREQ(args[0], ISCSIADM) &&
args[1] && STREQ(args[1], "--mode") &&
args[2] && STREQ(args[2], "session") &&
args[3] == NULL) {
if (data->output_version)
- ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutputNonFlash));
+ ignore_value(safewrite(STDOUT_FILENO, iscsiadmSessionOutputNonFlash,
+ strlen(iscsiadmSessionOutputNonFlash)));
else
- ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutput));
+ ignore_value(safewrite(STDOUT_FILENO, iscsiadmSessionOutput,
+ strlen(iscsiadmSessionOutput)));
} else if (args[0] && STREQ(args[0], ISCSIADM) &&
args[1] && STREQ(args[1], "--mode") &&
args[2] && STREQ(args[2], "discovery") &&
@@ -103,7 +114,8 @@ static void testIscsiadmCb(const char *const*args,
args[7] && STREQ(args[7], "--op") &&
args[8] && STREQ(args[8], "nonpersistent") &&
args[9] == NULL) {
- ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
+ ignore_value(safewrite(STDOUT_FILENO, iscsiadmSendtargetsOutput,
+ strlen(iscsiadmSendtargetsOutput)));
} else if (args[0] && STREQ(args[0], ISCSIADM) &&
args[1] && STREQ(args[1], "--mode") &&
args[2] && STREQ(args[2], "node") &&
@@ -127,10 +139,12 @@ static void testIscsiadmCb(const char *const*args,
args[1] && STREQ(args[1], "--mode") &&
args[2] && STREQ(args[2], "iface") &&
args[3] == NULL) {
- if (data->iface_created)
- ignore_value(VIR_STRDUP(*output, iscsiadmIfaceIfaceOutput));
+ if (iface_created)
+ ignore_value(safewrite(STDOUT_FILENO, iscsiadmIfaceIfaceOutput,
+ strlen(iscsiadmIfaceIfaceOutput)));
else
- ignore_value(VIR_STRDUP(*output, iscsiadmIfaceDefaultOutput));
+ ignore_value(safewrite(STDOUT_FILENO, iscsiadmIfaceDefaultOutput,
+ strlen(iscsiadmIfaceDefaultOutput)));
} else if (args[0] && STREQ(args[0], ISCSIADM) &&
args[1] && STREQ(args[1], "--mode") &&
args[2] && STREQ(args[2], "iface") &&
@@ -144,7 +158,18 @@ static void testIscsiadmCb(const char *const*args,
*
* New interface libvirt-iface-03020100 added
*/
- data->iface_created = true;
+ if (data) {
+ iface_created = 1;
+ fd = open(data->result_path, O_WRONLY);
+ if (fd < 0)
+ return -1;
+
+ if (safewrite(fd, &iface_created, sizeof(int)) != sizeof(int)) {
+ VIR_FORCE_CLOSE(fd);
+ return -1;
+ }
+ VIR_FORCE_CLOSE(fd);
+ }
} else if (args[0] && STREQ(args[0], ISCSIADM) &&
args[1] && STREQ(args[1], "--mode") &&
args[2] && STREQ(args[2], "iface") &&
@@ -157,7 +182,7 @@ static void testIscsiadmCb(const char *const*args,
args[9] && STREQ(args[9], "--value") &&
args[10] && STREQ(args[10], "iqn.2004-06.example:example1:initiator") &&
args[11] == NULL &&
- data->iface_created) {
+ iface_created) {
/* Mocking real environment output is not needed for now.
* Example output from real environment:
*
@@ -173,8 +198,9 @@ static void testIscsiadmCb(const char *const*args,
args[7] && STREQ(args[7], "--interface") &&
args[8] && STREQ(args[8], "libvirt-iface-03020100") &&
args[9] == NULL &&
- data->iface_created) {
- ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
+ iface_created) {
+ ignore_value(safewrite(STDOUT_FILENO, iscsiadmSendtargetsOutput,
+ strlen(iscsiadmSendtargetsOutput)));
} else if (args[0] && STREQ(args[0], ISCSIADM) &&
args[1] && STREQ(args[1], "--mode") &&
args[2] && STREQ(args[2], "node") &&
@@ -186,7 +212,7 @@ static void testIscsiadmCb(const char *const*args,
args[8] && STREQ(args[8], "--interface") &&
args[9] && STREQ(args[9], "libvirt-iface-03020100") &&
args[10] == NULL &&
- data->iface_created) {
+ iface_created) {
/* Mocking real environment output is not needed for now.
* Example output from real environment:
*
@@ -198,10 +224,31 @@ static void testIscsiadmCb(const char *const*args,
* portal: 10.20.30.40:3260,1] successful.
*/
} else {
- *status = -1;
+ return -1;
}
+ return 0;
}
+
+static int
+testCreateTempFile(char *path)
+{
+ int iface_created = 0;
+ int fd = -1;
+ if ((fd = mkostemp(path, O_RDWR)) < 0)
+ return -1;
+
+ if (safewrite(fd, &iface_created, sizeof(int)) != sizeof(int)) {
+ VIR_FORCE_CLOSE(fd);
+ unlink(path);
+ return -1;
+ }
+
+ VIR_FORCE_CLOSE(fd);
+ return 0;
+}
+
+
struct testSessionInfo {
const char *device_path;
bool output_version;
@@ -215,10 +262,15 @@ testISCSIGetSession(const void *data)
struct testIscsiadmCbData cbData = { 0 };
char *actual_session = NULL;
int ret = -1;
+ char path[] = "/dev/shm/testiscsi.XXXXXX";
+ if (testCreateTempFile(path) < 0)
+ return -1;
+
+ cbData.result_path = path;
cbData.output_version = info->output_version;
- virCommandSetDryRun(NULL, testIscsiadmCb, &cbData);
+ virTestSetDryRun(NULL, testIscsiadmCb, &cbData);
actual_session = virISCSIGetSession(info->device_path, true);
@@ -233,7 +285,8 @@ testISCSIGetSession(const void *data)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
+ unlink(path);
VIR_FREE(actual_session);
return ret;
}
@@ -254,7 +307,7 @@ testISCSIScanTargets(const void *data)
int ret = -1;
size_t i;
- virCommandSetDryRun(NULL, testIscsiadmCb, NULL);
+ virTestSetDryRun(NULL, testIscsiadmCb, NULL);
if (virISCSIScanTargets(info->portal, NULL,
false, &ntargets, &targets) < 0)
@@ -279,7 +332,7 @@ testISCSIScanTargets(const void *data)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
for (i = 0; i < ntargets; i++)
VIR_FREE(targets[i]);
VIR_FREE(targets);
@@ -300,15 +353,21 @@ testISCSIConnectionLogin(const void *data)
const struct testConnectionInfoLogin *info = data;
struct testIscsiadmCbData cbData = { 0 };
int ret = -1;
+ char path[] = "/dev/shm/testiscsi.XXXXXX";
+
+ if (testCreateTempFile(path) < 0)
+ return -1;
- virCommandSetDryRun(NULL, testIscsiadmCb, &cbData);
+ cbData.result_path = path;
+ virTestSetDryRun(NULL, testIscsiadmCb, &cbData);
if (virISCSIConnectionLogin(info->portal, info->initiatoriqn, info->target) < 0)
goto cleanup;
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
+ unlink(path);
return ret;
}
@@ -362,7 +421,6 @@ mymain(void)
DO_LOGIN_TEST("10.20.30.40:3260,1", NULL, "iqn.2004-06.example:example1:iscsi.test");
DO_LOGIN_TEST("10.20.30.40:3260,1", "iqn.2004-06.example:example1:initiator",
"iqn.2004-06.example:example1:iscsi.test");
-
if (rv < 0)
return EXIT_FAILURE;
return EXIT_SUCCESS;
diff --git a/tests/virkmodtest.c b/tests/virkmodtest.c
index b153e07..408e338 100644
--- a/tests/virkmodtest.c
+++ b/tests/virkmodtest.c
@@ -23,8 +23,6 @@
#ifdef __linux__
# include <stdlib.h>
-# define __VIR_COMMAND_PRIV_H_ALLOW__
-# include "vircommandpriv.h"
# include "virkmod.h"
# include "virstring.h"
@@ -96,7 +94,7 @@ testKModLoad(const void *args)
bool useBlacklist = info->useBlacklist;
virBuffer buf = VIR_BUFFER_INITIALIZER;
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
errbuf = virKModLoad(module, useBlacklist);
if (errbuf) {
@@ -110,7 +108,7 @@ testKModLoad(const void *args)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
VIR_FREE(errbuf);
return ret;
}
@@ -125,7 +123,7 @@ testKModUnload(const void *args)
const char *module = info->module;
virBuffer buf = VIR_BUFFER_INITIALIZER;
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
errbuf = virKModUnload(module);
if (errbuf) {
@@ -139,7 +137,7 @@ testKModUnload(const void *args)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
VIR_FREE(errbuf);
return ret;
}
diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
index 72bfeff..62cb044 100644
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -21,8 +21,6 @@
#include <config.h>
#include "testutils.h"
-#define __VIR_COMMAND_PRIV_H_ALLOW__
-#include "vircommandpriv.h"
#include "virnetdevbandwidth.h"
#include "netdev_bandwidth_conf.c"
@@ -79,7 +77,7 @@ testVirNetDevBandwidthSet(const void *data)
if (!iface)
iface = "eth0";
- virCommandSetDryRun(&buf, NULL, NULL);
+ virTestSetDryRun(&buf, NULL, NULL);
if (virNetDevBandwidthSet(iface, band, info->hierarchical_class, true) < 0)
goto cleanup;
@@ -103,7 +101,7 @@ testVirNetDevBandwidthSet(const void *data)
ret = 0;
cleanup:
- virCommandSetDryRun(NULL, NULL, NULL);
+ virTestSetDryRun(NULL, NULL, NULL);
virNetDevBandwidthFree(band);
virBufferFreeAndReset(&buf);
VIR_FREE(actual_cmd);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Aug 22, 2018 at 11:37:34AM +0800, Shi Lei wrote:
> Since dry-run of cmd is just for tests now, it would be better to remove
> dry-run code and move them to testutils. Then the code flow in virCommand*
> could be more general. There are 3 steps in this patch:
> 1. Introduce a new global hook (of virExecHook type) which will be called
> in code flow just before the cmd->hook. The global hook is also called in
> child process. If it returns 1, the child process will exit with status
> in advance and the parent will process io and wait for the child normally.
> It prepares for registering dry-run and anything else.
> The virCommandSetPreExecHook is modified for registering both types of hooks.
> 2. Implement virTestSetDryRun with dry-run code in "testutils.c".
> It substitutes for virCommandSetDryRun. The virTestSetDryRun invokes
> virCommandSetPreExecHook to register a function on the global hook which
> will fill in cmdline buffer and call callback for tests.
> 3. Remove all dryrun code in "vircommand.c" and remove virCommandSetDryRun API.
>
> Diffs from old dry-run:
> The new global hook is called in child process. So dryrun-callback for tests
> should write to stdout/stderr when they need output something.
>
> Now the opaque argument for dryrun-callback cannot be inout. In "tests/viriscsitest.c",
> iface_created in opaque of callback is an inout argument. There's a bit
> complicated to transfer it between the child and its parent. So this patch use
> a temporary file to do that.
>
> Signed-off-by: Shi Lei <shi_lei@massclouds.com>
> ---
> docs/internals/command.html.in | 33 +++++-
> src/libvirt_private.syms | 4 +-
> src/qemu/qemu_process.c | 8 +-
> src/util/vircommand.c | 151 ++++++++++++----------------
> src/util/vircommand.h | 18 +++-
> src/util/vircommandpriv.h | 19 ++--
> tests/networkxml2firewalltest.c | 7 +-
> tests/nwfilterebiptablestest.c | 32 +++---
> tests/nwfilterxml2firewalltest.c | 6 +-
> tests/testutils.c | 210 +++++++++++++++++++++++++++++++++++++++
> tests/testutils.h | 2 +
> tests/virfirewalltest.c | 71 ++++++-------
> tests/viriscsitest.c | 120 ++++++++++++++++------
> tests/virkmodtest.c | 10 +-
> tests/virnetdevbandwidthtest.c | 6 +-
> 15 files changed, 482 insertions(+), 215 deletions(-)
>
> diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
> index 43f51a4..44478f0 100644
> --- a/docs/internals/command.html.in
> +++ b/docs/internals/command.html.in
> @@ -398,17 +398,40 @@ virCommandSetWorkingDirectory(cmd, LOCALSTATEDIR);
> <h3><a id="hooks">Any additional hooks</a></h3>
>
> <p>
> - If anything else is needed, it is possible to request a hook
> - function that is called in the child after the fork, as the
> + If anything else is needed, it is possible to request hook
> + functions that are called in the child after the fork, as the
> last thing before changing directories, dropping capabilities,
> - and executing the new process. If hook(opaque) returns
> - non-zero, then the child process will not be run.
> + and executing the new process.
> +
> + There are two types of hooks:
> + <ul>
> + <li>Global hook takes effect to all command.
> + Setting global hook if passing NULL to @cmd.
> + </li>
> + <li>Command hook takes effect only to the command.
> + Passing the pointer of the command to @cmd.
> + </li>
> + </ul>
> +
> + If hook(opaque) returns -1, then the child process will not be
> + run for error. And if it returns 1, the child process will exit
> + with status in advance and this mode is used for command dryrun.
> +
> + Since @hook runs in the child, it should be careful to avoid
> + any functions that are not async-signal-safe.
> +
> + For global hook, Passing NULL for @cmd, @hook and @opaque to
> + cancel the effect.
> </p>
>
> <pre>
> -virCommandSetPreExecHook(cmd, hook, opaque);
> +virCommandSetPreExecHook(NULL, hook, opaque); /* global hook */
> +virCommandSetPreExecHook(cmd, hook, opaque); /* command hook */
> +
> +virCommandSetPreExecHook(NULL, NULL, NULL); /* cancel global hook */
> </pre>
[snip]
> void
> virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque)
> {
> - if (!cmd || cmd->has_error)
> + if (!cmd) {
> + /* Global hook */
> + preExecHook = hook;
> + preExecOpaque = opaque;
> + return;
> + }
I don't think this is an improvement.
With the virCommandSetDryRun() approach there is no way that the dry run
code can be accidentally triggered in production scenarios, as we can be
sure nothing will accidentally call virCommandSetDryRun().
Changing the semantics of virCommandSetPreExecHook() so that it sets a
global hook when 'cmd' is NULL introduces significant risk. The virCommand
APIs are designed to fail-safe in face of memory exhaustion or errors from
the caller. IOW passing a NULL for 'cmd' is an expected scenario in a
production environment, and this change breaks handling of that.
Personally I don't think the stated problem needs solving at all. The
virCommandSetDryRun() works reliably and doesn't need rewriting IMHO.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.