>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_...@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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
