On 06.03.24 10:54, Daniel Gustafsson wrote:
On 6 Mar 2024, at 10:07, Peter Eisentraut <pe...@eisentraut.org> wrote:
On 22.11.23 13:47, Alvaro Herrera wrote:
On 2023-Mar-07, Daniel Gustafsson wrote:
The attached POC diff replace fgets() with pg_get_line(), which may not be an
Ok way to cross the streams (it's clearly not a great fit), but as a POC it
provided a neater interface for reading one-off lines from a pipe IMO. Does
anyone else think this is worth fixing before too many callsites use it, or is
this another case of my fear of silent subtle truncation bugs? =)
I think this is generally a good change.
I think pipe_read_line should have a "%m" in the "no data returned"
error message. pg_read_line is careful to retain errno (and it was
already zero at start), so this should be okay ... or should we set
errno again to zero after popen(), even if it works?
Is this correct? The code now looks like this:
line = pg_get_line(pipe_cmd, NULL);
if (line == NULL)
{
if (ferror(pipe_cmd))
log_error(errcode_for_file_access(),
_("could not read from command \"%s\": %m"), cmd);
else
log_error(errcode_for_file_access(),
_("no data was returned by command \"%s\": %m"), cmd);
}
We already handle the case where an error happened in the first branch, so
there cannot be an error set in the second branch (unless something nonobvious
is going on?).
It seems to me that if the command being run just happens to print nothing but is
otherwise successful, this would print a bogus error code (or "Success")?
Good catch, that's an incorrect copy/paste, it should use ERRCODE_NO_DATA.
Also it shouldn't print %m, was my point.
