Date: Mon, 30 Sep 2002 17:30:46 -0400 (EDT) From: Robert Watson <rwatson@FreeBSD.org> To: Juli Mallett <jmallett@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: <Pine.NEB.3.96L.1020930172114.15622W-100000@fledge.watson.org> In-Reply-To: <200209302020.g8UKKMRt092756@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 > 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?Pine.NEB.3.96L.1020930172114.15622W-100000>