[libvirt] [PATCH v2 0/3] Enhance virnetdevbandwidthtest
Round two. Michal Privoznik (3): virfile: Introduce virFileAppendStr virCommand: Introduce virCommandSetDryRun virnetdevbandwidthtest: Introduce testVirNetDevBandwidthSet src/libvirt_private.syms | 2 + src/util/vircommand.c | 58 ++++++++++++++++++++++++- src/util/vircommand.h | 2 + src/util/virfile.c | 57 ++++++++++++++++++++---- src/util/virfile.h | 3 ++ tests/virnetdevbandwidthtest.c | 98 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 210 insertions(+), 10 deletions(-) -- 1.8.5.2
So far we only have an API that truncates the file prior to writing it. However, experience show need for new API that just appends a string to file. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 57 ++++++++++++++++++++++++++++++++++++++++-------- src/util/virfile.h | 3 +++ 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..f9bdd8c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1202,6 +1202,7 @@ virBuildPathInternal; virDirCreate; virFileAbsPath; virFileAccessibleAs; +virFileAppendStr; virFileBuildPath; virFileClose; virFileDeleteTree; diff --git a/src/util/virfile.c b/src/util/virfile.c index 631cd06..9357e09 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1291,23 +1291,21 @@ virFileReadAll(const char *path, int maxlen, char **buf) return len; } -/* Truncate @path and write @str to it. If @mode is 0, ensure that - @path exists; otherwise, use @mode if @path must be created. - Return 0 for success, nonzero for failure. - Be careful to preserve any errno value upon failure. */ -int -virFileWriteStr(const char *path, const char *str, mode_t mode) +static int +virFileWriteAppendStr(const char *path, const char *str, + int o_flags, mode_t mode, bool newline) { int fd; if (mode) - fd = open(path, O_WRONLY|O_TRUNC|O_CREAT, mode); + fd = open(path, o_flags | O_CREAT, mode); else - fd = open(path, O_WRONLY|O_TRUNC); + fd = open(path, o_flags); if (fd == -1) return -1; - if (safewrite(fd, str, strlen(str)) < 0) { + if (safewrite(fd, str, strlen(str)) < 0 || + (newline && safewrite(fd, "\n", strlen("\n")) < 0)) { VIR_FORCE_CLOSE(fd); return -1; } @@ -1319,6 +1317,47 @@ virFileWriteStr(const char *path, const char *str, mode_t mode) return 0; } +/** + * virFileWriteStr: + * @path: file to write to + * @str: string to write + * @mode: file mode used when creating @path + * + * Truncate @path and write @str to it. If @mode is 0, ensure + * that @path exists; otherwise, use @mode if @path must be + * created. Be careful to preserve any errno value upon failure. + * + * Returns 0 on success, -1 on failure. + */ +int +virFileWriteStr(const char *path, const char *str, mode_t mode) +{ + return virFileWriteAppendStr(path, str, O_WRONLY | O_TRUNC, + mode, false); +} + +/** + * virFileAppendStr: + * @path: file to write to + * @str: string to write + * @mode: file mode used when creating @path + * @newline: append the newline character after @str? + * + * Append @str to @path. If the file doesn't exist, + * it is created using @mode. Moreover, if @newline + * is true, the newline character is appended to + * @path after @str is written. + * + * Returns 0 on success, -1 on failure. + */ +int +virFileAppendStr(const char *path, const char *str, + mode_t mode, bool newline) +{ + return virFileWriteAppendStr(path, str, O_WRONLY | O_APPEND, + mode, newline); +} + int virFileMatchesNameSuffix(const char *file, const char *name, diff --git a/src/util/virfile.h b/src/util/virfile.h index 0d20cdb..bd255f4 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -131,6 +131,9 @@ int virFileReadAll(const char *path, int maxlen, char **buf) int virFileWriteStr(const char *path, const char *str, mode_t mode) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virFileAppendStr(const char *path, const char *str, + mode_t mode, bool newline) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virFileMatchesNameSuffix(const char *file, const char *name, -- 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/vircommand.h | 2 ++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f9bdd8c..814c74c 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..11e827f 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 const char *outfile; + /* * virCommandFDIsSet: * @fd: FD to test @@ -2263,7 +2266,21 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) str = virCommandToString(cmd); VIR_DEBUG("About to run %s", str ? str : cmd->args[0]); - VIR_FREE(str); + if (outfile) { + /* Dry-run is requested. Just append @str to @outfile + * and claim success. */ + + VIR_DEBUG("Dry run requested, writing stringified command to %s", outfile); + if (virFileAppendStr(outfile, str ? str : cmd->args[0], + S_IRUSR | S_IWUSR, true) < 0) { + virReportSystemError(errno, _("unable to append to %s"), + outfile); + goto cleanup; + } + + ret = 0; + goto cleanup; + } ret = virExec(cmd); VIR_DEBUG("Command result %d, with PID %d", @@ -2303,6 +2320,7 @@ cleanup: VIR_FORCE_CLOSE(cmd->infd); VIR_FORCE_CLOSE(cmd->inpipe); } + VIR_FREE(str); return ret; } @@ -2334,6 +2352,14 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) return -1; } + if (outfile) { + /* 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 +2695,33 @@ virCommandDoAsyncIO(virCommandPtr cmd) cmd->flags |= VIR_EXEC_ASYNC_IO | VIR_EXEC_NONBLOCK; } + +/** + * virCommandSetDryRun: + * @file: destination file + * + * 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 @file instead of being executed. For example: + * + * virCommandSetDryRun("/tmp/test.txt"); + * + * const char *const echocmd[] = {"/bin/echo", "Hello world"} + * virCommandRun(echocmd, NULL); + * + * After this, there's /tmp/test.txt with contents of: + * + * /bin/echo Hello world + * + * To cancel this effect pass NULL. + */ +void +virCommandSetDryRun(const char *file) +{ + outfile = file; +} diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e977f93..d942c5b 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(const char *file); #endif /* __VIR_COMMAND_H__ */ -- 1.8.5.2
On Tue, Jan 28, 2014 at 05:08:14PM +0100, 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 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.
diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e977f93..d942c5b 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(const char *file);
Do we really need to write it out to a file, or would it be simpler to just pass in a virBufferPtr instead, so we just write to RAM. This avoids any tmpfile naming complexity in code using this. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/28/2014 09:51 AM, Daniel P. Berrange wrote:
Do we really need to write it out to a file, or would it be simpler to just pass in a virBufferPtr instead, so we just write to RAM. This avoids any tmpfile naming complexity in code using this.
Ah, a virBufferPtr would be even nicer - less points of failure. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 | 98 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 989018e..a8156cd 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -20,7 +20,10 @@ #include <config.h> +#include <fcntl.h> +#include <stdlib.h> #include "testutils.h" +#include "vircommand.h" #include "virnetdevbandwidth.h" #include "netdev_bandwidth_conf.c" @@ -32,6 +35,14 @@ struct testMinimalStruct { const char *band2; }; +struct testSetStruct { + const char *outfile; + const char *band; + const char *exp_cmd; + const char *iface; + const bool hierarchical_class; +}; + #define PARSE(xml, var) \ do { \ xmlDocPtr doc; \ @@ -104,9 +115,64 @@ cleanup: } static int +testVirNetDevBandwidthSet(const void *data) +{ + int ret = -1; + const struct testSetStruct *info = data; + const char *iface = info->iface; + virNetDevBandwidthPtr band = NULL; + char *actual_cmd = NULL; + + PARSE(info->band, band); + + if (!iface) + iface = "eth0"; + + /* Clear the temporary file between runs */ + if (truncate(info->outfile, 0) < 0) { + fprintf(stderr, "Unable to truncate %s %d\n", info->outfile, errno); + goto cleanup; + } + + virCommandSetDryRun(info->outfile); + + if (virNetDevBandwidthSet(iface, band, info->hierarchical_class) < 0) + goto cleanup; + + if (virFileReadAll(info->outfile, 2048, &actual_cmd) < 0) + goto cleanup; + + if (STRNEQ(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; +} + + +#define OUTFILETEMPLATE abs_builddir "/virnetdevbandwidthtest-XXXXXX" + +static int mymain(void) { int ret = 0; + int outfile_fd = -1; + char *outfile; + + if (VIR_STRDUP_QUIET(outfile, OUTFILETEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if ((outfile_fd = mkostemp(outfile, O_CLOEXEC)) < 0) { + fprintf(stderr, "Cannot create outfile"); + abort(); + } #define DO_TEST_MINIMAL(r, ...) \ do { \ @@ -117,6 +183,18 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_SET(Band, Exp_cmd, ...) \ + do { \ + struct testSetStruct data = {.outfile = outfile, \ + .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 +227,26 @@ 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")); + + VIR_FORCE_CLOSE(outfile_fd); + + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) + unlink(outfile); + return ret; } -- 1.8.5.2
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik