[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 1/2] virCommand: Introduce virCommandSetDryRun



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]