[libvirt] [PATCH v2 06/10] virCommandWait: Propagate dryRunCallback return value properly
John Ferlan
jferlan at redhat.com
Mon Jul 23 12:07:37 UTC 2018
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?
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.
AFAICT, @DryRunStatus is meant to mimic @exitstatus.
John
More information about the libvir-list
mailing list