On Sun, Jan 20, 2013 at 11:14:47AM -0500, Tom Lane wrote: > Bruce Momjian <br...@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 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 <br...@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers