Re: pg_upgrade and system() return value

On Sun, Jan 20, 2013 at 11:14:47AM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Can someone comment on the attached patch? pg_upgrade was testing if
> > system() returned a non-zero value, while I am thinking I should be
> > adjusting system()'s return value with WEXITSTATUS().
>
> AFAIK it's not very good style to test the result as an integer, and
> testing for -1 is not an improvement on that. Actually it's a
> disimprovement, because the only case where the standard guarantees
> anything about the integer representation is that zero corresponds
> to "exited with status 0". See the Single Unix Spec, wherein system's
> result code is defined in terms of wait's, and the wait man page saith
>
> If and only if the status returned is from a terminated child process
> that returned 0 from main() or passed 0 as the status argument to
> _exit() or exit(), the value stored at the location pointed to by
> stat_loc will be 0. Regardless of its value, this information may be
> interpreted using the following macros ...
>
> If you want to do something different, then you could test for
> WIFEXITED && WEXITSTATUS == 0. (Testing the latter alone is flat
> wrong.) But I'm not particularly convinced that that's an improvement
> on what's there now. I note that your proposed patch would prevent
> any possibility of printing debug information about failure cases,
> since it loses the original result value.
>
> In short: it's not broken now, but this patch would break it.

I thought checking for non-zero was sufficient too, but my Debian
Squeeze system() manual page says:

The value returned is -1 on error (e.g. fork(2) failed), and the
return status of the command otherwise.

I am good with the above sentence, but the next sentences have me
confused:

This latter return status is in the format specified in wait(2).
Thus, the exit code of the command will be WEXITSTATUS(status).
In case /bin/sh could not be executed, the exit status will be
that of a command that does exit(127).

I assume my pg_upgrade waitpid() code is OK:

ret = waitpid(-1, &work_status, wait_for_child ? 0 : WNOHANG);

/* no children or, for WNOHANG, no dead children */
if (ret <= 0 || !WIFEXITED(work_status))
return false;

if (WEXITSTATUS(work_status) != 0)
pg_log(PG_FATAL, "child worker exited abnormally: %s\n", strerror(errno));

Can that be simplified too?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +