From owner-freebsd-current Wed Dec 18 18:23:49 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 426AA37B401 for ; Wed, 18 Dec 2002 18:23:47 -0800 (PST) Received: from apollo.backplane.com (apollo.backplane.com [216.240.41.2]) by mx1.FreeBSD.org (Postfix) with ESMTP id B117B43EDC for ; Wed, 18 Dec 2002 18:23:45 -0800 (PST) (envelope-from dillon@apollo.backplane.com) Received: from apollo.backplane.com (localhost [127.0.0.1]) by apollo.backplane.com (8.12.5/8.12.5) with ESMTP id gBJ2NhOM095345; Wed, 18 Dec 2002 18:23:43 -0800 (PST) (envelope-from dillon@apollo.backplane.com) Received: (from dillon@localhost) by apollo.backplane.com (8.12.5/8.12.5/Submit) id gBJ2NhD9095344; Wed, 18 Dec 2002 18:23:43 -0800 (PST) (envelope-from dillon) Date: Wed, 18 Dec 2002 18:23:43 -0800 (PST) From: Matthew Dillon Message-Id: <200212190223.gBJ2NhD9095344@apollo.backplane.com> To: David Schultz Cc: Eirik Nygaard , current@FreeBSD.ORG Subject: Re: patch #3 Re: swapoff code comitted. 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> 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. 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. :(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..) I ran it across Alan and he thought it looked ok at a glance, but I agree now that it is integrated in we should take a more involved look at 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")) ... :> + 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. They do subtly different things so I am not sure what you mean. You need to provide some code here. :> - :> + : :? It's probably a space or a tab. I'll track it down. -Matt Matthew Dillon To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message