> On 6 Mar 2024, at 10:07, Peter Eisentraut <peter@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
printa 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
