Re: Check return value of pclose() correctly

On Mon, Oct 31, 2022 at 09:12:53AM +0100, Peter Eisentraut wrote:
> I noticed that some (not all) callers didn't check the return value of
> pclose() or ClosePipeStream() correctly.  Either they didn't check it at all
> or they treated it like the return of fclose().  Here is a patch with fixes.
>
> (A failure to run the command issued by popen() is usually reported via the
> pclose() status, so while you can often get away with not checking fclose()
> or close(), checking pclose() is more often useful.)

-   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?

> 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.
--
Michael