On Mon, Oct 19, 2009 at 10:31:11AM +0200, Otto Moerbeek wrote: > On Sun, Oct 18, 2009 at 03:50:17PM -0700, Philip Guenther wrote: > > > On Sun, Oct 18, 2009 at 12:49 AM, Matt Fisher <mfisher...@maine.rr.com> > > wrote: > > > You're correct. I got started on this train of thought after reading > > > an old USENIX paper[1]. The line I suggested for deletion is identical > > > to what section 1.2.1 of that paper recommended should always be > > > included. Recent material continues to recommend such code, but not > > > specifically in OpenBSD[2],[3]. > > > > > > Since such code is only rarely used in OpenBSD -- 9 occurrences in the > > > whole tree -- I thought it might be an obsolete or inapplicable > > > recommendation. One such occurrence was in m4/main.c, so I contacted the > > > developer who had recently checked in that file. > > > > > > I did read manuals, study code, consider, and test first. But I knew my > > > expertise was limited, and that there could be other considerations. > > > > If all you have is a chunk of code that you don't understand the > > justification for, you have to work backwards to the problem. This > > can turn into the programming equivalent of disproving an existential, > > for which having a broad base of experience really helps. How do you > > get that base of experience? Well, start by tackling concrete > > problems ("program behaves incorrectly when I try <blah>") instead of > > jumping straight to the abstract cases. > > > > That, or you need to crank up your methodology and logic. The code in > > this case applies only when the process is started with SIGINT set to > > SIG_IGN. If the process was set by its parent to ignore SIGINT, why > > would it start paying attention to them now by catching them? Doing > > that might be fine if that was something that used to be normal > > practice but never happened nowadays. So what programs did that in > > the past? Didn't the paper say something about running stuff in the > > background from the shell with "&"? So let's check the sh(1) manpage > > to see what it says now: > > When an asynchronous command is started when job > > control is disabled (i.e. in most scripts), the command is started with > > signals SIGINT and SIGQUIT ignored > > > > Looks like the shell still does that. A little bit of noodling shows > > that this snippet of shell script > > #!/bin/sh > > ( : ; /some/program/here ) & > > > > Invokes /some/program/here with SIGINT ignored. > > > > So, it looks like running m4 in the background from a shell script > > will still result in it being invoked with SIGINT set to SIG_IGN. In > > such a case, if you interrupted the shell script using SIGINT, > > shouldn't m4 ignore the signal and continue? It would continue if the > > shell script exited normally, so why should signaling the script > > terminate it? Ergo, I think this code is correct and should be left > > as is. > > > > You wonder why only 9 programs in the tree have such code. Well, how > > programs does this problem apply to? It doesn't apply to any program > > that calls daemon(), for example, as they won't get SIGINTs a > > terminal. So rpc.statd's signal handling is fine, for example. It > > also doesn't apply to programs that can't be usefully backgrounded, so > > 'rain' is fine as is. > > > > But it looks like some programs do handle it wrong and could use > > fixing. For example, 'sort' appears to be wrong...but before I wrote > > that I figured out how to trigger the problem, both to confirm it and > > to verify the fix. Make sense? > > > > Philip Guenther > > I'm not convinced. What behaviour do you consider wrong? I think that > temp files created should always be cleaned up, even when the program > ran in the background.
Ehh, please ignore that comment. Too little coffee. The diff looks good. > > -Otto > > > > > > > --- sort.c 22 Aug 2007 06:56:40 -0000 1.36 > > +++ sort.c 18 Oct 2009 22:44:47 -0000 > > @@ -273,7 +273,7 @@ main(int argc, char *argv[]) > > outfile = outpath = toutpath; > > } else if (!(ch = access(outpath, 0)) && > > strncmp(_PATH_DEV, outpath, 5)) { > > - struct sigaction act; > > + struct sigaction oact, act; > > int sigtable[] = {SIGHUP, SIGINT, SIGPIPE, SIGXCPU, SIGXFSZ, > > SIGVTALRM, SIGPROF, 0}; > > int outfd; > > @@ -298,7 +298,9 @@ main(int argc, char *argv[]) > > act.sa_flags = SA_RESTART; > > act.sa_handler = onsig; > > for (i = 0; sigtable[i]; ++i) /* always unlink toutpath */ > > - sigaction(sigtable[i], &act, 0); > > + if (sigaction(sigtable[i], NULL, &oact) || > > + oact.sa_handler != SIG_IGN) > > + sigaction(sigtable[i], &act, NULL); > > } else > > outfile = outpath; > > if (outfp == NULL && (outfp = fopen(outfile, "w")) == NULL)