On Tue, 3 Mar 2009, Karel Zak wrote:
> The patch:
>
> commit 3399a218f4eff4016a22044e7c416521bc37c53c
> Author: Matthias Koenig <mkoenig@xxxxxxx>
> Date: Thu Nov 27 12:32:56 2008 +0100
> swapon: add swap format detection and pagesize check
>
> introduced a new feature. This feature should be optional (disabled by
> default) to keep happy people who use swap-space bad blocks or
> nonstandard swap-space size.
>
> CC: Hugh Dickins <hugh@xxxxxxxxxxx>
> CC: Olaf Hering <olh@xxxxxxx>
> CC: Matthias Koenig <mkoenig@xxxxxxx>
> Signed-off-by: Karel Zak <kzak@xxxxxxxxxx>
> ---
Yes, this looks good to me: how it should be done, thank you.
A couple of quibbles on wording below.
Hugh
> mount/swapon.8 | 8 ++++++++
> mount/swapon.c | 29 ++++++++++++++++++++---------
> 2 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/mount/swapon.8 b/mount/swapon.8
> index 6fc6571..66adc87 100644
> --- a/mount/swapon.8
> +++ b/mount/swapon.8
> @@ -54,6 +54,7 @@ Enable/disable:
> .br
> .in +5
> .B swapon
> +.RB [ \-f ]
> .RB [ \-p
> .IR priority ]
> .RB [ \-v ]
> @@ -73,6 +74,7 @@ Enable/disable all:
> .in +5
> .B swapon \-a
> .RB [ \-e ]
> +.RB [ \-f ]
> .RB [ \-v ]
> .br
> .B swapoff \-a
> @@ -116,6 +118,12 @@ Devices that are already being used as swap are silently skipped.
> .B "\-e, \-\-ifexists"
> Silently skip devices that do not exist.
> .TP
> +.B "\-f, \-\-fixpgsz"
> +Reinitialize (exec /sbin/mkswap) the swap space when the page size mismatch between
"the page size mismatch" sounds too much like that's normal. "a page size
mismatch" would be better, but I think it's even clearer to say:
Reinitialize (exec /sbin/mkswap) the swap space if its page size does not
match that of the the current running kernel. ...
> +the current running kernel and swap space is detected. Note that in such a case
> +.B mkswap(2)
> +initializes the whole device and does not check for bad blocks.
Good, thanks for making that point.
> +.TP
> .B \-h, \-\-help
> Provide help.
> .TP
> diff --git a/mount/swapon.c b/mount/swapon.c
> index b795ba0..a6726cd 100644
> --- a/mount/swapon.c
> +++ b/mount/swapon.c
> @@ -56,6 +56,7 @@ int priority = -1; /* non-prioritized swap by default */
>
> /* If true, don't complain if the device/file doesn't exist */
> int ifexists;
> +int fixpgsz;
>
> int verbose;
> char *progname;
> @@ -65,6 +66,7 @@ static struct option longswaponopts[] = {
> { "priority", required_argument, 0, 'p' },
> { "ifexists", 0, 0, 'e' },
> { "summary", 0, 0, 's' },
> + { "fixpgsz", 0, 0, 'f' },
> /* also for swapoff */
> { "all", 0, 0, 'a' },
> { "help", 0, 0, 'h' },
> @@ -73,7 +75,7 @@ static struct option longswaponopts[] = {
> { NULL, 0, 0, 0 }
> };
>
> -static struct option *longswapoffopts = &longswaponopts[3];
> +static struct option *longswapoffopts = &longswaponopts[4];
>
> static int cannot_find(const char *special);
>
> @@ -88,8 +90,8 @@ static int cannot_find(const char *special);
> static void
> swapon_usage(FILE *fp, int n) {
> fprintf(fp, _("\nUsage:\n"
> - " %1$s -a [-e] [-v] enable all swaps from /etc/fstab\n"
> - " %1$s [-p priority] [-v] <special> enable given swap\n"
> + " %1$s -a [-e] [-v] [-f] enable all swaps from /etc/fstab\n"
> + " %1$s [-p priority] [-v] [-f] <special> enable given swap\n"
> " %1$s -s display swap usage summary\n"
> " %1$s -h display help\n"
> " %1$s -V display version\n\n"), progname);
> @@ -193,6 +195,8 @@ swap_reinitialize(const char *device) {
> char *cmd[7];
> int idx=0;
>
> + warnx(_("%s: reinitializing the swap."), device);
> +
> switch((pid=fork())) {
> case -1: /* fork error */
> warn(_("fork failed"));
> @@ -407,11 +411,15 @@ swapon_checks(const char *special)
> " than actual size of swapspace"),
> special, swapsize);
> } else if (getpagesize() != pagesize) {
> - warn(_("%s: swap format pagesize does not match."
> - " Reinitializing the swap."),
> - special);
> - if (swap_reinitialize(special) < 0)
> - goto err;
> + if (fixpgsz) {
> + warn(_("%s: swap format pagesize does not match."),
> + special);
> + if (swap_reinitialize(special) < 0)
> + goto err;
> + } else
> + warn(_("%s: swap format pagesize does not match. "
> + "(may try --fixpgsz to reinitialize)"),
"(may try ...)" sounds to me a bit as if it might choose to do that itself.
I think "Use --fixpgsz to reinitialize it." sounds better.
I'm out of the habit of userspace programming, so unfamiliar with warn()
and warnx(), but wondering if the use of warn() here is correct - doesn't
it append an errno message, yet errno won't be relevant?
> + special);
> }
> } else if (sig == SIG_SWSUSPEND) {
> /* We have to reinitialize swap with old (=useless) software suspend
> @@ -601,7 +609,7 @@ main_swapon(int argc, char *argv[]) {
> int status = 0;
> int c, i;
>
> - while ((c = getopt_long(argc, argv, "ahep:svVL:U:",
> + while ((c = getopt_long(argc, argv, "ahefp:svVL:U:",
> longswaponopts, NULL)) != -1) {
> switch (c) {
> case 'a': /* all */
> @@ -622,6 +630,9 @@ main_swapon(int argc, char *argv[]) {
> case 'e': /* ifexists */
> ifexists = 1;
> break;
> + case 'f':
> + fixpgsz = 1;
> + break;
> case 's': /* status report */
> status = display_summary();
> exit(status);
> --
> 1.6.0.6
--
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
