Date: Sun, 29 Dec 2002 17:38:57 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: David Schultz <dschultz@uclink.Berkeley.EDU>, Eirik Nygaard <eirikn@bluezone.no>, <current@FreeBSD.ORG> Subject: Re: patch #3 Re: swapoff code comitted. Message-ID: <20021229170601.Q40414-100000@gamplex.bde.org> In-Reply-To: <200212190223.gBJ2NhD9095344@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[Reply to old mail] On Wed, 18 Dec 2002, Matthew Dillon wrote: > :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. Fine with me. I think I'd prefer them to be in separate source files too. That would be cleaner and I would find it easier to not see the style bugs in the new code. > 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. swapinfo is compatibility cruft for 386BSD-0.x through FreeBSD-1.x. pstat -s is the tradional place for this, and the other BSDs apparently dropped the compatibility cruft when they started using it. swapinfo is hard to drop now that it has been in FreeBSD for so long. I'd prefer not to have swapctl. It is just compatibibility cruft for NetBSD and OpenBSD. We have nothing interesting to control except on and off, which are more intuitively controlled by swapon and swapoff. > :[...] > :> + 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. I disagree with having this flexibility in programs whose behaviour depends on their name. > 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")) > ... Isn't basename(3) supposed to be used for things like this? Examples of existing practice: chown.c: - Uses home made basename. - Uses strcmp() with "chown", so the program acts as chown(8) if its basename is precisely "chown" and as chgrp(8) otherwise. - The strcmp() statement is missing style bugs. rmextattr.c: - Uses basename(3). - Uses strcmp() with the 4 valid variants and a usage message if no match, so the program name must match precisely. - The strcmp() statements use the style bug !strcmp(). pstat.c: - Like chown.c except the strcmp() statement uses the style bug !strcmp(). Bruce 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?20021229170601.Q40414-100000>