On Sunday 23 March 2014 00:33:04 Matt Whitlock wrote: > Attached are a series of three patches affecting swaponoff. > > Patch #1 fixes two small logic errors: (a) the bb_strtou function was called > inside an instantiation of the MIN macro, which would cause the function to > be called twice in the common case; and (b) the actual maximum allowable swap > priority is SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT, not simply > SWAP_FLAG_PRIO_MASK as the code previously assumed. It just so happens that > SWAP_FLAG_PRIO_SHIFT == 0, but one should not rely on that serendipity. > > Patch #2 fixes the interaction of the -a and -p options to swapon. When -p is > given on the command line, the specified priority should be applied to any > swap areas given in /etc/fstab that lack a 'pri' option. This is the way > swapon from util-linux does it, and it's the rational behavior, or else -p > and -a should be mutually exclusive options. > > Patch #3 adds a -d option to swapon, which sets SWAP_FLAG_DISCARD and > potentially SWAP_FLAG_DISCARD_ONCE or SWAP_FLAG_DISCARD_PAGES if a policy is > given. The patch also adds support for the 'discard' option in swap entries > in /etc/fstab. Hi, attached you will find a alternative patch to your patches #1 and #2 that includes your fixes and improvements and reworks swap_on_off_main to allow you to implement patch #3 without creating a #ifdef hell. I hope that the code is also easier readable than the original unpatched version.
Ciao, Tito --- util-linux/swaponoff.c.orig 2013-11-19 20:43:56.000000000 +0100 +++ util-linux/swaponoff.c 2014-03-23 20:22:56.189579044 +0100 @@ -49,6 +49,11 @@ struct globals { #endif #define INIT_G() do { } while (0) +enum { + OPT_a = (1 << 0), /* all */ + OPT_p = (1 << 1), /* priority */ +}; + static int swap_enable_disable(char *device) { int status; @@ -82,7 +87,9 @@ static int do_em_all(void) struct mntent *m; FILE *f; int err; - +#if ENABLE_FEATURE_SWAPON_PRI + int g_flags_save; +#endif f = setmntent("/etc/fstab", "r"); if (f == NULL) bb_perror_msg_and_die("/etc/fstab"); @@ -97,19 +104,27 @@ static int do_em_all(void) ) { #if ENABLE_FEATURE_SWAPON_PRI char *p; - g_flags = 0; /* each swap space might have different flags */ + /* each swap space might have different flags */ + /* if an option is given on the command line it should be applied */ + /* to any swap areas given in /etc/fstab that lack that option, */ + /* therefore we save the command line options. */ + g_flags_save = g_flags; p = hasmntopt(m, "pri"); if (p) { - /* Max allowed 32767 (==SWAP_FLAG_PRIO_MASK) */ - unsigned int swap_prio = MIN(bb_strtou(p + 4 , NULL, 10), SWAP_FLAG_PRIO_MASK); + /* Max allowed 32767 (== SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT) */ + unsigned prio = bb_strtou(p + 4, NULL, 10); /* We want to allow "NNNN,foo", thus errno == EINVAL is allowed too */ if (errno != ERANGE) { - g_flags = SWAP_FLAG_PREFER | - (swap_prio << SWAP_FLAG_PRIO_SHIFT); + g_flags = (g_flags & ~SWAP_FLAG_PRIO_MASK) | SWAP_FLAG_PREFER | + MIN(prio, SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT) << SWAP_FLAG_PRIO_SHIFT; } } #endif err += swap_enable_disable(m->mnt_fsname); +#if ENABLE_FEATURE_SWAPON_PRI + /* restore commandline options for next round */ + g_flags = g_flags_save; +#endif } } } @@ -124,24 +139,28 @@ int swap_on_off_main(int argc, char **ar int swap_on_off_main(int argc UNUSED_PARAM, char **argv) { int ret; + const char * option_str = "a"; +#if ENABLE_FEATURE_SWAPON_PRI + int prio; +#endif INIT_G(); -#if !ENABLE_FEATURE_SWAPON_PRI - ret = getopt32(argv, "a"); -#else - if (applet_name[5] == 'n') - opt_complementary = "p+"; - ret = getopt32(argv, (applet_name[5] == 'n') ? "ap:" : "a", &g_flags); + if (applet_name[5] == 'n') { + option_str = "a" IF_FEATURE_SWAPON_PRI("p:"); + if (ENABLE_FEATURE_SWAPON_PRI) + opt_complementary = "p+"; + } - if (ret & 2) { // -p + ret = getopt32(argv, option_str IF_FEATURE_SWAPON_PRI(, &prio)); +#if ENABLE_FEATURE_SWAPON_PRI + if (ret & OPT_p) { + /* priority is a value between 0 and 32767 */ g_flags = SWAP_FLAG_PREFER | - ((g_flags & SWAP_FLAG_PRIO_MASK) << SWAP_FLAG_PRIO_SHIFT); - ret &= 1; + MIN(prio, SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT) << SWAP_FLAG_PRIO_SHIFT; } #endif - - if (ret /* & 1: not needed */) // -a + if (ret & OPT_a) return do_em_all(); argv += optind;
Signed-off by Tito Ragusa <farmat...@tiscali.it> --- util-linux/swaponoff.c.orig 2013-11-19 20:43:56.000000000 +0100 +++ util-linux/swaponoff.c 2014-03-23 20:22:56.189579044 +0100 @@ -49,6 +49,11 @@ struct globals { #endif #define INIT_G() do { } while (0) +enum { + OPT_a = (1 << 0), /* all */ + OPT_p = (1 << 1), /* priority */ +}; + static int swap_enable_disable(char *device) { int status; @@ -82,7 +87,9 @@ static int do_em_all(void) struct mntent *m; FILE *f; int err; - +#if ENABLE_FEATURE_SWAPON_PRI + int g_flags_save; +#endif f = setmntent("/etc/fstab", "r"); if (f == NULL) bb_perror_msg_and_die("/etc/fstab"); @@ -97,19 +104,27 @@ static int do_em_all(void) ) { #if ENABLE_FEATURE_SWAPON_PRI char *p; - g_flags = 0; /* each swap space might have different flags */ + /* each swap space might have different flags */ + /* if an option is given on the command line it should be applied */ + /* to any swap areas given in /etc/fstab that lack that option, */ + /* therefore we save the command line options. */ + g_flags_save = g_flags; p = hasmntopt(m, "pri"); if (p) { - /* Max allowed 32767 (==SWAP_FLAG_PRIO_MASK) */ - unsigned int swap_prio = MIN(bb_strtou(p + 4 , NULL, 10), SWAP_FLAG_PRIO_MASK); + /* Max allowed 32767 (== SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT) */ + unsigned prio = bb_strtou(p + 4, NULL, 10); /* We want to allow "NNNN,foo", thus errno == EINVAL is allowed too */ if (errno != ERANGE) { - g_flags = SWAP_FLAG_PREFER | - (swap_prio << SWAP_FLAG_PRIO_SHIFT); + g_flags = (g_flags & ~SWAP_FLAG_PRIO_MASK) | SWAP_FLAG_PREFER | + MIN(prio, SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT) << SWAP_FLAG_PRIO_SHIFT; } } #endif err += swap_enable_disable(m->mnt_fsname); +#if ENABLE_FEATURE_SWAPON_PRI + /* restore commandline options for next round */ + g_flags = g_flags_save; +#endif } } } @@ -124,24 +139,28 @@ int swap_on_off_main(int argc, char **ar int swap_on_off_main(int argc UNUSED_PARAM, char **argv) { int ret; + const char * option_str = "a"; +#if ENABLE_FEATURE_SWAPON_PRI + int prio; +#endif INIT_G(); -#if !ENABLE_FEATURE_SWAPON_PRI - ret = getopt32(argv, "a"); -#else - if (applet_name[5] == 'n') - opt_complementary = "p+"; - ret = getopt32(argv, (applet_name[5] == 'n') ? "ap:" : "a", &g_flags); + if (applet_name[5] == 'n') { + option_str = "a" IF_FEATURE_SWAPON_PRI("p:"); + if (ENABLE_FEATURE_SWAPON_PRI) + opt_complementary = "p+"; + } - if (ret & 2) { // -p + ret = getopt32(argv, option_str IF_FEATURE_SWAPON_PRI(, &prio)); +#if ENABLE_FEATURE_SWAPON_PRI + if (ret & OPT_p) { + /* priority is a value between 0 and 32767 */ g_flags = SWAP_FLAG_PREFER | - ((g_flags & SWAP_FLAG_PRIO_MASK) << SWAP_FLAG_PRIO_SHIFT); - ret &= 1; + MIN(prio, SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT) << SWAP_FLAG_PRIO_SHIFT; } #endif - - if (ret /* & 1: not needed */) // -a + if (ret & OPT_a) return do_em_all(); argv += optind;
_______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox