Re: Check return value of pclose() correctly

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Check return value of pclose() correctly
Date: 2022-11-01 05:35:29
Message-ID: Y2CwITuzlNpF/
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2022-11-01 05:43:07 Re: Lock on ShmemVariableCache fields?
Previous Message Michael Paquier 2022-11-01 04:40:43 Re: Commit fest 2022-11