[libvirt] [PATCH v3 0/2] Enhance virnetdevbandwidthtest
yet another version - this time with virBuffer approach. Michal Privoznik (2): virCommand: Introduce virCommandSetDryRun virnetdevbandwidthtest: Introduce testVirNetDevBandwidthSet src/libvirt_private.syms | 1 + src/util/vircommand.c | 57 +++++++++++++++++++++++++++++-- src/util/vircommand.h | 2 ++ tests/virnetdevbandwidthtest.c | 76 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 2 deletions(-) -- 1.8.5.2
There are some units within libvirt that utilize virCommand API to run some commands and deserve own unit testing. These units are, however, not desired to be rewritten to dig virCommand API usage out. As a great example virNetDevBandwidth could be used. The problem with the bandwidth unit is: it uses virComamnd API heavily. Therefore we need a mechanism to not really run a command, but rather see its string representation after which we can decide if the unit construct the correct sequence of commands or not. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++-- src/util/vircommand.h | 2 ++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..7655247 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1096,6 +1096,7 @@ virCommandRequireHandshake; virCommandRun; virCommandRunAsync; virCommandSetAppArmorProfile; +virCommandSetDryRun; virCommandSetErrorBuffer; virCommandSetErrorFD; virCommandSetGID; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index a52a1ab..b337962 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -129,6 +129,9 @@ struct _virCommand { #endif }; +/* See virCommandSetDryRun for description on this variable */ +static virBufferPtr dryRunBuffer; + /* * virCommandFDIsSet: * @fd: FD to test @@ -2199,7 +2202,7 @@ int virCommandRunAsync(virCommandPtr cmd, pid_t *pid) { int ret = -1; - char *str; + char *str = NULL; size_t i; bool synchronous = false; int infd[2] = {-1, -1}; @@ -2263,7 +2266,17 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) str = virCommandToString(cmd); VIR_DEBUG("About to run %s", str ? str : cmd->args[0]); - VIR_FREE(str); + if (dryRunBuffer) { + /* Dry-run requested. Just append @str to @dryRunBuffer + * and claim success. */ + + VIR_DEBUG("Dry run requested, appending stringified " + "command to dryRunBuffer=%p", dryRunBuffer); + virBufferAdd(dryRunBuffer, str ? str : cmd->args[0], -1); + virBufferAddChar(dryRunBuffer, '\n'); + ret = 0; + goto cleanup; + } ret = virExec(cmd); VIR_DEBUG("Command result %d, with PID %d", @@ -2303,6 +2316,7 @@ cleanup: VIR_FORCE_CLOSE(cmd->infd); VIR_FORCE_CLOSE(cmd->inpipe); } + VIR_FREE(str); return ret; } @@ -2334,6 +2348,14 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) return -1; } + if (dryRunBuffer) { + /* Dry-run requested. Claim success. */ + VIR_DEBUG("Dry run requested, claiming success"); + if (exitstatus) + *exitstatus = 0; + return 0; + } + if (cmd->pid == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("command is not yet running")); @@ -2669,3 +2691,34 @@ virCommandDoAsyncIO(virCommandPtr cmd) cmd->flags |= VIR_EXEC_ASYNC_IO | VIR_EXEC_NONBLOCK; } + +/** + * virCommandSetDryRun: + * @buf: buffer to store stringified commands + * + * 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. For example: + * + * virBuffer buffer = VIR_BUFFER_INITIALIZER; + * virCommandSetDryRun(&buffer); + * + * const char *const echocmd[] = {"/bin/echo", "Hello world"} + * virCommandRun(echocmd, NULL); + * + * After this, the @buffer should contain: + * + * /bin/echo Hello world + * + * To cancel this effect pass NULL. + */ +void +virCommandSetDryRun(virBufferPtr buf) +{ + dryRunBuffer = buf; +} diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e977f93..a743200 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -184,4 +184,6 @@ void virCommandAbort(virCommandPtr cmd); void virCommandFree(virCommandPtr cmd); void virCommandDoAsyncIO(virCommandPtr cmd); + +void virCommandSetDryRun(virBufferPtr buf); #endif /* __VIR_COMMAND_H__ */ -- 1.8.5.2
On 01/28/2014 07:37 PM, Michal Privoznik wrote:
There are some units within libvirt that utilize virCommand API to run some commands and deserve own unit testing. These units are, however, not desired to be rewritten to dig virCommand API usage out. As a great example virNetDevBandwidth could be used. The problem with the bandwidth unit is: it uses virComamnd API heavily. Therefore we need a mechanism
s/virComamnd/virCommand/
to not really run a command, but rather see its string representation after which we can decide if the unit construct the correct sequence of commands or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++-- src/util/vircommand.h | 2 ++ 3 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..7655247 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1096,6 +1096,7 @@ virCommandRequireHandshake; virCommandRun; virCommandRunAsync; virCommandSetAppArmorProfile; +virCommandSetDryRun; virCommandSetErrorBuffer; virCommandSetErrorFD; virCommandSetGID; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index a52a1ab..b337962 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -129,6 +129,9 @@ struct _virCommand { #endif };
+/* See virCommandSetDryRun for description on this variable */
s/on/of/
+static virBufferPtr dryRunBuffer; + /* * virCommandFDIsSet: * @fd: FD to test @@ -2199,7 +2202,7 @@ int virCommandRunAsync(virCommandPtr cmd, pid_t *pid) { int ret = -1; - char *str; + char *str = NULL; size_t i; bool synchronous = false; int infd[2] = {-1, -1}; @@ -2263,7 +2266,17 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
str = virCommandToString(cmd); VIR_DEBUG("About to run %s", str ? str : cmd->args[0]); - VIR_FREE(str); + if (dryRunBuffer) { + /* Dry-run requested. Just append @str to @dryRunBuffer + * and claim success. */ +
This comment seems redundant.
+ VIR_DEBUG("Dry run requested, appending stringified " + "command to dryRunBuffer=%p", dryRunBuffer); + virBufferAdd(dryRunBuffer, str ? str : cmd->args[0], -1);
str can only be NULL on OOM or if the virCommand API was misused. If we're only trying to print a debug message, an OOM error is not so fatal, but I think it should be on a dry run - converting the command to string is what it's supposed to do.
+ virBufferAddChar(dryRunBuffer, '\n'); + ret = 0; + goto cleanup; + }
ret = virExec(cmd); VIR_DEBUG("Command result %d, with PID %d", @@ -2303,6 +2316,7 @@ cleanup: VIR_FORCE_CLOSE(cmd->infd); VIR_FORCE_CLOSE(cmd->inpipe); } + VIR_FREE(str); return ret; }
@@ -2334,6 +2348,14 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) return -1; }
+ if (dryRunBuffer) { + /* Dry-run requested. Claim success. */ + VIR_DEBUG("Dry run requested, claiming success"); + if (exitstatus) + *exitstatus = 0;
Either remove the comment above or add another one here for more hilarity: /* Success successfully claimed */
+ return 0; + } + if (cmd->pid == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("command is not yet running")); @@ -2669,3 +2691,34 @@ virCommandDoAsyncIO(virCommandPtr cmd)
cmd->flags |= VIR_EXEC_ASYNC_IO | VIR_EXEC_NONBLOCK; } + +/** + * virCommandSetDryRun: + * @buf: buffer to store stringified commands + * + * 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. For example:
It would be nice to mention that the strings are escaped for a shell and separated by a newline.
+ * + * virBuffer buffer = VIR_BUFFER_INITIALIZER; + * virCommandSetDryRun(&buffer); + * + * const char *const echocmd[] = {"/bin/echo", "Hello world"}
You should use a working example, like: virCommandPtr echocmd = virCommandNewArgList("/bin/echo", "Hello world", NULL);
+ * virCommandRun(echocmd, NULL); + * + * After this, the @buffer should contain: + * + * /bin/echo Hello world
in that case, it will contain: "/bin/echo 'Hello world'\n".
+ * + * To cancel this effect pass NULL. + */ +void +virCommandSetDryRun(virBufferPtr buf) +{ + dryRunBuffer = buf; +} diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e977f93..a743200 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -184,4 +184,6 @@ void virCommandAbort(virCommandPtr cmd); void virCommandFree(virCommandPtr cmd);
void virCommandDoAsyncIO(virCommandPtr cmd); + +void virCommandSetDryRun(virBufferPtr buf); #endif /* __VIR_COMMAND_H__ */
ACK Jan
The test tries to set some QoS limits and check if the commands that are actually executed are the expected ones. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 76 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 989018e..044decd 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -21,6 +21,7 @@ #include <config.h> #include "testutils.h" +#include "vircommand.h" #include "virnetdevbandwidth.h" #include "netdev_bandwidth_conf.c" @@ -32,6 +33,13 @@ struct testMinimalStruct { const char *band2; }; +struct testSetStruct { + const char *band; + const char *exp_cmd; + const char *iface; + const bool hierarchical_class; +}; + #define PARSE(xml, var) \ do { \ xmlDocPtr doc; \ @@ -104,6 +112,48 @@ cleanup: } static int +testVirNetDevBandwidthSet(const void *data) +{ + int ret = -1; + const struct testSetStruct *info = data; + const char *iface = info->iface; + virNetDevBandwidthPtr band = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual_cmd = NULL; + + PARSE(info->band, band); + + if (!iface) + iface = "eth0"; + + virCommandSetDryRun(&buf); + + if (virNetDevBandwidthSet(iface, band, info->hierarchical_class) < 0) + goto cleanup; + + if (!(actual_cmd = virBufferContentAndReset(&buf))) { + int err = virBufferError(&buf); + if (err) { + fprintf(stderr, "buffer's in error state: %d", err); + goto cleanup; + } + /* This is interesting, no command has been executed. + * Maybe that's expected, actually. */ + } + + if (STRNEQ_NULLABLE(info->exp_cmd, actual_cmd)) { + virtTestDifference(stderr, info->exp_cmd, actual_cmd); + goto cleanup; + } + + ret = 0; +cleanup: + virNetDevBandwidthFree(band); + VIR_FREE(actual_cmd); + return ret; +} + +static int mymain(void) { int ret = 0; @@ -117,6 +167,17 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_SET(Band, Exp_cmd, ...) \ + do { \ + struct testSetStruct data = {.band = Band, \ + .exp_cmd = Exp_cmd, \ + __VA_ARGS__}; \ + if (virtTestRun("virNetDevBandwidthSet", \ + testVirNetDevBandwidthSet, \ + &data) < 0) \ + ret = -1; \ + } while (0) + DO_TEST_MINIMAL(NULL, NULL, NULL); @@ -149,6 +210,21 @@ mymain(void) " <inbound average='1' peak='2' floor='3'/>" " <outbound average='5' peak='6'/>" "</bandwidth>"); + + DO_TEST_SET(("<bandwidth>" + " <inbound average='1' peak='2' floor='3' burst='4'/>" + " <outbound average='5' peak='6' burst='7'/>" + "</bandwidth>"), + ("/sbin/tc qdisc del dev eth0 root\n" + "/sbin/tc qdisc del dev eth0 ingress\n" + "/sbin/tc qdisc add dev eth0 root handle 1: htb default 1\n" + "/sbin/tc class add dev eth0 parent 1: classid 1:1 htb rate 1kbps ceil 2kbps burst 4kb\n" + "/sbin/tc qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" + "/sbin/tc filter add dev eth0 parent 1:0 protocol ip handle 1 fw flowid 1\n" + "/sbin/tc qdisc add dev eth0 ingress\n" + "/sbin/tc filter add dev eth0 parent ffff: protocol ip u32 match ip src 0.0.0.0/0 " + "police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n")); + return ret; } -- 1.8.5.2
On 01/28/2014 07:37 PM, Michal Privoznik wrote:
The test tries to set some QoS limits and check if the commands that are actually executed are the expected ones.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 76 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 989018e..044decd 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -104,6 +112,48 @@ cleanup: }
static int +testVirNetDevBandwidthSet(const void *data) +{ + int ret = -1; + const struct testSetStruct *info = data; + const char *iface = info->iface; + virNetDevBandwidthPtr band = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual_cmd = NULL; + + PARSE(info->band, band); + + if (!iface) + iface = "eth0"; + + virCommandSetDryRun(&buf); + + if (virNetDevBandwidthSet(iface, band, info->hierarchical_class) < 0) + goto cleanup; + + if (!(actual_cmd = virBufferContentAndReset(&buf))) { + int err = virBufferError(&buf); + if (err) { + fprintf(stderr, "buffer's in error state: %d", err); + goto cleanup; + } + /* This is interesting, no command has been executed. + * Maybe that's expected, actually. */ + } + + if (STRNEQ_NULLABLE(info->exp_cmd, actual_cmd)) { + virtTestDifference(stderr, info->exp_cmd, actual_cmd); + goto cleanup; + } + + ret = 0; +cleanup:
if virNetDevBandwidthSet executes some commands but fails, you should virBufferFreeAndReset(&buf) here
+ virNetDevBandwidthFree(band); + VIR_FREE(actual_cmd); + return ret; +} + +static int mymain(void) { int ret = 0;
ACK Jan
participants (2)
-
Ján Tomko -
Michal Privoznik