Re: [libvirt] [PATCH] Take DryRun code out of virCommand* code flow
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(a)massclouds.com>
> ---
[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
Okay. Thanks for your comments.
Regards,
ShiLei