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 <[email protected]>
--- 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
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
