Date: Thu, 19 Dec 2002 08:10:40 -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: <20021219161040.GA2952@HAL9000.homeunix.com> In-Reply-To: <200212190223.gBJ2NhD9095344@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> <20021219012606.GA2165@HAL9000.homeunix.com> <200212190223.gBJ2NhD9095344@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Thus spake Matthew Dillon <dillon@apollo.backplane.com>: > :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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021219161040.GA2952>