[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 at 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 :|