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