From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Check return value of pclose() correctly
Date: Tue, 01 Nov 2022 01:52:02 -0400
Message-ID: <1091330.1667281922@sss.pgh.pa.us> (raw)
In-Reply-To: <Y2CwITuzlNpF/dWj@paquier.xyz>
References: <8cd9fb02-bc26-65f1-a809-b1cb360eef73@enterprisedb.com>
<Y2CwITuzlNpF/dWj@paquier.xyz>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, michael@paquier.xyz, peter.eisentraut@enterprisedb.com
Subject: Re: Check return value of pclose() correctly
In-Reply-To: <1091330.1667281922@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox