[PATCH] add discard option -d to swapon

Tito farmatito at tiscali.it
Sun Mar 23 23:06:22 UTC 2014


On Sunday 23 March 2014 23:09:59 you wrote:
> On Sunday, 23 March 2014, at 8:29 pm, Tito wrote:
> > On Sunday 23 March 2014 00:33:04 Matt Whitlock wrote:
> > > Attached are a series of three patches affecting swaponoff.
> > 
> > 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.
> 
> So are the main improvements here the use of the IF_FEATURE* macros and testing the ENABLE_FEATURE* macros at the C level rather than at the preprocessor level? 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.
> 
> I do agree that I could clean up the construction of the options string using the IF_FEATURE* macros. See attached patch.

Hi,
looks good to me.
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.   ;-)

Ciao,
Tito


More information about the busybox mailing list