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

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.