Call perror on popen failure
If popen fails in pipe_read_line we invoke perror for the error message, and
pipe_read_line is in turn called by find_other_exec which is used in both
frontend and backend code. This is an old codepath, and it seems like it ought
to be replaced with the more common logging tools we now have like in the
attached diff (which also makes the error translated as opposed to now). Any
objections to removing this last perror() call?
--
Daniel Gustafsson
Attachments:
no_perror.diffapplication/octet-stream; name=no_perror.diff; x-unix-mode=0644Download
diff --git a/src/common/exec.c b/src/common/exec.c
index da929f15b9..bcd5b71101 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -378,7 +378,8 @@ pipe_read_line(char *cmd)
errno = 0;
if ((pipe_cmd = popen(cmd, "r")) == NULL)
{
- perror("popen failure");
+ log_error(errcode(ERRCODE_SYSTEM_ERROR),
+ _("could not execute command \"%s\": %m"), cmd);
return NULL;
}
Daniel Gustafsson <daniel@yesql.se> writes:
If popen fails in pipe_read_line we invoke perror for the error message, and
pipe_read_line is in turn called by find_other_exec which is used in both
frontend and backend code. This is an old codepath, and it seems like it ought
to be replaced with the more common logging tools we now have like in the
attached diff (which also makes the error translated as opposed to now). Any
objections to removing this last perror() call?
I didn't check your replacement code in detail, but I think we have
a policy against using perror, mainly because we can't count on it
to localize consistently with ereport et al. My grep confirms this
is the only use, so +1 for removing it.
regards, tom lane
On 08.03.24 11:05, Daniel Gustafsson wrote:
If popen fails in pipe_read_line we invoke perror for the error message, and
pipe_read_line is in turn called by find_other_exec which is used in both
frontend and backend code. This is an old codepath, and it seems like it ought
to be replaced with the more common logging tools we now have like in the
attached diff (which also makes the error translated as opposed to now). Any
objections to removing this last perror() call?
This change makes it consistent with other popen() calls, so it makes
sense to me.
On 8 Mar 2024, at 18:08, Peter Eisentraut <peter@eisentraut.org> wrote:
On 08.03.24 11:05, Daniel Gustafsson wrote:
If popen fails in pipe_read_line we invoke perror for the error message, and
pipe_read_line is in turn called by find_other_exec which is used in both
frontend and backend code. This is an old codepath, and it seems like it ought
to be replaced with the more common logging tools we now have like in the
attached diff (which also makes the error translated as opposed to now). Any
objections to removing this last perror() call?This change makes it consistent with other popen() calls, so it makes sense to me.
Thanks for review, committed that way.
--
Daniel Gustafsson