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