Re: pipe_read_line for reading arbitrary strings

On 06.03.24 10:54, Daniel Gustafsson wrote:
>> 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.

Also it shouldn't print %m, was my point.