Re: Check return value of pclose() correctly

On 01.11.22 06:35, Michael Paquier wrote:
> -   if (WIFEXITED(exitstatus))
> +   if (exitstatus == -1)
> +   {
> +       snprintf(str, sizeof(str), "%m");
> +   }
> This addition in wait_result_to_str() looks inconsistent with the
> existing callers of pclose() and ClosePipeStream() that check for -1
> as exit status.  copyfrom.c and basebackup_to_shell.c fall into this
> category.  Wouldn't it be better to unify everything?

With the above addition, the extra check for -1 at those existing places 
could be removed.

>> There are some places where the return value is apparently intentionally
>> ignored, such as in error recovery paths, or psql ignoring a failure to
>> launch the pager.  (The intention can usually be inferred by the kind of
>> error checking attached to the corresponding popen() call.)  But there are a
>> few places in psql that I'm suspicious about that I have marked, but need to
>> think about further.
> 
> Hmm.  I would leave these out, I think.  setQFout() relies on the
> result of openQueryOutputFile().  And this could make commands like
> \watch less reliable.

I don't quite understand what you are saying here.  My point is that, 
for example, setQFout() thinks it's important to check the result of 
popen() and write an error message, but it doesn't check the result of 
pclose() at all.  I don't think that makes sense in practice.