Date: Wed, 18 Dec 2002 17:26:06 -0800 From: David Schultz <dschultz@uclink.Berkeley.EDU> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: Eirik Nygaard <eirikn@bluezone.no>, current@FreeBSD.ORG Subject: Re: patch #3 Re: swapoff code comitted. Message-ID: <20021219012606.GA2165@HAL9000.homeunix.com> In-Reply-To: <200212182300.gBIN0ZW7094554@apollo.backplane.com> References: <200212151946.gBFJktmo090730@apollo.backplane.com> <20021215223540.GA601@unixpages.org> <200212152247.gBFMlp4d098705@apollo.backplane.com> <20021218182724.GB853@eirikn.net> <200212181918.gBIJIOIV093115@apollo.backplane.com> <20021218203854.GC853@eirikn.net> <200212182222.gBIMMsnw094344@apollo.backplane.com> <200212182300.gBIN0ZW7094554@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Looks good to me, modulo a few nits. I try not to nitpick, but
I've mentioned a few of them below. (BDE does a better job of it
than I do anyway. :-)
The patch puts identical functionality in two places, so maybe it
would make sense to rip support for -s out of pstat/swapinfo (and
integrate 'pstat -ss' support into swapctl). If we really want to
go the NetBSD way, we could even integrate the swapon(2) and
swapoff(2) into swapctl(2). Doesn't matter to me.
(BTW, when I get the chance, I'll re-run my swapoff torture tests
now that Alan Cox's new locking is in place. Chances are the
swapoff code needs to be brought up to date..)
> Index: swapon.8
> ===================================================================
> RCS file: /home/ncvs/src/sbin/swapon/swapon.8,v
> retrieving revision 1.21
> diff -u -r1.21 swapon.8
[...]
> +.Nm Swapoff
> +must move sawpped pages out of the device being removed which could
I think you have a tpyo there. ;-)
> +Swap information can be generated using the
> +.Nm swapinfo
> +program,
> +.Nm pstat
> +.Fl s ,
> +or
> +.Nm swapctl
> +.Fl lshk .
IIRC, swapinfo is just there for compatibility (it's a hard link
to pstat), so there's no need to advertise it.
> Index: swapon.c
> ===================================================================
> RCS file: /home/ncvs/src/sbin/swapon/swapon.c,v
> retrieving revision 1.13
> diff -u -r1.13 swapon.c
[...]
> + if (strstr(argv[0], "swapon"))
> + which_prog = SWAPON;
> + else if (strstr(argv[0], "swapoff"))
> + which_prog = SWAPOFF;
It's probably better to do a strcmp on strrchr(argv[0], '/') when
argv[0] contains a slash. Otherwise people will wonder why
swapoff(8) breaks when they (perhaps mistakenly) compile and run
it from the src/sbin/swapon directory.
> + while ((ch = getopt(argc, argv, "AadlhksU")) != -1) {
> + switch(ch) {
> + case 'A':
> + if (which_prog == SWAPCTL) {
> + doall = 1;
> + which_prog = SWAPON;
> + } else {
> + usage();
> + }
> + break;
[...]
The repeated 'whichprog == foo' tests can be combined into a
single test at the end of the loop.
> -
> +
?
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021219012606.GA2165>