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@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
-- |: 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 :|