From owner-freebsd-current Thu Dec 19 8:10:47 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 C255E37B401 for ; Thu, 19 Dec 2002 08:10:45 -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 DECA443EDC for ; Thu, 19 Dec 2002 08:10:44 -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 gBJGAe6I005102; Thu, 19 Dec 2002 08:10:40 -0800 (PST) (envelope-from dschultz@uclink.Berkeley.EDU) Received: (from das@localhost) by HAL9000.homeunix.com (8.12.6/8.12.5/Submit) id gBJGAeZo005101; Thu, 19 Dec 2002 08:10:40 -0800 (PST) (envelope-from dschultz@uclink.Berkeley.EDU) Date: Thu, 19 Dec 2002 08:10:40 -0800 From: David Schultz To: Matthew Dillon Cc: Eirik Nygaard , current@FreeBSD.ORG Subject: Re: patch #3 Re: swapoff code comitted. Message-ID: <20021219161040.GA2952@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> <20021219012606.GA2165@HAL9000.homeunix.com> <200212190223.gBJ2NhD9095344@apollo.backplane.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200212190223.gBJ2NhD9095344@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 Thus spake Matthew Dillon : > :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. > > I think we should keep swapon and swapoff as separate commands. > They are the most intuitive of the lot. > > NetBSD's pstat supports -s, as does OpenBSD's, so there is no reason > to rip out support for -s in our pstat. > > Neither OpenBSD or NetBSD have swapinfo that I can find. We could > potentially rip out the swapinfo command though all it is is a hardlink > to pstat so it wouldn't really save us anything. I guess I'm just bothered by the fact that it's duplicating functionality. (In particular, the part that is duplicated was working fine in pstat and doesn't need to be on the root filesystem.) But when it comes down to it, I don't have a problem as long as other people are maintaining it. > :> + 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. > > Hmm. How about a strstr on a strrchr. I don't like making exact > comparisons because it removes flexibility that someone might want > in regards to hardlinks (for example, someone might want to add a > version or other suffix to differentiate certain binaries in a test > environment or in an emulation environment). e.g. bsdswapon vs > swapon. > > Isn't there a shortcut procedure to handle the NULL return case? > I know there is one for a forward scan. I thought there was one for > the reverse scan too. > > if ((ptr = strrchr(argv[0], '/')) == NULL) > ptr = argv[0]; > if (strstr(ptr, "swapon")) > ... Sounds fine. I don't know of a more concise approach offhand, and the original version used essentially what you just wrote. (I used strcmp(), so ptr had to be incremented to skip the slash.) > :The repeated 'whichprog == foo' tests can be combined into a > :single test at the end of the loop. > > They do subtly different things so I am not sure what you mean. > You need to provide some code here. Yow, I didn't realize that -a and -d mean completely different things in swalctl vs swapon/swapoff. If that's how it has to work for compatibility, then it looks okay to me. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message