Date: Wed, 10 Nov 2010 22:20:01 -0800 (PST) From: "Ronald F.Guilmette" <rfg@tristatelogic.com> To: FreeBSD-gnats-submit@FreeBSD.org Subject: bin/152132: Useless code in script.c (part 2) Message-ID: <20101111062001.CC954BDC35@segfault.tristatelogic.com> Resent-Message-ID: <201011110620.oAB6KAos045621@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 152132 >Category: bin >Synopsis: Useless code in script.c (part 2) >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: change-request >Submitter-Id: current-users >Arrival-Date: Thu Nov 11 06:20:09 UTC 2010 >Closed-Date: >Last-Modified: >Originator: Ronald F. Guilmette >Release: FreeBSD 7.0-RELEASE i386 >Organization: Infinite Monkeys & Co. >Environment: System: FreeBSD segfault.tristatelogic.com 7.0-RELEASE FreeBSD 7.0-RELEASE #0: Tue Aug 5 02:38:40 PDT 2008 root@segfault.monkeys.com:/usr/src/sys/i386/compile/rfg20080805 i386 >Description: There appears to be a bunch of utterly useless (and perhaps even time wasting) code in script.c, and specifically within the finish() function. In that function there is a loop that reaps all child processes that are immediately available for reaping. But this must be a leftover from an earlier time when script.c was implemented with -two- child processes. Nowadays, it uses only one child process, i.e. the child shell process. So it makes no sense to loop while trying to reap multiple child processes, because there is at most only one, the child shell process (and so it isn't even necessary or useful to call wait3() because a simple call to wait() should do.) Furthermore, if the idea is to reap the one and only child process, then before the reaping attempt even begins something positive should be done to cause the child to understand that it should exit now, i.e. calling close() on the "master" pty fd, i.e. just prior to all places in the code where the finish() function (or the done() function) is about to be called. That should cause the child (shell) process to receive EOF on stdin/stdout/stderr, and that will probably cause it to figure out that it is time to go bye bye (if it hasn't done so already). >How-To-Repeat: Look at the code. >Fix: *** script.c.orig 2004-02-15 09:30:13.000000000 -0800 --- script.c 2010-11-10 22:02:59.000000000 -0800 *************** *** 155,158 **** --- 155,159 ---- if (child < 0) { warn("fork"); + (void)close(master); done(1); } *************** *** 204,207 **** --- 203,207 ---- } } + (void)close(master); finish(); done(0); *************** *** 220,238 **** { pid_t pid; ! int die, e, status; ! die = e = 0; ! while ((pid = wait3(&status, WNOHANG, 0)) > 0) ! if (pid == child) { ! die = 1; ! if (WIFEXITED(status)) ! e = WEXITSTATUS(status); ! else if (WIFSIGNALED(status)) ! e = WTERMSIG(status); ! else /* can't happen */ ! e = 1; ! } ! if (die) done(e); } --- 220,235 ---- { pid_t pid; ! int e, status; ! e = 0; ! wait(&status); ! if (WIFEXITED(status)) ! e = WEXITSTATUS(status); ! else if (WIFSIGNALED(status)) ! e = WTERMSIG(status); ! else /* can't happen */ ! e = 1; ! if (e) done(e); } *************** *** 264,267 **** --- 261,265 ---- { (void)kill(0, SIGTERM); + (void)close(master); done(1); } *************** *** 280,284 **** } (void)fclose(fscript); - (void)close(master); exit(eno); } --- 278,281 ---- >Release-Note: >Audit-Trail: >Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101111062001.CC954BDC35>