Re: pipe_read_line for reading arbitrary strings

> 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 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