Re: pipe_read_line for reading arbitrary strings

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")?