[libvirt] [PATCH v2 06/10] virCommandWait: Propagate dryRunCallback return value properly

[John Ferlan]

On 07/23/2018 08:27 AM, Michal Prívozník wrote:
> On 07/23/2018 02:07 PM, John Ferlan wrote:
>>
>>
>> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
>>> On 07/17/2018 08:43 PM, John Ferlan wrote:
>>>>
>>>>
>>>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>>>>> The documentation to virCommandWait() function states that if
>>>>> @exitstatus is NULL and command finished with error -1 is
>>>>> returned. In other words, if @dryRunCallback is set and returns
>>>>> an error (by setting its @status argument to a nonzero value) we
>>>>> must propagate this error properly honouring the documentation
>>>>> (and also regular run).
>>>>>
>>>>
>>>> That's not how I read virCommandWait:
>>>>
>>>>  * Wait for the command previously started with virCommandRunAsync()
>>>>  * to complete. Return -1 on any error waiting for
>>>>  * completion. Returns 0 if the command
>>>>  * finished with the exit status set.  If @exitstatus is NULL, then the
>>>>  * child must exit with status 0 for this to succeed.  By default,
>>>>  * a non-NULL @exitstatus contains the normal exit status of the child
>>>>  * (death from a signal is treated as execution error); but if
>>>>  * virCommandRawStatus() was used, it instead contains the raw exit
>>>>  * status that the caller must then decipher using WIFEXITED() and friends.
>>>>
>>>> perhaps the author (danpb) of commit id 7b3f1f8c3 would be able to say
>>>> for sure...
>>>>
>>>> I only see -1 being returned "on any error waiting for completion".
>>>> Filling @exitstatus with @dryRunStatus is reasonable since it is
>>>> initialized to 0 in virCommandRunAsync and is what is passed to
>>>> @dryRunCallback and thus only changed as a result of running
>>>> @dryRunCallback.
>>>>
>>>> It has nothing to do with virCommandWait AFAICT.
>>>
>>> So there are two ways how virCommandWait() can be called. The first is
>>> with @exitstatus being non-NULL. In this case, error is returned iff
>>> there was an error fetching command's exit status (e.g. because
>>> virProcessWait() failed). The second way is to call virCommandWait()
>>> with NULL in which case the function fails for all the cases in the
>>> first case plus if the command exit status is not zero. This is
>>> documented in docs/internals/command.html#async:
>>>
>>>
>>>   As with virCommandRun, the status arg for virCommandWait can be
>>>   omitted, in which case it will validate that exit status is zero and
>>>   raise an error if not.
>>>
>>> Let's put aside dry run case for a while. Imagine /bin/false was started
>>> asynchronously and control now reaches virCommandWait(cmd, NULL). What
>>> do you think should be expected return value? I'd expect "Child process
>>> (%) unexpected..." error message and return -1. However, this is not the
>>> case if dry run callback sets an error.
>>>
>>> Michal
>>>
>>
>> Was /bin/false run successfully?  It returns 1 (non zero). Isn't that
>> expected?  Did virCommandWait fail to wait for /bin/false to return?
> 
> Yes. Yes. No.
> 
>>
>> If someone wants the status from virCommandRunAsync, then they need to
>> pass the @exitstatus in; otherwise, the command itself actually ran to
>> completion and returned the expected result. If I want to know that
>> result, then I should use the proper mechanism which is to pass @exitstatus.
>>
>> The virCommandWait didn't fail (regardless of DryRun or not) to wait for
>> completion, so returning -1 because the underlying command "failed"
>> seems to be outside it's scope of purpose.
> 
> Okay. I believe picture is more than words. So try this example:
> 
> diff --git i/tools/virsh.c w/tools/virsh.c
> index 62226eea4c..2544fe0d74 100644
> --- i/tools/virsh.c
> +++ w/tools/virsh.c
> @@ -867,6 +867,18 @@ main(int argc, char **argv)
>      virshControl virshCtl;
>      bool ret = true;
> 
> +    virCommandPtr cmd = virCommandNewArgList("/bin/false", NULL);
> +
> +    if (virCommandRunAsync(cmd, NULL) < 0) {
> +        fprintf(stderr, "async()\n");
> +        return -1;
> +    }
> +
> +    if (virCommandWait(cmd, NULL) < 0) {
> +        fprintf(stderr, "wait()\n");
> +        return -1;
> +    }
> +
>      memset(ctl, 0, sizeof(vshControl));
>      memset(&virshCtl, 0, sizeof(virshControl));
>      ctl->name = "virsh";        /* hardcoded name of the binary */
> 
> 
> And try replacing /bin/false with /bin/true. Also, try passing an int to
> virCommandWait() instead of NULL. You'll see what I mean then.

What you expect me to WORK for the answer ;-)

Code as written:

$ ./run tools/virsh
wait()
$ echo $?
255
$

<change from /bin/false to /bin/true>

$ ./run tools/virsh
virCommandWait exitstatus=1
virsh # quit
$

<go back to /bin/false, add &exitstatus to virCommandWait, *and* a print
of status>

$ ./run tools/virsh
virCommandWait exitstatus=0
virsh # quit
$

<change from /bin/false to /bin/true>

$ ./run tools/virsh
virCommandWait exitstatus=0
virsh # quit
$


I don't see the problem if you decide to pass @exitstatus to
virCommandWait and then choose to ignore actually checking the status,
then you WYSIWYG.

If in the /bin/false case you checked "if (exitstatus != 0)" and fail,
then you'd get what I think you're hinting should happen.

> 
> And the whole point is that if we have a dry run callback set and it
> indicates an error we have to make virCommandWait(, NULL) fail - just
> like when it's failing with real execution.
> 

If the caller passes @exitstatus to virCommandWait, but doesn't check
it's value, then who's coding error is that?

I see virCommandWait documented as:

"If @exitstatus is NULL, then the child must exit with status 0 for this
to succeed."

IOW, AIUI, usage of @exitstatus gives one finer grained control over
knowing whether the command run by virCommandRunAsync failed or whether
virCommandWait failed.  If exitstatus == NULL, then if either fails, you
get a -1 returned; otherwise, using exitstatus you can determine whether
the command failed or the wait for the command to run failed.

John