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