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

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