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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list