On Sun, Jan 20, 2013 at 11:14:47AM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.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 thereturn 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).Incase /bin/sh could not be executed, the exit status will bethat 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@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ It's impossible for everything to be true. +