[libvirt] [PATCH 2/3] virCommand: Introduce virCommandSetDryRun

[Eric Blake]

On 01/27/2014 07:03 AM, 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.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---

>      str = virCommandToString(cmd);
> -    VIR_DEBUG("About to run %s", str ? str : cmd->args[0]);
> -    VIR_FREE(str);
> +    str = str ? str : cmd->args[0];

If virCommandToString() succeeded, str points to malloc'd memory.
Otherwise it points to cmd->args[0].  [1]

> +    VIR_DEBUG("About to run %s", str);
> +    if (outfile) {
> +        /* Dry-run is requested. Just append @str to @outfile
> +         * and claim success. */
> +
> +        if (virFileAppendStr(outfile, str, S_IRUSR | S_IWUSR) < 0 ||
> +            virFileAppendStr(outfile, "\n", 0) < 0) {

Hmm - that open()s the file twice.  Would it be any better to have
virFileAppendStr() take an additional bool parameter that says whether
to guarantee the appended text ends in newline, with only one open()?
But this is only a factor in the testsuite, so it's probably not worth
the micro-optimization.

> @@ -2303,6 +2320,7 @@ cleanup:
>          VIR_FORCE_CLOSE(cmd->infd);
>          VIR_FORCE_CLOSE(cmd->inpipe);
>      }
> +    VIR_FREE(str);

[1] If you hit OOM earlier, you now risk a double free of cmd->args[0].
 Not good.

>      return ret;
>  }
>  
> @@ -2334,6 +2352,13 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
>          return -1;
>      }
>  
> +    if (outfile) {
> +        /* Dry-run requested. Claim success. */
> +        if (exitstatus)

It might be worth a VIR_DEBUG here and above so that it is obvious when
reading logs that the command was not executed but that we faked success.


> + *
> + * Sometimes it's desired to not actually run given command, but
> + * see its string representation without having to change the
> + * callee. As a great example can serve unit testing. In such

s/As a great example can serve unit testing/Unit testing serves as a
great example/

But overall, the idea does look rather nice.  I'm glad it didn't add too
much code.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140127/a97e91c1/attachment-0001.sig>