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

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.