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

[Michal Privoznik]

On 07/23/2018 03:49 PM, John Ferlan wrote:
> 
> 
> 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()

So virCommandWait() failed. As expected.

> $ echo $?
> 255
> $
> 
> <change from /bin/false to /bin/true>
> 
> $ ./run tools/virsh
> virCommandWait exitstatus=1

It did not fail here, because /bin/true returns 0 (in contrast to
/bin/false which returns -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.
> 

Sure, but that is not the point. The point is, caller can pass NULL
which is equivalent to saying "I don't care what the actual problem was,
if anything fails (be it failing to wait, or command returning non zero,
or OOM, or anything else) I want you to return -1".
It can be written in dozen other ways, sure. But we already have chosen
one. And if we use dry run there is a bug because it behaves differently
than actual command execution.

> 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.

This is not the point.

> 
>>
>> 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?

No one's. This is equivalent to saying: "I don't care what command
returned (whether it succeeded or not), I'm only interested whether it
finished, i.e. we successfully waited for it". This is allowed also.
But then again, I'm trying to fix case where @exitstatus is NULL.

> 
> 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.

Exactly! But, this is not the case when dry run callback returns an
error but caller waits with @exitstatus = NULL.

IOW, now that we've seen how virCommandWait() behaves with real
binaries, we can take that behaviour for granted and have to make sure
that using dry run callbacks behaves the same. Does it? I don't think so.

Michal