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
signature.asc
Description: PGP signature
