> 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. I'm not convinced that a function to read from a pipe should consider not reading anything successful by default, output is sort expected here. We could add a flag parameter to use for signalling that no data is fine though as per the attached (as of yet untested) diff? -- Daniel Gustafsson
fix_errhandling.diff
Description: Binary data
