From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Check return value of pclose() correctly
In-reply-to: <Y2CwITuzlNpF/dWj@paquier.xyz>
References: <8cd9fb02-bc26-65f1-a809-b1cb360eef73@enterprisedb.com>
 <Y2CwITuzlNpF/dWj@paquier.xyz>
Comments: In-reply-to Michael Paquier <michael@paquier.xyz>
	message dated "Tue, 01 Nov 2022 14:35:29 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1091329.1667281922.1@sss.pgh.pa.us>
Date: Tue, 01 Nov 2022 01:52:02 -0400
Message-ID: <1091330.1667281922@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1091330.1667281922%40sss.pgh.pa.us>
Precedence: bulk

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Oct 31, 2022 at 09:12:53AM +0100, Peter Eisentraut wrote:
>> (A failure to run the command issued by popen() is usually reported via the
>> pclose() status, so while you can often get away with not checking fclose()
>> or close(), checking pclose() is more often useful.)
 
> -   if (WIFEXITED(exitstatus))
> +   if (exitstatus == -1)
> +   {
> +       snprintf(str, sizeof(str), "%m");
> +   }
> This addition in wait_result_to_str() looks inconsistent with the 
> existing callers of pclose() and ClosePipeStream() that check for -1
> as exit status.  copyfrom.c and basebackup_to_shell.c fall into this
> category.  Wouldn't it be better to unify everything?

I think there are two issues here.  POSIX says

    Upon successful return, pclose() shall return the termination status
    of the command language interpreter. Otherwise, pclose() shall return
    -1 and set errno to indicate the error.

That is, first you need to make sure that pclose returned a valid
child process status, and then you need to decode that status.
It's not obvious to me that -1 is disjoint from the set of possible
child statuses.  Do we need to add some logic that clears and then
checks errno?

Also, we have a number of places --- at least FreeDesc() and
ClosePipeStream() --- that consider pclose()'s return value to be
perfectly equivalent to that of close() etc, because they'll
return either one without telling the caller which is which.
It seems like we have to fix that if we want to issue sane
error reports.

This patch isn't moving things forward on this fundamental
confusion.

			regards, tom lane