: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
<[EMAIL PROTECTED]>
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
