Date: Mon, 30 Sep 2002 14:50:01 -0700 From: Juli Mallett <jmallett@FreeBSD.org> To: Robert Watson <rwatson@FreeBSD.org> Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/alpha/osf1 osf1_signal.c src/sys/compat/linprocfs linprocfs.c src/sys/compat/linux linux_misc.c linux_signal.c src/sys/compat/svr4 svr4_filio.c svr4_signal.c src/sys/conf files src/sys/fs/procfs procfs_ctl.c src/sys/kern init_main.c ... Message-ID: <20020930145001.A44977@FreeBSD.org> In-Reply-To: <Pine.NEB.3.96L.1020930172114.15622W-100000@fledge.watson.org>; from rwatson@FreeBSD.org on Mon, Sep 30, 2002 at 05:30:46PM -0400 References: <200209302020.g8UKKMRt092756@freefall.freebsd.org> <Pine.NEB.3.96L.1020930172114.15622W-100000@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Sorry [and sorry for top-replying, but there's no good inline place to respond], this is something I'm kicking myself for, too, and it's what I get for writing a commit message one day, and using it another. Especially, when said message was written early early in the morning. I'll flesh out what exactly *is* going on: I've added a structure, kernel-private, to represent a pending or in-delivery signal, called `ksiginfo'. It is roughly analogous to the basic information that is exported by the POSIX interface 'siginfo_t', but more basic. I've added functions to allocate these structures, and further to wrap all signal operations using them. Once the operations are wrapped, I've added a TailQ (see queue(3)) of these structures to 'struct proc', and all pending signals are in that TailQ. When a signal is being delivered, it is dequeued from the list. Once I finish the spreading of ksiginfo throughout the tree, the dequeued structure will be delivered to the process in question, whereas currently and normally, the signal number is what is used. The point of the bullshit (pardon me) that was in my commit message was to attempt to alleviate worries that threaded programs, etc., may be very very confused. Unfortunately that obscured the real message, and kept me from adequately giving exposition of the changes in the commit message. I'd accept a day in the box for this grave oversight on my part. juli. * De: Robert Watson <rwatson@FreeBSD.org> [ Data: 2002-09-30 ] [ Subjecte: Re: cvs commit: src/sys/alpha/osf1 osf1_signal.c src/sys/compat/linprocfs linprocfs.c src/sys/compat/linux linux_misc.c linux_signal.c src/sys/compat/svr4 svr4_fil > Your commit message neglected to really say what this actually did: > > First half of implementation of ksiginfo, signal queues, and such. > > This is not sufficient; you failed to identify the semantic and syntactic > differences before and after your change, and what (if any) benefit the > changes had. I've noticed a recent downturn in terms of commit message > quality from a number of developers, and we really need to reverse the > trend. A good commit message should identify the problem being addressed > or task being performed at a high level, provide a high level description > of the changes necessary to guide a reader of the patches (or inform > someone who doesn't have time to read every patch), and leave a decent > independent and historical record that can be used to cross-reference > changes from the output of individual log files. Also, any expected side > effects may be useful. > > The following commit messages are disastrous examples or > mis-interpretations of our commit message policies: > > Oops, here's the rest of that patch. > > Redo X. > > More of the same. > > Change the way we handle <x>. > > This kind of commit message is worse than useless, since it's also > frustrating. Your commit message was a lot more verbose, but still not > very much more useful than those examples. Indicating that you've tested > the changes with X is also nice to know, or that you plan to go somewhere > in the future, but far less vital to the commit message than some actual > semantic content on what you changed (and maybe even why). For example, > you throw around the word "ksiginfo" but don't attempt to define it. You > lead me, as the reader, to guess that what you might be doing is using > queues to hold signal data, but you never explicitly say that. It's not > sufficient just to do the kernel hacking--committers need to communicate > the work they've done more effectively for both immediate and long-term > consumption. > > Please get your next few commit messages reviewed by some third parties > before committing if you are in any doubt about what I'm talking about. > > Also, on the Coda topic -- you were confused about what it did. Did you > try talking to the maintainer? > > Robert N M Watson FreeBSD Core Team, TrustedBSD Projects > robert@fledge.watson.org Network Associates Laboratories > > On Mon, 30 Sep 2002, Juli Mallett wrote: > > > jmallett 2002/09/30 13:20:22 PDT > > > > Modified files: > > sys/alpha/osf1 osf1_signal.c > > sys/compat/linprocfs linprocfs.c > > sys/compat/linux linux_misc.c linux_signal.c > > sys/compat/svr4 svr4_filio.c svr4_signal.c > > sys/conf files > > sys/fs/procfs procfs_ctl.c > > sys/kern init_main.c kern_exit.c kern_fork.c > > kern_kthread.c kern_proc.c kern_sig.c > > subr_trap.c tty.c > > sys/sys proc.h > > Added files: > > sys/kern subr_sigq.c > > sys/sys ksiginfo.h > > Log: > > First half of implementation of ksiginfo, signal queues, and such. This > > gets signals operating based on a TailQ, and is good enough to run X11, > > GNOME, and do job control. There are some intricate parts which could be > > more refined to match the sigset_t versions, but those require further > > evaluation of directions in which our signal system can expand and contract > > to fit our needs. > > > > After this has been in the tree for a while, I will make in kernel API > > changes, most notably to trapsignal(9) and sendsig(9), to use ksiginfo > > more robustly, such that we can actually pass information with our > > (queued) signals to the userland. That will also result in using a > > struct ksiginfo pointer, rather than a signal number, in a lot of > > kern_sig.c, to refer to an individual pending signal queue member, but > > right now there is no defined behaviour for such. > > > > CODAFS is unfinished in this regard because the logic is unclear in > > some places. > > > > Sponsored by: New Gold Technology > > Reviewed by: bde, tjr, jake [an older version, logic similar] > > > > Revision Changes Path > > 1.20 +2 -1 src/sys/alpha/osf1/osf1_signal.c > > 1.56 +1 -1 src/sys/compat/linprocfs/linprocfs.c > > 1.131 +2 -1 src/sys/compat/linux/linux_misc.c > > 1.36 +2 -1 src/sys/compat/linux/linux_signal.c > > 1.17 +3 -0 src/sys/compat/svr4/svr4_filio.c > > 1.19 +1 -1 src/sys/compat/svr4/svr4_signal.c > > 1.707 +1 -0 src/sys/conf/files > > 1.45 +2 -1 src/sys/fs/procfs/procfs_ctl.c > > 1.208 +2 -0 src/sys/kern/init_main.c > > 1.178 +8 -1 src/sys/kern/kern_exit.c > > 1.165 +1 -0 src/sys/kern/kern_fork.c > > 1.25 +9 -8 src/sys/kern/kern_kthread.c > > 1.155 +3 -2 src/sys/kern/kern_proc.c > > 1.192 +36 -28 src/sys/kern/kern_sig.c > > 1.1 +301 -0 src/sys/kern/subr_sigq.c (new) > > 1.224 +3 -1 src/sys/kern/subr_trap.c > > 1.186 +9 -5 src/sys/kern/tty.c > > 1.1 +78 -0 src/sys/sys/ksiginfo.h (new) > > 1.261 +1 -1 src/sys/sys/proc.h > > > -- Juli Mallett <jmallett@FreeBSD.org> | FreeBSD: The Power To Serve Will break world for fulltime employment. | finger jmallett@FreeBSD.org http://people.FreeBSD.org/~jmallett/ | Support my FreeBSD hacking! To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020930145001.A44977>