[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] add discard option -d to swapon
From:       Tito <farmatito () tiscali ! it>
Date:       2014-03-24 7:08:04
Message-ID: 201403240808.04219.farmatito () tiscali ! it
[Download RAW message or body]

On Monday 24 March 2014 01:09:01 you wrote:
> On Monday, 24 March 2014, at 12:06 am, Tito wrote:
> > On Sunday 23 March 2014 23:09:59 you wrote:
> > > This is still going to create a "hell" when adding the discard flag. Consider \
> > > what will happen in the enum: if DISCARD is enabled but PRI is disabled, then \
> > > OPT_d will be (1 << 1), but if both are enabled, then OPT_p will be (1 << 1) \
> > > and OPT_d will be (1 << 2) (or vice versa). Imagine adding five more optional \
> > > features. It leads to a combinatorial explosion. It's cleaner to downshift the \
> > > bitmap after checking each bit within its appropriate #if section.
> > 
> > The problem with flags could be solved easily, take a look at how it is done in \
> > df.c: 
> > 	enum {
> > 		OPT_KILO  = (1 << 0),
> > 		OPT_POSIX = (1 << 1),
> > 		OPT_ALL   = (1 << 2) * ENABLE_FEATURE_DF_FANCY,
> > 		OPT_INODE = (1 << 3) * ENABLE_FEATURE_DF_FANCY,
> > 		OPT_BSIZE = (1 << 4) * ENABLE_FEATURE_DF_FANCY,
> > 		OPT_HUMAN = (1 << (2 + 3*ENABLE_FEATURE_DF_FANCY)) * \
> > ENABLE_FEATURE_HUMAN_READABLE,  OPT_MEGA  = (1 << (3 + \
> > 3*ENABLE_FEATURE_DF_FANCY)) * ENABLE_FEATURE_HUMAN_READABLE,  };
> > 
> > that way the enum is independent from the combination of options enabled and most \
> > #ifdefs could be removed and we are lucky  becuase we have only 3 flags.   ;-)
> 
> That would start to get quite messy as the number of options increases:
> 
> 	enum {
> 		OPT_a = (1 << 0),	/* all */
> 		OPT_d = (1 << 1) * ENABLE_FEATURE_SWAPON_DISCARD,
> 		OPT_f = (1 << (1 + ENABLE_FEATURE_SWAPON_DISCARD)) * \
> ENABLE_FEATURE_SWAPON_FIXPGSZ,  OPT_p = (1 << (1 + ENABLE_FEATURE_SWAPON_DISCARD + \
> ENABLE_FEATURE_SWAPON_FIXPGSZ)) * ENABLE_FEATURE_SWAPON_PRI,  OPT_s = (1 << (1 + \
> ENABLE_FEATURE_SWAPON_DISCARD + ENABLE_FEATURE_SWAPON_FIXPGSZ + \
> ENABLE_FEATURE_SWAPON_PRI)) * ENABLE_FEATURE_SWAPON_SUMMARY  };
> 
> Bleh.
> 
> I prefer this approach:
> 
> 	ret = getopt32(argv,
> 		IF_FEATURE_SWAPON_DISCARD("d::")
> 		IF_FEATURE_SWAPON_FIXPGSZ("f")
> 		IF_FEATURE_SWAPON_PRI("p:")
> 		IF_FEATURE_SWAPON_SUMMARY("s")
> 		"a"
> 		IF_FEATURE_SWAPON_DISCARD(, &discard)
> 		IF_FEATURE_SWAPON_PRI(, &prio)
> 	);
> #if ENABLE_FEATURE_SWAPON_DISCARD
> 	if (ret & 1) { // -d
> 		/* ...handle discard option... */
> 	}
> 	ret >>= 1;
> #endif
> #if ENABLE_FEATURE_SWAPON_FIXPGSZ
> 	if (ret & 1) { // -f
> 		/* ...handle fixpgsz option... */
> 	}
> 	ret >>= 1;
> #endif
> #if ENABLE_FEATURE_SWAPON_PRI
> 	if (ret & 1) { // -p
> 		/* ...handle priority option... */
> 	}
> 	ret >>= 1;
> #endif
> #if ENABLE_FEATURE_SWAPON_SUMMARY
> 	if (ret & 1) { // -s
> 		/* ...handle summary option... */
> 	}
> 	ret >>= 1;
> #endif
> 	if (ret /* & 1: redundant */) { // -a
> 		/* ...handle all option... */
> 	}
> 
> That's much cleaner and more scalable.
> 

Hi Matt,
if you like it this way it is fine for me, but for example
the (ret &1 ) ... ret >>= 1 is harder to read
than (ret & OPT_a) as at a first glance it looks always
as the same option. For me that way it is easier to read
and for some options depending on their needs 
you could get it done with no ifdefs at all but relaying
only on compiler's dead code elimination for
size optimization (for example when they have only a flag
but no variables).

Ciao and have a nice day,
Tito

int swap_on_off_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
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 (applet_name[5] == 'n') {
		option_str = "a" IF_FEATURE_SWAPON_PRI("p:");
		if (ENABLE_FEATURE_SWAPON_PRI)
			opt_complementary = "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 |
			MIN(prio, SWAP_FLAG_PRIO_MASK >> SWAP_FLAG_PRIO_SHIFT) << SWAP_FLAG_PRIO_SHIFT;
	}
#endif
	if (ret & OPT_a)
		return do_em_all();

	argv += optind;
	if (!*argv)
		bb_show_usage();

	/* ret = 0; redundant */
	do {
		ret += swap_enable_disable(*argv);
	} while (*++argv);

	return ret;
}
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


[prev in list] [next in list] [prev in thread] [next in thread]