Date: Wed, 18 Dec 2002 18:23:43 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: David Schultz <dschultz@uclink.Berkeley.EDU> Cc: Eirik Nygaard <eirikn@bluezone.no>, current@FreeBSD.ORG Subject: Re: patch #3 Re: swapoff code comitted. Message-ID: <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>
next in thread | previous in thread | raw e-mail | index | archive | help
: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
<dillon@backplane.com>
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?200212190223.gBJ2NhD9095344>