From owner-freebsd-current Wed Dec 18 17:26:23 2002 Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5BD5737B401 for ; Wed, 18 Dec 2002 17:26:21 -0800 (PST) Received: from HAL9000.homeunix.com (12-232-220-15.client.attbi.com [12.232.220.15]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9452643E4A for ; Wed, 18 Dec 2002 17:26:15 -0800 (PST) (envelope-from dschultz@uclink.Berkeley.EDU) Received: from HAL9000.homeunix.com (localhost [127.0.0.1]) by HAL9000.homeunix.com (8.12.6/8.12.5) with ESMTP id gBJ1Q66I002326; Wed, 18 Dec 2002 17:26:06 -0800 (PST) (envelope-from dschultz@uclink.Berkeley.EDU) Received: (from das@localhost) by HAL9000.homeunix.com (8.12.6/8.12.5/Submit) id gBJ1Q676002325; Wed, 18 Dec 2002 17:26:06 -0800 (PST) (envelope-from dschultz@uclink.Berkeley.EDU) Date: Wed, 18 Dec 2002 17:26:06 -0800 From: David Schultz To: Matthew Dillon Cc: Eirik Nygaard , current@FreeBSD.ORG Subject: Re: patch #3 Re: swapoff code comitted. Message-ID: <20021219012606.GA2165@HAL9000.homeunix.com> Mail-Followup-To: Matthew Dillon , Eirik Nygaard , current@FreeBSD.ORG 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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200212182300.gBIN0ZW7094554@apollo.backplane.com> Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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