swapoff code comitted.
Matthew Dillon
as being highly experimental (and it still needs to be vetted for
VM locking changes and other recent changes in -current). A considerable
amount of testing has been done already but -current is a moving target.
I am tentitively planning on MFCing the code in a few weeks (some time
after christmas) but it will depend on my schedule. If someone else
wants to take on that work I will would be happy to act as a reviewer.
-Matt
Matthew Dillon
<dil...@backplane.com>
To Unsubscribe: send mail to majo...@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Christian Brueffer
> David Schultz's swapoff code has been comitted. It should be regarded
> as being highly experimental (and it still needs to be vetted for
> VM locking changes and other recent changes in -current). A considerable
> amount of testing has been done already but -current is a moving target.
>
> I am tentitively planning on MFCing the code in a few weeks (some time
> after christmas) but it will depend on my schedule. If someone else
> wants to take on that work I will would be happy to act as a reviewer.
>
> -Matt
> Matthew Dillon
> <dil...@backplane.com>
>
How about renaming swapon(8) into swapctl(8) after this function enhancement?
This name reflects it's purpose much better and would be consistent with the
other BSDs.
- Christian
--
http://www.unixpages.org ch...@unixpages.org
GPG Pub-Key : www.unixpages.org/cbrueffer.asc
GPG Fingerprint: A5C8 2099 19FF AACA F41B B29B 6C76 178C A0ED 982D
GPG Key ID : 0xA0ED982D
Matthew Dillon
:How about renaming swapon(8) into swapctl(8) after this function enhancemen=
:t?
:This name reflects it's purpose much better and would be consistent with the
:other BSDs.
:
:- Christian
I think that's an excellent idea. We would have to do some
rewriting to add the expected options but it would not be too
difficult to do. Mainly just grunt work. e.g. the NetBSD swapctl
command is this:
swapctl -A [-p priority] [-t blk|noblk]
-D dumpdev
-U [-t blk|noblk]
-a [-p priority] path
-c -p priority path
-d path
-l | -s [-k]
-z
swapon -a | path
And the OpenBSD swapctl command is this:
swapctl -A [-p priority] [-t blk|noblk]
swapctl -a [-p priority] path
swapctl -c -p priority path
swapctl -d path
swapctl -l | -s [-k]
swapon -a | path
We would simply ignore priority since we don't support it, nor would we
need a -t option since we do not have buffered block devices any more
or a -c option since, again, we do not have priorities.
I would keep 'swapoff' in anycase. It's an obvious command and like
swapon could simply wind up being a hardlink to swapctl.
I am not volunteering to do this, at least not right now. I have too
big a stack of things that still need to be committed, but if someone
else would like to tackle this I think it would be a nice little project
for a developer with some free time to waste and I would be happy to
review and test the work.
-Matt
David Schultz
> How about renaming swapon(8) into swapctl(8) after this function enhancement?
> This name reflects it's purpose much better and would be consistent with the
> other BSDs.
It would be trivial to change the name, although I don't see what
it buys you. NetBSD changed the name because they wanted to be
able to set swap device priority and do other things through the
same interface. From previous conversations, I gather that swap
device priority is not a particularly desired feature for FreeBSD,
and it would probably require rewriting the entire swap subsystem
again in any case. Unless you intend to extend the interface
further, having swapctl seems to me like changing mount/umount to
mountctl. That's not to say I'm opposed to the idea, just that I
don't care one way or the other.
Matthew Dillon
:I have made a small patch, added l, s and h switches to show
:information about the swap devices. And the U switch to swapctl only
:to remove all activated swap devices.
:If anything else is needed let me know and I will add it.
:
:--=20
:
:Eirik Nygaard <eir...@bluezone.no>
:PGP Key: 83C55EDE
That is a pretty good first attempt. I have a few suggests and found
one bug. First the bug:
:+ is_swapctl ? "lsU" : "");
I think that was supposed to be a call to is_swapctl, not a pointer
to the function.
Suggestions: Get rid of the is_swap*() functions and instead use
av[0] at the top of main() and use strstr() to determine if the
program is swapon, swapoff, or swapctl. Check against "swapon" and
"swapoff" and if it is neither then default to swapctl (don't test
against "swapctl"). Store which program it is in a global variable,
e.g. an enum like this:
enum { SWAPON, SWAPOFF, SWAPCTL } which_prog = SWAPCTL;
...
main(...)
{
if (strstr(av[0], "swapon"))
which_prog = SWAPON;
else if (strstr(av[0], "swapoff"))
which_prog = SWAPOFF;
...
}
In regards to retrieving swap information, in -current there is a
sysctl() to do it. Take a look at /usr/src/usr.sbin/pstat/pstat.c
(in the current source tree), at the swapmode_kvm() and swapmode_sysctl()
functions. The sysctl is much, much faster then the kvm call because
the kvm call has to run through the swap radix tree to collect the useage
information.
-Matt
Matthew Dillon
<dil...@backplane.com>
Matthew Dillon
:sysctl to get the swap information.
All right, I found a couple more bugs and fleshed it out a bit.
You got your LINKS and MLINKS reversed and forgot a +=,
you forgot to initialize the do_swapoff variable in swap_on_off(),
and you reuse 'total' and 'used' in swaplist() in a manner which breaks
the -s option.
I have included an updated patch below based on these fixes and a few
other minor cleanups. I also changed the block size for -h from 1000
to 1024 bytes to make it consistent with pstat -s and friends (and
also OpenBSD and NetBSD).
I also added -A, -U, cleaned up usage(), and made the options conform
to NetBSD and OpenBSD.
The only thing really missing is for us to handle the BLOCKSIZE
environment variable like 'df' does, and appropriate additions to
the manual page (which I would be happy to do). Once we get those in
I will commit it.
Here is an updated patch.
-Matt
Index: Makefile
===================================================================
RCS file: /home/ncvs/src/sbin/swapon/Makefile,v
retrieving revision 1.7
diff -u -r1.7 Makefile
--- Makefile 15 Dec 2002 19:17:56 -0000 1.7
+++ Makefile 18 Dec 2002 21:31:41 -0000
@@ -4,6 +4,8 @@
PROG= swapon
MAN= swapon.8
LINKS= ${BINDIR}/swapon ${BINDIR}/swapoff
+LINKS+= ${BINDIR}/swapon ${BINDIR}/swapctl
MLINKS= swapon.8 swapoff.8
+MLINKS+=swapon.8 swapctl.8
.include <bsd.prog.mk>
Index: swapon.c
===================================================================
RCS file: /home/ncvs/src/sbin/swapon/swapon.c,v
retrieving revision 1.13
diff -u -r1.13 swapon.c
--- swapon.c 15 Dec 2002 19:17:56 -0000 1.13
+++ swapon.c 18 Dec 2002 22:20:42 -0000
@@ -45,6 +45,11 @@
"$FreeBSD: src/sbin/swapon/swapon.c,v 1.13 2002/12/15 19:17:56 dillon Exp $";
#endif /* not lint */
+#include <sys/stat.h>
+#include <sys/param.h>
+#include <sys/user.h>
+#include <sys/sysctl.h>
+
#include <err.h>
#include <errno.h>
#include <fstab.h>
@@ -52,10 +57,13 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
+#include <fcntl.h>
+
+static void usage(void);
+static int swap_on_off(char *name, int ignoreebusy);
+static void swaplist(int, int, int);
-static void usage(const char *);
-static int is_swapoff(const char *);
-int swap_on_off(char *name, int ignoreebusy, int do_swapoff);
+enum { SWAPON, SWAPOFF, SWAPCTL } orig_prog, which_prog = SWAPCTL;
int
main(int argc, char **argv)
@@ -63,48 +71,105 @@
struct fstab *fsp;
int stat;
int ch, doall;
- int do_swapoff;
- char *pname = argv[0];
-
- do_swapoff = is_swapoff(pname);
-
+ int sflag = 0, lflag = 0, hflag = 0;
+
+ if (strstr(argv[0], "swapon"))
+ which_prog = SWAPON;
+ else if (strstr(argv[0], "swapoff"))
+ which_prog = SWAPOFF;
+ orig_prog = which_prog;
+
doall = 0;
- while ((ch = getopt(argc, argv, "a")) != -1)
- switch((char)ch) {
+ while ((ch = getopt(argc, argv, "AadlhksU")) != -1) {
+ switch(ch) {
+ case 'A':
+ if (which_prog == SWAPCTL) {
+ doall = 1;
+ which_prog = SWAPON;
+ } else {
+ usage();
+ }
+ break;
case 'a':
- doall = 1;
+ if (which_prog == SWAPON || which_prog == SWAPOFF)
+ doall = 1;
+ else
+ which_prog = SWAPON;
+ break;
+ case 'd':
+ if (which_prog == SWAPCTL)
+ which_prog = SWAPOFF;
+ else
+ usage();
+ break;
+ case 's':
+ sflag = 1;
+ break;
+ case 'l':
+ lflag = 1;
+ break;
+ case 'h':
+ hflag = 'M';
+ break;
+ case 'k':
+ hflag = 'K';
+ break;
+ case 'U':
+ if (which_prog == SWAPCTL) {
+ doall = 1;
+ which_prog = SWAPOFF;
+ } else {
+ usage();
+ }
break;
case '?':
default:
- usage(pname);
+ usage();
}
+ }
argv += optind;
-
+
stat = 0;
- if (doall)
- while ((fsp = getfsent()) != NULL) {
- if (strcmp(fsp->fs_type, FSTAB_SW))
- continue;
- if (strstr(fsp->fs_mntops, "noauto"))
- continue;
- if (swap_on_off(fsp->fs_spec, 1, do_swapoff))
+ if (which_prog == SWAPON || which_prog == SWAPOFF) {
+ if (doall) {
+ while ((fsp = getfsent()) != NULL) {
+ if (strcmp(fsp->fs_type, FSTAB_SW))
+ continue;
+ if (strstr(fsp->fs_mntops, "noauto"))
+ continue;
+ if (swap_on_off(fsp->fs_spec, 0)) {
+ stat = 1;
+ } else {
+ printf("%s: %sing %s as swap device\n",
+ getprogname(), which_prog == SWAPOFF ? "remov" : "add",
+ fsp->fs_spec);
+ }
+ }
+ }
+ else if (!*argv)
+ usage();
+ for (; *argv; ++argv) {
+ if (swap_on_off(*argv, 0)) {
stat = 1;
- else
+ } else if (orig_prog == SWAPCTL) {
printf("%s: %sing %s as swap device\n",
- pname, do_swapoff ? "remov" : "add",
- fsp->fs_spec);
+ getprogname(), which_prog == SWAPOFF ? "remov" : "add",
+ *argv);
+ }
}
- else if (!*argv)
- usage(pname);
- for (; *argv; ++argv)
- stat |= swap_on_off(*argv, 0, do_swapoff);
+ } else {
+ if (lflag || sflag)
+ swaplist(lflag, sflag, hflag);
+ else
+ usage();
+ }
exit(stat);
}
-int
-swap_on_off(char *name, int ignoreebusy, int do_swapoff)
+static int
+swap_on_off(char *name, int ignoreebusy)
{
- if ((do_swapoff ? swapoff(name) : swapon(name)) == -1) {
+ if ((which_prog == SWAPOFF ? swapoff(name) : swapon(name)) == -1) {
switch (errno) {
case EBUSY:
if (!ignoreebusy)
@@ -120,23 +185,84 @@
}
static void
-usage(const char *pname)
+usage(void)
{
- fprintf(stderr, "usage: %s [-a] [special_file ...]\n", pname);
+ fprintf(stderr, "usage: %s ", getprogname());
+ switch(orig_prog) {
+ case SWAPOFF:
+ fprintf(stderr, "[-a] [special_file ...]\n");
+ break;
+ case SWAPON:
+ fprintf(stderr, "[-a] [special_file ...]\n");
+ break;
+ case SWAPCTL:
+ fprintf(stderr, "[-lshAU] [-a special_file ...]\n");
+ break;
+ }
exit(1);
}
-static int
-is_swapoff(const char *s)
+/* Some code is based on the code in the swapmode_sysctl command in pstat */
+static void
+swaplist(int lflag, int sflag, int hflag)
{
- const char *u;
-
- if ((u = strrchr(s, '/')) != NULL)
- ++u;
- else
- u = s;
- if (strcmp(u, "swapoff") == 0)
- return 1;
- else
- return 0;
+ size_t mibsize, size;
+ struct xswdev xsw;
+ int mib[16], n, pagesize, blocksize;
+ long long total = 0;
+ long long used = 0;
+ long long tmp_total;
+ long long tmp_used;
+
+ pagesize = getpagesize();
+ switch(hflag) {
+ case 'K':
+ blocksize = 1024;
+ break;
+ case 'M':
+ blocksize = 1024 * 1024;
+ break;
+ default:
+ blocksize = 1024; /* default block size */
+ hflag = 'K';
+ break;
+ }
+
+ mibsize = sizeof mib / sizeof mib[0];
+ if (sysctlnametomib("vm.swap_info", mib, &mibsize) == -1)
+ err(1, "sysctlnametomib()");
+
+ if (lflag)
+ printf("%-*s %11s %11s\n",
+ 13, "Device:",
+ "Total:", "Used:");
+
+ for (n = 0; ; ++n) {
+ mib[mibsize] = n;
+ size = sizeof xsw;
+ if (sysctl(mib, mibsize + 1, &xsw, &size, NULL, NULL) == -1)
+ break;
+ if (xsw.xsw_version != XSWDEV_VERSION)
+ errx(1, "xswdev version mismatch");
+
+ tmp_total = (long long)xsw.xsw_nblks * pagesize / blocksize;
+ tmp_used = (long long)xsw.xsw_used * pagesize / blocksize;
+ total += tmp_total;
+ used += tmp_used;
+ if (lflag) {
+ printf("/dev/%-8s %10lld%c %10lld%c\n",
+ devname(xsw.xsw_dev, S_IFCHR),
+ tmp_total, hflag,
+ tmp_used, hflag);
+ }
+ }
+ if (errno != ENOENT)
+ err(1, "sysctl()");
+
+ if (sflag)
+ printf("Total: %10lld%c %10lld%c\n",
+ total, hflag,
+ used, hflag);
+
}
+
Matthew Dillon
support, and updated the manual page. It looks quite nice now.
-Matt
Index: Makefile
===================================================================
RCS file: /home/ncvs/src/sbin/swapon/Makefile,v
retrieving revision 1.7
diff -u -r1.7 Makefile
--- Makefile 15 Dec 2002 19:17:56 -0000 1.7
+++ Makefile 18 Dec 2002 21:31:41 -0000
@@ -4,6 +4,8 @@
PROG= swapon
MAN= swapon.8
LINKS= ${BINDIR}/swapon ${BINDIR}/swapoff
+LINKS+= ${BINDIR}/swapon ${BINDIR}/swapctl
MLINKS= swapon.8 swapoff.8
+MLINKS+=swapon.8 swapctl.8
.include <bsd.prog.mk>
Index: swapon.8
===================================================================
RCS file: /home/ncvs/src/sbin/swapon/swapon.8,v
retrieving revision 1.21
diff -u -r1.21 swapon.8
--- swapon.8 15 Dec 2002 19:17:56 -0000 1.21
+++ swapon.8 18 Dec 2002 22:46:01 -0000
@@ -43,45 +43,101 @@
.Fl a
.Nm swap[on|off]
.Ar special_file ...
+.Nm swapctl
+.Fl lshk
+.Nm swapctl
+.Fl AU
+.Nm swapctl
+.Fl a
+.Ar special_file ...
+.Nm swapctl
+.Fl d
+.Ar special_file ...
.Sh DESCRIPTION
The
+.Nm swap[on,off,ctl]
+utilties are used to control swap devices in the system. At boot time all
+swap entries in
+.Pa /etc/fstab
+are added automatically when the system goes multi-user.
+Swap devices are interleaved and kernels are typically configured
+to handle a maximum of 4 swap devices. There is no priority mechanism.
+.Pp
+The
.Nm swapon
-utility is used to specify additional devices on which paging and swapping
-are to take place.
-The system begins by swapping and paging on only a single device
-so that only one disk is required at bootstrap time.
-Calls to
-.Nm swapon
-normally occur in the system multi-user initialization file
-.Pa /etc/rc
-making all swap devices available, so that the paging and swapping
-activity is interleaved across several devices.
+utility adds the specified swap devices to the system. If the
+.Fl a
+option is used, all swap devices in
+.Pa /etc/fstab
+will be added, unless their ``noauto'' option is also set.
.Pp
The
.Nm swapoff
-utility disables paging and swapping on a device.
-Calls to
+utility removes the specified swap devices from the system. If the
+.Fl a
+option is used, all swap devices in
+.Pa /etc/fstab
+will be removed, unless their ``noauto'' option is also set.
+Note that
.Nm swapoff
-succeed only if disabling the device would leave enough
-remaining virtual memory to accomodate all running programs.
+will fail and refuse to remove a swap device if there is insufficient
+VM (memory + remaining swap devices) to run the system.
+.Nm Swapoff
+must move sawpped pages out of the device being removed which could
+lead to high system loads for a period of time, depending on how
+much data has been swapped out to that device.
.Pp
-Normally, the first form is used:
-.Bl -tag -width indent
-.It Fl a
-All devices marked as ``sw''
-swap devices in
+The
+.Nm swapctl
+utility exists primarily for those familiar with other BSDs and may be
+used to add, remove, or list swap. Note that the
+.Fl a
+option is used diferently in
+.Nm swapctl
+and indicates that a specific list of devices should be added.
+The
+.Fl d
+option indicates that a specific list should be removed. The
+.Fl A
+and
+.Fl D
+options to
+.Nm swapctl
+operate on all swap entries in
.Pa /etc/fstab
-are added to or removed from the pool of available swap
-unless their ``noauto'' option is also set.
-.El
+which do not have their ``noauto'' option set.
+.Pp
+Swap information can be generated using the
+.Nm swapinfo
+program,
+.Nm pstat
+.Fl s ,
+or
+.Nm swapctl
+.Fl lshk .
+The
+.Nm swapctl
+utility has the following options for listing swap:
+.Bl -tag -width indent
+.It Fl l
+List the devices making up system swap.
+.It Fl s
+Print a summary line for system swap.
+.It Fl h
+Output values in megabytes.
+.It Fl k
+Output values in kilobytes.
.Pp
-The second form is used to configure or disable individual devices.
+The BLOCKSIZE environment variable is used if not specifically
+overridden. 512 byte blocks are used by default.
+.El
.Sh SEE ALSO
.Xr swapon 2 ,
.Xr fstab 5 ,
.Xr init 8 ,
.Xr mdconfig 8 ,
.Xr pstat 8 ,
+.Xr swapinfo 8 ,
.Xr rc 8
.Sh FILES
.Bl -tag -width "/dev/{ad,da}?s?b" -compact
Index: swapon.c
===================================================================
RCS file: /home/ncvs/src/sbin/swapon/swapon.c,v
retrieving revision 1.13
diff -u -r1.13 swapon.c
--- swapon.c 15 Dec 2002 19:17:56 -0000 1.13
+++ swapon.c 18 Dec 2002 22:53:52 -0000
@@ -120,23 +185,90 @@
}
static void
-usage(const char *pname)
+usage(void)
{
- fprintf(stderr, "usage: %s [-a] [special_file ...]\n", pname);
+ fprintf(stderr, "usage: %s ", getprogname());
+ switch(orig_prog) {
+ case SWAPOFF:
+ fprintf(stderr, "[-a] [special_file ...]\n");
+ break;
+ case SWAPON:
+ fprintf(stderr, "[-a] [special_file ...]\n");
+ break;
+ case SWAPCTL:
+ fprintf(stderr, "[-lshAU] [-a special_file ...]\n");
+ break;
+ }
exit(1);
}
-static int
-is_swapoff(const char *s)
+static void
+swaplist(int lflag, int sflag, int hflag)
{
- const char *u;
-
- if ((u = strrchr(s, '/')) != NULL)
- ++u;
- else
- u = s;
- if (strcmp(u, "swapoff") == 0)
- return 1;
- else
- return 0;
+ size_t mibsize, size;
+ struct xswdev xsw;
+ int mib[16], n, pagesize;
+ size_t hlen;
+ long blocksize;
+ long long total = 0;
+ long long used = 0;
+ long long tmp_total;
+ long long tmp_used;
+
+ pagesize = getpagesize();
+ switch(hflag) {
+ case 'K':
+ blocksize = 1024;
+ hlen = 10;
+ break;
+ case 'M':
+ blocksize = 1024 * 1024;
+ hlen = 10;
+ break;
+ default:
+ getbsize(&hlen, &blocksize);
+ break;
+ }
+
+ mibsize = sizeof mib / sizeof mib[0];
+ if (sysctlnametomib("vm.swap_info", mib, &mibsize) == -1)
+ err(1, "sysctlnametomib()");
+
+ if (lflag) {
+ char buf[32];
+ snprintf(buf, sizeof(buf), "%ld-blocks", blocksize);
+ printf("%-13s %*s %*s\n",
+ "Device:",
+ hlen, buf,
+ hlen, "Used:");
+ }
+
+ for (n = 0; ; ++n) {
+ mib[mibsize] = n;
+ size = sizeof xsw;
+ if (sysctl(mib, mibsize + 1, &xsw, &size, NULL, NULL) == -1)
+ break;
+ if (xsw.xsw_version != XSWDEV_VERSION)
+ errx(1, "xswdev version mismatch");
+
+ tmp_total = (long long)xsw.xsw_nblks * pagesize / blocksize;
+ tmp_used = (long long)xsw.xsw_used * pagesize / blocksize;
+ total += tmp_total;
+ used += tmp_used;
+ if (lflag) {
+ printf("/dev/%-8s %*lld %*lld\n",
+ devname(xsw.xsw_dev, S_IFCHR),
+ hlen, tmp_total,
+ hlen, tmp_used);
+ }
+ }
+ if (errno != ENOENT)
+ err(1, "sysctl()");
+
+ if (sflag) {
+ printf("Total: %*lld %*lld\n",
+ hlen, total,
+ hlen, used);
Bruce Evans
> Here's another update. I cleaned things up even more, add BLOCKSIZE
> support, and updated the manual page. It looks quite nice now.
I still dislike it. It starts by adding style bugs to the Makefile
(changing "=" to "+=" for the initial assignments to LINKS and MLINKS)
and doesn't get any better.
Bruce
David Schultz
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.
(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..)
> Index: swapon.8
> ===================================================================
> RCS file: /home/ncvs/src/sbin/swapon/swapon.8,v
> retrieving revision 1.21
> diff -u -r1.21 swapon.8
[...]
> +.Nm Swapoff
> +must move sawpped pages out of the device being removed which could
I think you have a tpyo there. ;-)
> +Swap information can be generated using the
> +.Nm swapinfo
> +program,
> +.Nm pstat
> +.Fl s ,
> +or
> +.Nm swapctl
> +.Fl lshk .
IIRC, swapinfo is just there for compatibility (it's a hard link
to pstat), so there's no need to advertise it.
> Index: swapon.c
> ===================================================================
> RCS file: /home/ncvs/src/sbin/swapon/swapon.c,v
> retrieving revision 1.13
> diff -u -r1.13 swapon.c
[...]
> + 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.
> + while ((ch = getopt(argc, argv, "AadlhksU")) != -1) {
> + switch(ch) {
> + case 'A':
> + 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.
> -
> +
?
Matthew Dillon
: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
<dil...@backplane.com>
Matthew Dillon
:
:On Wed, 18 Dec 2002, Matthew Dillon wrote:
:
:> Here's another update. I cleaned things up even more, add BLOCKSIZE
:> support, and updated the manual page. It looks quite nice now.
:
:I still dislike it. It starts by adding style bugs to the Makefile
:(changing "=" to "+=" for the initial assignments to LINKS and MLINKS)
:and doesn't get any better.
:
:Bruce
It hasn't been committed yet so please feel free to email me
patches for stylistic issues over the next day or two.
In regards to having a swapctl command at all, I think anything
that offers familiarity between the BSDs is a good thing to have.
If anything we should remove 'swapinfo'. 'pstat -s' is actually
shorter and easier to type :-)
-Matt
Matthew Dillon
<dil...@backplane.com>
David Schultz
> :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.
Matthew Dillon
in regards to your original nits, but feel free to clean the code up
now that it has been comitted. All other bullets have been taken care
of.
I'm still on the fence in regards to backporting it. I would like to,
but do not have the time at the moment.
-Matt
Bruce Evans
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
Bruce Evans
> The swapctl code has been comitted. Bruce, you never supplied feedback
> in regards to your original nits, but feel free to clean the code up
> now that it has been comitted. All other bullets have been taken care
> of.
I'm not sure if I want to get that involved. It has enough nits to form
a few gnats. I most recently noticed misindented case statements and
printf format errors which generate mail from non-i386 tinderboxes.
Bruce