[libvirt] [PATCH] Take DryRun code out of virCommand* code flow

[Daniel P. Berrangé]

On Wed, Aug 22, 2018 at 11:37:34AM +0800, Shi Lei wrote:
> Since dry-run of cmd is just for tests now, it would be better to remove
> dry-run code and move them to testutils. Then the code flow in virCommand*
> could be more general. There are 3 steps in this patch:
> 1. Introduce a new global hook (of virExecHook type) which will be called
>    in code flow just before the cmd->hook. The global hook is also called in
>    child process. If it returns 1, the child process will exit with status
>    in advance and the parent will process io and wait for the child normally.
>    It prepares for registering dry-run and anything else.
>    The virCommandSetPreExecHook is modified for registering both types of hooks.
> 2. Implement virTestSetDryRun with dry-run code in "testutils.c".
>    It substitutes for virCommandSetDryRun. The virTestSetDryRun invokes
>    virCommandSetPreExecHook to register a function on the global hook which
>    will fill in cmdline buffer and call callback for tests.
> 3. Remove all dryrun code in "vircommand.c" and remove virCommandSetDryRun API.
> 
> Diffs from old dry-run:
> The new global hook is called in child process. So dryrun-callback for tests
> should write to stdout/stderr when they need output something.
> 
> Now the opaque argument for dryrun-callback cannot be inout. In "tests/viriscsitest.c",
> iface_created in opaque of callback is an inout argument. There's a bit
> complicated to transfer it between the child and its parent. So this patch use
> a temporary file to do that.
>    
> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
> ---
>  docs/internals/command.html.in   |  33 +++++-
>  src/libvirt_private.syms         |   4 +-
>  src/qemu/qemu_process.c          |   8 +-
>  src/util/vircommand.c            | 151 ++++++++++++----------------
>  src/util/vircommand.h            |  18 +++-
>  src/util/vircommandpriv.h        |  19 ++--
>  tests/networkxml2firewalltest.c  |   7 +-
>  tests/nwfilterebiptablestest.c   |  32 +++---
>  tests/nwfilterxml2firewalltest.c |   6 +-
>  tests/testutils.c                | 210 +++++++++++++++++++++++++++++++++++++++
>  tests/testutils.h                |   2 +
>  tests/virfirewalltest.c          |  71 ++++++-------
>  tests/viriscsitest.c             | 120 ++++++++++++++++------
>  tests/virkmodtest.c              |  10 +-
>  tests/virnetdevbandwidthtest.c   |   6 +-
>  15 files changed, 482 insertions(+), 215 deletions(-)
> 
> diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
> index 43f51a4..44478f0 100644
> --- a/docs/internals/command.html.in
> +++ b/docs/internals/command.html.in
> @@ -398,17 +398,40 @@ virCommandSetWorkingDirectory(cmd, LOCALSTATEDIR);
>      <h3><a id="hooks">Any additional hooks</a></h3>
>  
>      <p>
> -      If anything else is needed, it is possible to request a hook
> -      function that is called in the child after the fork, as the
> +      If anything else is needed, it is possible to request hook
> +      functions that are called in the child after the fork, as the
>        last thing before changing directories, dropping capabilities,
> -      and executing the new process.  If hook(opaque) returns
> -      non-zero, then the child process will not be run.
> +      and executing the new process.
> +
> +      There are two types of hooks:
> +    <ul>
> +      <li>Global hook takes effect to all command.
> +      Setting global hook if passing NULL to @cmd.
> +      </li>
> +      <li>Command hook takes effect only to the command.
> +      Passing the pointer of the command to @cmd.
> +      </li>
> +    </ul>
> +
> +      If hook(opaque) returns -1, then the child process will not be
> +      run for error. And if it returns 1, the child process will exit
> +      with status in advance and this mode is used for command dryrun.
> +
> +      Since @hook runs in the child, it should be careful to avoid
> +      any functions that are not async-signal-safe.
> +
> +      For global hook, Passing NULL for @cmd, @hook and @opaque to
> +      cancel the effect.
>      </p>
>  
>  <pre>
> -virCommandSetPreExecHook(cmd, hook, opaque);
> +virCommandSetPreExecHook(NULL, hook, opaque); /* global hook */
> +virCommandSetPreExecHook(cmd, hook, opaque); /* command hook */
> +
> +virCommandSetPreExecHook(NULL, NULL, NULL); /* cancel global hook */
>  </pre>

[snip]

>  void
>  virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque)
>  {
> -    if (!cmd || cmd->has_error)
> +    if (!cmd) {
> +        /* Global hook */
> +        preExecHook = hook;
> +        preExecOpaque = opaque;
> +        return;
> +    }

I don't think this is an improvement.

With the virCommandSetDryRun() approach there is no way that the dry run
code can be accidentally triggered in production scenarios, as we can be
sure nothing will accidentally call virCommandSetDryRun().

Changing the semantics of virCommandSetPreExecHook() so that it sets a
global hook when 'cmd' is NULL introduces significant risk. The virCommand
APIs are designed to fail-safe in face of memory exhaustion or errors from
the caller. IOW passing a NULL for 'cmd' is an expected scenario in a
production environment, and this change breaks handling of that.

Personally I don't think the stated problem needs solving at all. The
virCommandSetDryRun() works reliably and doesn't need rewriting IMHO. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|