Getopt improvements

Getopt improvements

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]


	Hey guys -

	Here's a patch against the new reiserfs_getopt stuff.

	There are a few shortcomings with the existing implementation:
	1) There's no easy way to specify the negation of an option. For 
example, in the current implementation, you can do -oremount,attrs - but 
you can't turn them off again. The only way of handling this would have 
been to add another bit.. and then you can't deal with 
-oremount,attrs,noattrs,attrs - which is the correct one in that case? 
Clearly it's the last one, but there's no way to divine that using the 
existing code.

	2) All of the multiple option sets are mutually exclusive. For example, 
-ohash=r5,hash=rupasov would set BOTH FORCE_RUPASOV_HASH and FORCE_R5_HASH.

	The patch addresses both of these problems.
	The first shortcoming is solved by adding a "no_prefix" member to 
opt_desc_t, which allows the option to be negated by prefixing "no" to 
the option text. In addition, I pass in a populated mount_options so 
that bits can be set and cleared on the current running options. The 
result is that as the options are parsed, they are set or cleared in 
order, so -oattrs,noattrs,attrs results in REISERFS_ATTRS being set, but 
-oattrs,noattrs,attrs,noattrs results in it being cleared. This may seem 
silly, but -oremount prepends the mount options from fstab to the mount 
strings, so a filesytem mounted initially with -oattrs, when remounted 
with -oremount,noattrs will actually parse -oremount,attrs,noattrs. I've 
removed the SET_OPT mechanism, since it only sets bits, it doesn't clear 
them. Also, since some options aren't valid during remount (like hashes, 
conversion, etc), I've added a "safe_mask" that only allows the bits 
contained in it to be updated in s_mount_opts.

	The second shortcoming is addressed by clearing all of the bits 
contained in an arg_desc_t array before setting the final value if one 
exists. As with the above solution, the last one on the command line is 
the one that is kept. If, in the future, some multivalue options aren't 
mutually exclusive, it would be trivial to add a "opt_mutex" flag or 
similar to skip this behavior on a per-option basis.

	The end result is this: Except for the addition of "noattrs" as a 
demonstration, the behavior of existing options is unchanged. 
Multi-value options are now handled correctly such that only one of the 
bits can be set at any given time.

	Please apply.

	-Jeff

-- 
jeffm@suse.com
jeffm@csh.rit.edu

[-- Attachment #2: vanilla.getopt.diff --]
[-- Type: text/plain, Size: 5060 bytes --]

Binary files linux-2.4.20.vanilla/fs/reiserfs/.super.c.swp and linux-2.4.20.vanilla.getopt/fs/reiserfs/.super.c.swp differ
diff -ruN linux-2.4.20.vanilla/fs/reiserfs/super.c linux-2.4.20.vanilla.getopt/fs/reiserfs/super.c
--- linux-2.4.20.vanilla/fs/reiserfs/super.c	2002-11-28 18:53:15.000000000 -0500
+++ linux-2.4.20.vanilla.getopt/fs/reiserfs/super.c	2003-02-26 15:03:33.000000000 -0500
@@ -415,6 +415,7 @@
     const arg_desc_t * values; /* list of values accepted by an option */
     int bitmask;  /* bit which is to be set in mount_options bitmask when this
 		     option is selected, 0 is not bits are to be set */
+    int no_prefix : 1; /* "no" can be prefixed to clear this option */
 } opt_desc_t;
 
 
@@ -490,7 +491,13 @@
 	    if (bit_flags && opt->bitmask != -1 )
 		set_bit (opt->bitmask, bit_flags);
 	    break;
-	}
+	} else if (opt->no_prefix && !strncmp (p, "no", 2) &&
+                !strncmp (p + 2, opt->option_name, strlen (opt->option_name))) {
+            if (bit_flags && opt->bitmask != -1)
+                clear_bit (opt->bitmask, bit_flags);
+            p += 2; /* Skip the "no" */
+            break;
+        }
     }
     if (!opt->option_name) {
 	printk ("reiserfs_getopt: unknown option \"%s\"\n", p);
@@ -536,6 +543,14 @@
     /* values possible for this option are listed in opt->values */
     for (arg = opt->values; arg->value; arg ++) {
 	if (!strcmp (p, arg->value)) {
+            const arg_desc_t * arg_clear;
+            /* Clear all the values from this descriptor */
+            for (arg_clear = opt->values; arg_clear->value; arg_clear++) {
+                if (arg_clear->bitmask != -1)
+                    clear_bit (arg_clear->bitmask, bit_flags);
+            }
+
+            /* The the option we found has a bit.. set that */
 	    if (bit_flags && arg->bitmask != -1 )
 		set_bit (arg->bitmask, bit_flags);
 	    return opt->arg_required;
@@ -548,6 +563,7 @@
 
 
 /* returns 0 if something is wrong in option string, 1 - otherwise */
+/* Caller is responsible for clearing mount_options before use if desired */
 static int reiserfs_parse_options (struct super_block * s, char * options, /* string given via mount's -o */
 				   unsigned long * mount_options,
 				   /* after the parsing phase, contains the
@@ -559,18 +575,18 @@
     char * arg = NULL;
     char * pos;
     opt_desc_t opts[] = {
-		{"tails", 't', tails, -1},
-		{"notail", 0, 0, -1}, /* Compatibility stuff, so that -o notail for old setups still work */
-		{"conv", 0, 0, REISERFS_CONVERT}, 
-		{"nolog", 0, 0, -1},
-		{"replayonly", 0, 0, REPLAYONLY},
+		{"tails", 't', tails, -1, 0},
+		{"notail", 0, 0, -1, 0}, /* Compatibility stuff, so that -o notail for old setups still work */
+		{"conv", 0, 0, REISERFS_CONVERT, 0}, 
+		{"nolog", 0, 0, -1, 0},
+		{"replayonly", 0, 0, REPLAYONLY, 0},
 		
-		{"block-allocator", 'a', balloc, -1}, 
-		{"hash", 'h', hash, FORCE_HASH_DETECT},
+		{"block-allocator", 'a', balloc, -1, 0}, 
+		{"hash", 'h', hash, FORCE_HASH_DETECT, 0},
 		
-		{"resize", 'r', 0, -1},
-		{"attrs", 0, 0, REISERFS_ATTRS},
-		{NULL, 0, 0, 0}
+		{"resize", 'r', 0, -1, 0},
+		{"attrs", 0, 0, REISERFS_ATTRS, 1},
+		{NULL, 0, 0, 0, 0}
     };
 	
     *blocks = 0;
@@ -578,9 +594,6 @@
 	/* use default configuration: create tails, journaling on, no
 	   conversion to newest format */
 	return 1;
-    else
-	/* Drop defaults to zeroes */
-	*mount_options = 0;
     
     for (pos = options; pos; ) {
 	c = reiserfs_getopt (s, &pos, opts, &arg, mount_options);
@@ -634,26 +647,26 @@
   struct reiserfs_super_block * rs;
   struct reiserfs_transaction_handle th ;
   unsigned long blocks;
-  unsigned long mount_options = 0;
+  unsigned long mount_options = s->u.reiserfs_sb.s_mount_opt;
+  unsigned long safe_mask = 0;
 
   rs = SB_DISK_SUPER_BLOCK (s);
 
   if (!reiserfs_parse_options(s, data, &mount_options, &blocks))
   	return 0;
 
-#define SET_OPT( opt, bits, super )					\
-    if( ( bits ) & ( 1 << ( opt ) ) )					\
-	    ( super ) -> u.reiserfs_sb.s_mount_opt |= ( 1 << ( opt ) )
-
-  /* set options in the super-block bitmask */
-  SET_OPT( REISERFS_SMALLTAIL, mount_options, s );
-  SET_OPT( REISERFS_LARGETAIL, mount_options, s );
-  SET_OPT( REISERFS_NO_BORDER, mount_options, s );
-  SET_OPT( REISERFS_NO_UNHASHED_RELOCATION, mount_options, s );
-  SET_OPT( REISERFS_HASHED_RELOCATION, mount_options, s );
-  SET_OPT( REISERFS_TEST4, mount_options, s );
-  SET_OPT( REISERFS_ATTRS, mount_options, s );
-#undef SET_OPT
+  /* Add options that are safe here */
+  safe_mask |= 1 << REISERFS_SMALLTAIL;
+  safe_mask |= 1 << REISERFS_LARGETAIL;
+  safe_mask |= 1 << REISERFS_NO_BORDER;
+  safe_mask |= 1 << REISERFS_NO_UNHASHED_RELOCATION;
+  safe_mask |= 1 << REISERFS_HASHED_RELOCATION;
+  safe_mask |= 1 << REISERFS_TEST4;
+  safe_mask |= 1 << REISERFS_ATTRS;
+
+  /* Update the bitmask, taking care to keep
+   * the bits we're not allowed to change here */
+  s->u.reiserfs_sb.s_mount_opt = (s->u.reiserfs_sb.s_mount_opt & ~safe_mask) | (mount_options & safe_mask);
 
   handle_attrs( s );
 

Re: Getopt improvements

[Hans Reiser]

Sounds good to me.  Let's see what the mailing list says, and let us 
have edward test it.

Hans

Jeff Mahoney wrote:

>
>     Hey guys -
>
>     Here's a patch against the new reiserfs_getopt stuff.
>
>     There are a few shortcomings with the existing implementation:
>     1) There's no easy way to specify the negation of an option. For 
> example, in the current implementation, you can do -oremount,attrs - 
> but you can't turn them off again. The only way of handling this would 
> have been to add another bit.. and then you can't deal with 
> -oremount,attrs,noattrs,attrs - which is the correct one in that case? 
> Clearly it's the last one, but there's no way to divine that using the 
> existing code.
>
>     2) All of the multiple option sets are mutually exclusive. For 
> example, -ohash=r5,hash=rupasov would set BOTH FORCE_RUPASOV_HASH and 
> FORCE_R5_HASH.
>
>     The patch addresses both of these problems.
>     The first shortcoming is solved by adding a "no_prefix" member to 
> opt_desc_t, which allows the option to be negated by prefixing "no" to 
> the option text. In addition, I pass in a populated mount_options so 
> that bits can be set and cleared on the current running options. The 
> result is that as the options are parsed, they are set or cleared in 
> order, so -oattrs,noattrs,attrs results in REISERFS_ATTRS being set, 
> but -oattrs,noattrs,attrs,noattrs results in it being cleared. This 
> may seem silly, but -oremount prepends the mount options from fstab to 
> the mount strings, so a filesytem mounted initially with -oattrs, when 
> remounted with -oremount,noattrs will actually parse 
> -oremount,attrs,noattrs. I've removed the SET_OPT mechanism, since it 
> only sets bits, it doesn't clear them. Also, since some options aren't 
> valid during remount (like hashes, conversion, etc), I've added a 
> "safe_mask" that only allows the bits contained in it to be updated in 
> s_mount_opts.
>
>     The second shortcoming is addressed by clearing all of the bits 
> contained in an arg_desc_t array before setting the final value if one 
> exists. As with the above solution, the last one on the command line 
> is the one that is kept. If, in the future, some multivalue options 
> aren't mutually exclusive, it would be trivial to add a "opt_mutex" 
> flag or similar to skip this behavior on a per-option basis.
>
>     The end result is this: Except for the addition of "noattrs" as a 
> demonstration, the behavior of existing options is unchanged. 
> Multi-value options are now handled correctly such that only one of 
> the bits can be set at any given time.
>
>     Please apply.
>
>     -Jeff
>
>------------------------------------------------------------------------
>
>Binary files linux-2.4.20.vanilla/fs/reiserfs/.super.c.swp and linux-2.4.20.vanilla.getopt/fs/reiserfs/.super.c.swp differ
>diff -ruN linux-2.4.20.vanilla/fs/reiserfs/super.c linux-2.4.20.vanilla.getopt/fs/reiserfs/super.c
>--- linux-2.4.20.vanilla/fs/reiserfs/super.c	2002-11-28 18:53:15.000000000 -0500
>+++ linux-2.4.20.vanilla.getopt/fs/reiserfs/super.c	2003-02-26 15:03:33.000000000 -0500
>@@ -415,6 +415,7 @@
>     const arg_desc_t * values; /* list of values accepted by an option */
>     int bitmask;  /* bit which is to be set in mount_options bitmask when this
> 		     option is selected, 0 is not bits are to be set */
>+    int no_prefix : 1; /* "no" can be prefixed to clear this option */
> } opt_desc_t;
> 
> 
>@@ -490,7 +491,13 @@
> 	    if (bit_flags && opt->bitmask != -1 )
> 		set_bit (opt->bitmask, bit_flags);
> 	    break;
>-	}
>+	} else if (opt->no_prefix && !strncmp (p, "no", 2) &&
>+                !strncmp (p + 2, opt->option_name, strlen (opt->option_name))) {
>+            if (bit_flags && opt->bitmask != -1)
>+                clear_bit (opt->bitmask, bit_flags);
>+            p += 2; /* Skip the "no" */
>+            break;
>+        }
>     }
>     if (!opt->option_name) {
> 	printk ("reiserfs_getopt: unknown option \"%s\"\n", p);
>@@ -536,6 +543,14 @@
>     /* values possible for this option are listed in opt->values */
>     for (arg = opt->values; arg->value; arg ++) {
> 	if (!strcmp (p, arg->value)) {
>+            const arg_desc_t * arg_clear;
>+            /* Clear all the values from this descriptor */
>+            for (arg_clear = opt->values; arg_clear->value; arg_clear++) {
>+                if (arg_clear->bitmask != -1)
>+                    clear_bit (arg_clear->bitmask, bit_flags);
>+            }
>+
>+            /* The the option we found has a bit.. set that */
> 	    if (bit_flags && arg->bitmask != -1 )
> 		set_bit (arg->bitmask, bit_flags);
> 	    return opt->arg_required;
>@@ -548,6 +563,7 @@
> 
> 
> /* returns 0 if something is wrong in option string, 1 - otherwise */
>+/* Caller is responsible for clearing mount_options before use if desired */
> static int reiserfs_parse_options (struct super_block * s, char * options, /* string given via mount's -o */
> 				   unsigned long * mount_options,
> 				   /* after the parsing phase, contains the
>@@ -559,18 +575,18 @@
>     char * arg = NULL;
>     char * pos;
>     opt_desc_t opts[] = {
>-		{"tails", 't', tails, -1},
>-		{"notail", 0, 0, -1}, /* Compatibility stuff, so that -o notail for old setups still work */
>-		{"conv", 0, 0, REISERFS_CONVERT}, 
>-		{"nolog", 0, 0, -1},
>-		{"replayonly", 0, 0, REPLAYONLY},
>+		{"tails", 't', tails, -1, 0},
>+		{"notail", 0, 0, -1, 0}, /* Compatibility stuff, so that -o notail for old setups still work */
>+		{"conv", 0, 0, REISERFS_CONVERT, 0}, 
>+		{"nolog", 0, 0, -1, 0},
>+		{"replayonly", 0, 0, REPLAYONLY, 0},
> 		
>-		{"block-allocator", 'a', balloc, -1}, 
>-		{"hash", 'h', hash, FORCE_HASH_DETECT},
>+		{"block-allocator", 'a', balloc, -1, 0}, 
>+		{"hash", 'h', hash, FORCE_HASH_DETECT, 0},
> 		
>-		{"resize", 'r', 0, -1},
>-		{"attrs", 0, 0, REISERFS_ATTRS},
>-		{NULL, 0, 0, 0}
>+		{"resize", 'r', 0, -1, 0},
>+		{"attrs", 0, 0, REISERFS_ATTRS, 1},
>+		{NULL, 0, 0, 0, 0}
>     };
> 	
>     *blocks = 0;
>@@ -578,9 +594,6 @@
> 	/* use default configuration: create tails, journaling on, no
> 	   conversion to newest format */
> 	return 1;
>-    else
>-	/* Drop defaults to zeroes */
>-	*mount_options = 0;
>     
>     for (pos = options; pos; ) {
> 	c = reiserfs_getopt (s, &pos, opts, &arg, mount_options);
>@@ -634,26 +647,26 @@
>   struct reiserfs_super_block * rs;
>   struct reiserfs_transaction_handle th ;
>   unsigned long blocks;
>-  unsigned long mount_options = 0;
>+  unsigned long mount_options = s->u.reiserfs_sb.s_mount_opt;
>+  unsigned long safe_mask = 0;
> 
>   rs = SB_DISK_SUPER_BLOCK (s);
> 
>   if (!reiserfs_parse_options(s, data, &mount_options, &blocks))
>   	return 0;
> 
>-#define SET_OPT( opt, bits, super )					\
>-    if( ( bits ) & ( 1 << ( opt ) ) )					\
>-	    ( super ) -> u.reiserfs_sb.s_mount_opt |= ( 1 << ( opt ) )
>-
>-  /* set options in the super-block bitmask */
>-  SET_OPT( REISERFS_SMALLTAIL, mount_options, s );
>-  SET_OPT( REISERFS_LARGETAIL, mount_options, s );
>-  SET_OPT( REISERFS_NO_BORDER, mount_options, s );
>-  SET_OPT( REISERFS_NO_UNHASHED_RELOCATION, mount_options, s );
>-  SET_OPT( REISERFS_HASHED_RELOCATION, mount_options, s );
>-  SET_OPT( REISERFS_TEST4, mount_options, s );
>-  SET_OPT( REISERFS_ATTRS, mount_options, s );
>-#undef SET_OPT
>+  /* Add options that are safe here */
>+  safe_mask |= 1 << REISERFS_SMALLTAIL;
>+  safe_mask |= 1 << REISERFS_LARGETAIL;
>+  safe_mask |= 1 << REISERFS_NO_BORDER;
>+  safe_mask |= 1 << REISERFS_NO_UNHASHED_RELOCATION;
>+  safe_mask |= 1 << REISERFS_HASHED_RELOCATION;
>+  safe_mask |= 1 << REISERFS_TEST4;
>+  safe_mask |= 1 << REISERFS_ATTRS;
>+
>+  /* Update the bitmask, taking care to keep
>+   * the bits we're not allowed to change here */
>+  s->u.reiserfs_sb.s_mount_opt = (s->u.reiserfs_sb.s_mount_opt & ~safe_mask) | (mount_options & safe_mask);
> 
>   handle_attrs( s );
> 
>  
>


-- 
Hans

Re: Getopt improvements

[Oleg Drokin]

Hello!

On Wed, Feb 26, 2003 at 03:42:10PM -0500, Jeff Mahoney wrote:

> 	Here's a patch against the new reiserfs_getopt stuff.
> 	There are a few shortcomings with the existing implementation:
> 	1) There's no easy way to specify the negation of an option. For 
> example, in the current implementation, you can do -oremount,attrs - but 
> you can't turn them off again. The only way of handling this would have 
> been to add another bit.. and then you can't deal with 
> -oremount,attrs,noattrs,attrs - which is the correct one in that case? 
> Clearly it's the last one, but there's no way to divine that using the 
> existing code.

All this is highly true. And I got highly annoyed by this myself
That's why I developed similar stuff (my patch is against 2.5, see below).
Should work with remounting (but necessary two lines are missed in this patch).
Also I was in need to set or clear two options, so my approach is a bit different.
(Though initially I was going pretty similar stuff to yours).

> 	2) All of the multiple option sets are mutually exclusive. For example, 
> -ohash=r5,hash=rupasov would set BOTH FORCE_RUPASOV_HASH and FORCE_R5_HASH.

My version makes it so that only last one takes effect. In fact I just made it possible
to specify for each option, what other bits should be cleared when this one is in effect.

> 	The patch addresses both of these problems.
> 	The first shortcoming is solved by adding a "no_prefix" member to 
> opt_desc_t, which allows the option to be negated by prefixing "no" to 

Well, I just decided I won't die if I add negated value to description table ;)

Bye,
    Oleg
===== fs/reiserfs/super.c 1.58 vs edited =====
--- 1.58/fs/reiserfs/super.c	Tue Jan 14 04:12:01 2003
+++ edited/fs/reiserfs/super.c	Thu Feb 27 09:22:34 2003
@@ -500,8 +500,11 @@
    mount options that have values rather than being toggles. */
 typedef struct {
     char * value;
-    int bitmask; /* bit which is to be set in mount_options bitmask when this
-                    value is found, 0 is no bits are to be set */
+    int setmask; /* bitmask which is to set on mount_options bitmask when this
+                    value is found, 0 is no bits are to be changed. */
+    int clrmask; /* bitmask which is to clear on mount_options bitmask when this
+		    value is found, 0 is no bits are to be changed. This is
+		    applied BEFORE setmask */
 } arg_desc_t;
 
 
@@ -511,25 +514,30 @@
     char * option_name;
     int arg_required; /* 0 if argument is not required, not 0 otherwise */
     const arg_desc_t * values; /* list of values accepted by an option */
-    int bitmask;  /* bit which is to be set in mount_options bitmask when this
-		     option is selected, 0 is not bits are to be set */
+    int setmask; /* bitmask which is to set on mount_options bitmask when this
+                    value is found, 0 is no bits are to be changed. */
+    int clrmask; /* bitmask which is to clear on mount_options bitmask when this
+		    value is found, 0 is no bits are to be changed. This is
+		    applied BEFORE setmask */
 } opt_desc_t;
 
 /* possible values for "-o block-allocator=" and bits which are to be set in
    s_mount_opt of reiserfs specific part of in-core super block */
 static const arg_desc_t balloc[] = {
-    {"noborder", REISERFS_NO_BORDER},
-    {"no_unhashed_relocation", REISERFS_NO_UNHASHED_RELOCATION},
-    {"hashed_relocation", REISERFS_HASHED_RELOCATION},
-    {"test4", REISERFS_TEST4},
-    {NULL, -1}
+    {"noborder", 1<<REISERFS_NO_BORDER, 0},
+    {"border", 0, 1<<REISERFS_NO_BORDER},
+    {"no_unhashed_relocation", 1<<REISERFS_NO_UNHASHED_RELOCATION, 0},
+    {"hashed_relocation", 1<<REISERFS_HASHED_RELOCATION, 0},
+    {"test4", 1<<REISERFS_TEST4, 0},
+    {"notest4", 0, 1<<REISERFS_TEST4},
+    {NULL, 0, 0}
 };
 
 static const arg_desc_t tails[] = {
-    {"on", REISERFS_LARGETAIL},
-    {"off", -1},
-    {"small", REISERFS_SMALLTAIL},
-    {NULL, 0}
+    {"on", 1<<REISERFS_LARGETAIL, 1<<REISERFS_SMALLTAIL},
+    {"off", 0, (1<<REISERFS_LARGETAIL)|(1<<REISERFS_SMALLTAIL)},
+    {"small", 1<<REISERFS_SMALLTAIL, 1<<REISERFS_LARGETAIL},
+    {NULL, 0, 0}
 };
 
 
@@ -574,8 +582,10 @@
     /* for every option in the list */
     for (opt = opts; opt->option_name; opt ++) {
 	if (!strncmp (p, opt->option_name, strlen (opt->option_name))) {
-	    if (bit_flags && opt->bitmask != -1)
-		set_bit (opt->bitmask, bit_flags);
+	    if (bit_flags) {
+		*bit_flags &= ~opt->clrmask;
+		*bit_flags |= opt->setmask;
+	    }
 	    break;
 	}
     }
@@ -623,8 +633,10 @@
     /* values possible for this option are listed in opt->values */
     for (arg = opt->values; arg->value; arg ++) {
 	if (!strcmp (p, arg->value)) {
-	    if (bit_flags && arg->bitmask != -1 )
-		set_bit (arg->bitmask, bit_flags);
+	    if (bit_flags) {
+		*bit_flags &= ~arg->clrmask;
+		*bit_flags |= arg->setmask;
+	    }
 	    return opt->arg_required;
 	}
     }
@@ -633,7 +645,6 @@
     return -1;
 }
 
-
 /* returns 0 if something is wrong in option string, 1 - otherwise */
 static int reiserfs_parse_options (struct super_block * s, char * options, /* string given via mount's -o */
 				   unsigned long * mount_options,
@@ -647,17 +658,19 @@
     char * arg = NULL;
     char * pos;
     opt_desc_t opts[] = {
-		{"tails", 't', tails, -1},
-		{"notail", 0, 0, -1}, /* Compatibility stuff, so that -o notail
-for old setups still work */
-		{"conv", 0, 0, REISERFS_CONVERT}, 
-		{"attrs", 0, 0, REISERFS_ATTRS}, 
-		{"nolog", 0, 0, -1},
-		{"replayonly", 0, 0, REPLAYONLY},
-		{"block-allocator", 'a', balloc, -1}, 
-		{"resize", 'r', 0, -1},
-		{"jdev", 'j', 0, -1},
-		{NULL, 0, 0, -1}
+		{"tails", 't', tails, 0, 0},
+		/* Compatibility stuff, so that -o notail
+		   for old setups still work */
+		{"notail", 0, 0, 0, (1<<REISERFS_LARGETAIL)|(1<<REISERFS_SMALLTAIL)},
+		{"conv", 0, 0, 1<<REISERFS_CONVERT, 0}, 
+		{"attrs", 0, 0, 1<<REISERFS_ATTRS, 0}, 
+		{"noattrs", 0, 0, 0, 1<<REISERFS_ATTRS}, 
+		{"nolog", 0, 0, 0, 0}, /* This is unsupported */
+		{"replayonly", 0, 0, 1<<REPLAYONLY, 0},
+		{"block-allocator", 'a', balloc, 0, 0}, 
+		{"resize", 'r', 0, 0, 0},
+		{"jdev", 'j', 0, 0, 0},
+		{NULL, 0, 0, 0, 0}
     };
 	
     *blocks = 0;
@@ -665,9 +678,6 @@
 	/* use default configuration: create tails, journaling on, no
 	   conversion to newest format */
 	return 1;
-    else
-	/* Drop defaults to zeroes */
-	*mount_options = 0;
     
     for (pos = options; pos; ) {
 	c = reiserfs_getopt (s, &pos, opts, &arg, mount_options);

Re: Getopt improvements

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

Oleg Drokin wrote:
> Hello!
> 
> On Wed, Feb 26, 2003 at 03:42:10PM -0500, Jeff Mahoney wrote:
> 
> 
>>	Here's a patch against the new reiserfs_getopt stuff.
>>	There are a few shortcomings with the existing implementation:
>>	1) There's no easy way to specify the negation of an option. For 
>>example, in the current implementation, you can do -oremount,attrs - but 
>>you can't turn them off again. The only way of handling this would have 
>>been to add another bit.. and then you can't deal with 
>>-oremount,attrs,noattrs,attrs - which is the correct one in that case? 
>>Clearly it's the last one, but there's no way to divine that using the 
>>existing code.
> 
> 
> All this is highly true. And I got highly annoyed by this myself
> That's why I developed similar stuff (my patch is against 2.5, see below).
> Should work with remounting (but necessary two lines are missed in this patch).
> Also I was in need to set or clear two options, so my approach is a bit different.
> (Though initially I was going pretty similar stuff to yours).
> 
> 
>>	2) All of the multiple option sets are mutually exclusive. For example, 
>>-ohash=r5,hash=rupasov would set BOTH FORCE_RUPASOV_HASH and FORCE_R5_HASH.
> 
> 
> My version makes it so that only last one takes effect. In fact I just made it possible
> to specify for each option, what other bits should be cleared when this one is in effect.
> 
> 
>>	The patch addresses both of these problems.
>>	The first shortcoming is solved by adding a "no_prefix" member to 
>>opt_desc_t, which allows the option to be negated by prefixing "no" to 
> 
> 
> Well, I just decided I won't die if I add negated value to description table ;)

	Oleg -
	
	I like the ability to specify the bits that should be cleared. It's 
more flexible than my solution, and doesn't involve looping after each 
option.

	However, I noticed in the stock 2.5.63 code that the bits during 
remount don't get propogated at all to s_mount_opts.
	
	Attached is your patch, updated to set the bits on remount.

	Any chance in getting the getopt update into the next 2.4 release?

	Thanks.

	-Jeff

-- 
jeffm@suse.com
jeffm@csh.rit.edu

[-- Attachment #2: linux-2.5.diff --]
[-- Type: text/plain, Size: 5979 bytes --]

diff -ruN linux-2.5.63/fs/reiserfs/super.c linux-2.5.63.devel/fs/reiserfs/super.c
--- linux-2.5.63/fs/reiserfs/super.c	2003-02-24 14:05:31.000000000 -0500
+++ linux-2.5.63.devel/fs/reiserfs/super.c	2003-02-27 10:27:56.000000000 -0500
@@ -500,8 +500,11 @@
    mount options that have values rather than being toggles. */
 typedef struct {
     char * value;
-    int bitmask; /* bit which is to be set in mount_options bitmask when this
-                    value is found, 0 is no bits are to be set */
+    int setmask; /* bitmask which is to set on mount_options bitmask when this
+                    value is found, 0 is no bits are to be changed. */
+    int clrmask; /* bitmask which is to clear on mount_options bitmask when this
+		    value is found, 0 is no bits are to be changed. This is
+		    applied BEFORE setmask */
 } arg_desc_t;
 
 
@@ -511,25 +514,30 @@
     char * option_name;
     int arg_required; /* 0 if argument is not required, not 0 otherwise */
     const arg_desc_t * values; /* list of values accepted by an option */
-    int bitmask;  /* bit which is to be set in mount_options bitmask when this
-		     option is selected, 0 is not bits are to be set */
+    int setmask; /* bitmask which is to set on mount_options bitmask when this
+                    value is found, 0 is no bits are to be changed. */
+    int clrmask; /* bitmask which is to clear on mount_options bitmask when this
+		    value is found, 0 is no bits are to be changed. This is
+		    applied BEFORE setmask */
 } opt_desc_t;
 
 /* possible values for "-o block-allocator=" and bits which are to be set in
    s_mount_opt of reiserfs specific part of in-core super block */
 static const arg_desc_t balloc[] = {
-    {"noborder", REISERFS_NO_BORDER},
-    {"no_unhashed_relocation", REISERFS_NO_UNHASHED_RELOCATION},
-    {"hashed_relocation", REISERFS_HASHED_RELOCATION},
-    {"test4", REISERFS_TEST4},
-    {NULL, -1}
+    {"noborder", 1<<REISERFS_NO_BORDER, 0},
+    {"border", 0, 1<<REISERFS_NO_BORDER},
+    {"no_unhashed_relocation", 1<<REISERFS_NO_UNHASHED_RELOCATION, 0},
+    {"hashed_relocation", 1<<REISERFS_HASHED_RELOCATION, 0},
+    {"test4", 1<<REISERFS_TEST4, 0},
+    {"notest4", 0, 1<<REISERFS_TEST4},
+    {NULL, 0, 0}
 };
 
 static const arg_desc_t tails[] = {
-    {"on", REISERFS_LARGETAIL},
-    {"off", -1},
-    {"small", REISERFS_SMALLTAIL},
-    {NULL, 0}
+    {"on", 1<<REISERFS_LARGETAIL, 1<<REISERFS_SMALLTAIL},
+    {"off", 0, (1<<REISERFS_LARGETAIL)|(1<<REISERFS_SMALLTAIL)},
+    {"small", 1<<REISERFS_SMALLTAIL, 1<<REISERFS_LARGETAIL},
+    {NULL, 0, 0}
 };
 
 
@@ -574,8 +582,10 @@
     /* for every option in the list */
     for (opt = opts; opt->option_name; opt ++) {
 	if (!strncmp (p, opt->option_name, strlen (opt->option_name))) {
-	    if (bit_flags && opt->bitmask != -1)
-		set_bit (opt->bitmask, bit_flags);
+	    if (bit_flags) {
+		*bit_flags &= ~opt->clrmask;
+		*bit_flags |= opt->setmask;
+	    }
 	    break;
 	}
     }
@@ -623,8 +633,10 @@
     /* values possible for this option are listed in opt->values */
     for (arg = opt->values; arg->value; arg ++) {
 	if (!strcmp (p, arg->value)) {
-	    if (bit_flags && arg->bitmask != -1 )
-		set_bit (arg->bitmask, bit_flags);
+	    if (bit_flags) {
+		*bit_flags &= ~arg->clrmask;
+		*bit_flags |= arg->setmask;
+	    }
 	    return opt->arg_required;
 	}
     }
@@ -633,7 +645,6 @@
     return -1;
 }
 
-
 /* returns 0 if something is wrong in option string, 1 - otherwise */
 static int reiserfs_parse_options (struct super_block * s, char * options, /* string given via mount's -o */
 				   unsigned long * mount_options,
@@ -647,17 +658,19 @@
     char * arg = NULL;
     char * pos;
     opt_desc_t opts[] = {
-		{"tails", 't', tails, -1},
-		{"notail", 0, 0, -1}, /* Compatibility stuff, so that -o notail
-for old setups still work */
-		{"conv", 0, 0, REISERFS_CONVERT}, 
-		{"attrs", 0, 0, REISERFS_ATTRS}, 
-		{"nolog", 0, 0, -1},
-		{"replayonly", 0, 0, REPLAYONLY},
-		{"block-allocator", 'a', balloc, -1}, 
-		{"resize", 'r', 0, -1},
-		{"jdev", 'j', 0, -1},
-		{NULL, 0, 0, -1}
+		{"tails", 't', tails, 0, 0},
+		/* Compatibility stuff, so that -o notail
+		   for old setups still work */
+		{"notail", 0, 0, 0, (1<<REISERFS_LARGETAIL)|(1<<REISERFS_SMALLTAIL)},
+		{"conv", 0, 0, 1<<REISERFS_CONVERT, 0}, 
+		{"attrs", 0, 0, 1<<REISERFS_ATTRS, 0}, 
+		{"noattrs", 0, 0, 0, 1<<REISERFS_ATTRS}, 
+		{"nolog", 0, 0, 0, 0}, /* This is unsupported */
+		{"replayonly", 0, 0, 1<<REPLAYONLY, 0},
+		{"block-allocator", 'a', balloc, 0, 0}, 
+		{"resize", 'r', 0, 0, 0},
+		{"jdev", 'j', 0, 0, 0},
+		{NULL, 0, 0, 0, 0}
     };
 	
     *blocks = 0;
@@ -665,9 +678,6 @@
 	/* use default configuration: create tails, journaling on, no
 	   conversion to newest format */
 	return 1;
-    else
-	/* Drop defaults to zeroes */
-	*mount_options = 0;
     
     for (pos = options; pos; ) {
 	c = reiserfs_getopt (s, &pos, opts, &arg, mount_options);
@@ -703,13 +713,27 @@
   struct reiserfs_super_block * rs;
   struct reiserfs_transaction_handle th ;
   unsigned long blocks;
-  unsigned long mount_options;
+  unsigned long mount_options = s->u.reiserfs_sb.s_mount_opt;
+  unsigned long safe_mask = 0;
 
   rs = SB_DISK_SUPER_BLOCK (s);
 
   if (!reiserfs_parse_options(s, arg, &mount_options, &blocks, NULL))
     return -EINVAL;
-  
+
+  /* Add options that are safe here */
+  safe_mask |= 1 << REISERFS_SMALLTAIL;
+  safe_mask |= 1 << REISERFS_LARGETAIL;
+  safe_mask |= 1 << REISERFS_NO_BORDER;
+  safe_mask |= 1 << REISERFS_NO_UNHASHED_RELOCATION;
+  safe_mask |= 1 << REISERFS_HASHED_RELOCATION;
+  safe_mask |= 1 << REISERFS_TEST4;
+  safe_mask |= 1 << REISERFS_ATTRS;
+
+  /* Update the bitmask, taking care to keep
+   * the bits we're not allowed to change here */
+  s->u.reiserfs_sb.s_mount_opt = (s->u.reiserfs_sb.s_mount_opt & ~safe_mask) | (mount_options & safe_mask);
+
   if(blocks) {
     int rc = reiserfs_resize(s, blocks);
     if (rc != 0)

Re: Getopt improvements

[Oleg Drokin]

Hello!

On Thu, Feb 27, 2003 at 10:39:24AM -0500, Jeff Mahoney wrote:

> 	However, I noticed in the stock 2.5.63 code that the bits during 
> remount don't get propogated at all to s_mount_opts.

Sure.
As I said, it would be trivial to do (and I was doing getopt update with this in mind),
but yet not done.

> 	Attached is your patch, updated to set the bits on remount.

Thank's for doing that.

> 	Any chance in getting the getopt update into the next 2.4 release?

I hope for that too. Current behaviour is pretty annoying, I'd say.
But we have 3 patches already in queue. This would be 4th, hope we will get that in time.
I'll port it to 2.4 shortly.

Thank you.

Bye,
    Oleg

Re: Getopt improvements

[Hans Reiser]

Jeff Mahoney wrote:

>
>     Oleg -
>     
>     I like the ability to specify the bits that should be cleared. 

What do you mean by that, forgive me for not reading closely?

> It's more flexible than my solution, and doesn't involve looping after 
> each option. 

Doesn't yours do a better job of handling "no"?

-- 
Hans

Re: Getopt improvements

[Jeff Mahoney]

Hans Reiser wrote:
> Jeff Mahoney wrote:
> 
>>
>>     Oleg -
>>         I like the ability to specify the bits that should be cleared. 
> 
> 
> What do you mean by that, forgive me for not reading closely
>> It's more flexible than my solution, and doesn't involve looping after 
>> each option. 
> 
> 
> Doesn't yours do a better job of handling "no"?

	For the simple cases (which also happen to be all we have right now), 
yes, I think that my implementation is cleaner. It allows the simple use 
of mutually exclusive options, through the "no" prefix, and clearing of 
the other bits in a multivalue option.  For now, that's all we need - 
and it's a valid argument for using my code. However, what I like about 
Oleg's implementation is that if you have an option that excludes other 
options (even when it's not multivalue), it can clear those bits as 
well. What I don't like is how it makes the descriptor definition for 
each argument pretty long and really ugly.

	-Jeff

-- 
jeffm@suse.com
jeffm@csh.rit.edu

Re: Getopt improvements

[Hans Reiser]

Jeff Mahoney wrote:

> Hans Reiser wrote:
>
>> Jeff Mahoney wrote:
>>
>>>
>>>     Oleg -
>>>         I like the ability to specify the bits that should be cleared. 
>>
>>
>>
>> What do you mean by that, forgive me for not reading closely
>>
>>> It's more flexible than my solution, and doesn't involve looping 
>>> after each option. 
>>
>>
>>
>> Doesn't yours do a better job of handling "no"?
>
>
>     For the simple cases (which also happen to be all we have right 
> now), yes, I think that my implementation is cleaner. It allows the 
> simple use of mutually exclusive options, through the "no" prefix, and 
> clearing of the other bits in a multivalue option.  For now, that's 
> all we need - and it's a valid argument for using my code. However, 
> what I like about Oleg's implementation is that if you have an option 
> that excludes other options (even when it's not multivalue), it can 
> clear those bits as well.

It clears them without failing, yes?  Not sure I like that.

> What I don't like is how it makes the descriptor definition for each 
> argument pretty long and really ugly. 

An example?

>
>
>     -Jeff
>


-- 
Hans

Re: Getopt improvements

[Jeff Mahoney]

Hans Reiser wrote:
> Jeff Mahoney wrote:
> 
>> Hans Reiser wrote:
>>
>>> Jeff Mahoney wrote:
>>>
>>>>
>>>>     Oleg -
>>>>         I like the ability to specify the bits that should be cleared. 
>>>
>>>
>>>
>>>
>>> What do you mean by that, forgive me for not reading closely
>>>
>>>> It's more flexible than my solution, and doesn't involve looping 
>>>> after each option. 
>>>
>>>
>>>
>>>
>>> Doesn't yours do a better job of handling "no"?
>>
>>
>>
>>     For the simple cases (which also happen to be all we have right 
>> now), yes, I think that my implementation is cleaner. It allows the 
>> simple use of mutually exclusive options, through the "no" prefix, and 
>> clearing of the other bits in a multivalue option.  For now, that's 
>> all we need - and it's a valid argument for using my code. However, 
>> what I like about Oleg's implementation is that if you have an option 
>> that excludes other options (even when it's not multivalue), it can 
>> clear those bits as well.
> 
> 
> It clears them without failing, yes?  Not sure I like that.
> 
>> What I don't like is how it makes the descriptor definition for each 
>> argument pretty long and really ugly. 
> 
> 
> An example?

	Well, the more possible values a given multivalue option has, the more 
values that need to be explicitly negated for a mutually exclusive 
option. The descriptor lines get quite long with more than 2 options 
when you need to specify every value you need to clear.

	I also got to thinking that perhaps the idea of being able to specify 
multiple values for multivalue options isn't that great. If more than 
one value is possible, the option should probably be a first level 
option (ie: -ooption) rather than a second level option (ie: 
-oname=value). With that in mind, I'd prefer to see my original 
implementation used.

	-Jeff


-- 
jeffm@suse.com
jeffm@csh.rit.edu

Re: Getopt improvements

[Anders Widman]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: MD5


>> It clears them without failing, yes?  Not sure I like that.
>>
>>> What I don't like is how it makes the descriptor definition for each
>>> argument pretty long and really ugly.
>>
>>
>> An example?

>         Well, the more possible values a given multivalue option has, the more
> values that need to be explicitly negated for a mutually exclusive
> option. The descriptor lines get quite long with more than 2 options
> when you need to specify every value you need to clear.

  How  about  a 'clear' option that clears out previous options passed
  when  the fs was mounted?

  ~Anders

-----BEGIN PGP SIGNATURE-----
Version: 2.6

iQEVAwUAPl6r0X+Fu6F2M6JJAQFShAgAnkmFq++U+RCzDdhPppmoEywBVpdNe1qh
oktxPB1SZENmmWwKKFCzTstrBSGW9Ip68ZkxdBUfpj81m/ZRzzdhFvEmIzb4TEVj
tK55D15uaM+yPhdk7dDfMG3c7aCWTp0I1MtOCt1rNoXesz7GX6oh3q53bZ0FMAPc
YfclwxcTu81lPcM0VqyvjiFfPXe7+L5HDX/ipsIIFcxAOVVOLH1E9IheMCRUPtjB
NwLxvaR0r3qzaOmjVhW/nnNV2pbjSMtdh48YDywUIcjNuojPDF39njrS2oW+fyby
94oFprS+Q6YD9oyTfPQGmK36VOVbzmtEwGblbCLn/kEdH93c7jGnXw==
=+IBT
-----END PGP SIGNATURE-----


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 2228 bytes --]

Re: Getopt improvements

[Oleg Drokin]

Hello!

On Fri, Feb 28, 2003 at 01:35:38AM +0300, Hans Reiser wrote:
> >    For the simple cases (which also happen to be all we have right 
> >now), yes, I think that my implementation is cleaner. It allows the 
> >simple use of mutually exclusive options, through the "no" prefix, and 
> >clearing of the other bits in a multivalue option.  For now, that's 
> >all we need - and it's a valid argument for using my code. However, 
> >what I like about Oleg's implementation is that if you have an option 
> >that excludes other options (even when it's not multivalue), it can 
> >clear those bits as well.
> It clears them without failing, yes?  Not sure I like that.

Hm, why should it fail?

Bye,
    Oleg

Re: Getopt improvements

[Oleg Drokin]

Hello!

On Thu, Feb 27, 2003 at 07:15:54PM -0500, Jeff Mahoney wrote:

> 	Well, the more possible values a given multivalue option has, the more 
> values that need to be explicitly negated for a mutually exclusive 
> option. The descriptor lines get quite long with more than 2 options 
> when you need to specify every value you need to clear.

That's true. Not sure we can do something about that.

> 	I also got to thinking that perhaps the idea of being able to specify 
> multiple values for multivalue options isn't that great. If more than 
> one value is possible, the option should probably be a first level 
> option (ie: -ooption) rather than a second level option (ie: 
> -oname=value). With that in mind, I'd prefer to see my original 
> implementation used.

Well, we have two kinds of tails now, large and small.
So do you propose we should have:
notail
smalltail
largetail options?
please keep in mind that smalltail and largetail are mutually exclusive.

Bye,
    Oleg

Re: Getopt improvements

[Oleg Drokin]

Hello!

On Fri, Feb 28, 2003 at 01:22:37AM +0100, Anders Widman wrote:
> >         Well, the more possible values a given multivalue option has, the more
> > values that need to be explicitly negated for a mutually exclusive
> > option. The descriptor lines get quite long with more than 2 options
> > when you need to specify every value you need to clear.
>   How  about  a 'clear' option that clears out previous options passed
>   when  the fs was mounted?

This is counter intuitive, I'd say.

Bye,
    Oleg

Re: Getopt improvements

[Hans Reiser]

Oleg Drokin wrote:

>Hello!
>
>On Fri, Feb 28, 2003 at 01:35:38AM +0300, Hans Reiser wrote:
>  
>
>>>   For the simple cases (which also happen to be all we have right 
>>>now), yes, I think that my implementation is cleaner. It allows the 
>>>simple use of mutually exclusive options, through the "no" prefix, and 
>>>clearing of the other bits in a multivalue option.  For now, that's 
>>>all we need - and it's a valid argument for using my code. However, 
>>>what I like about Oleg's implementation is that if you have an option 
>>>that excludes other options (even when it's not multivalue), it can 
>>>clear those bits as well.
>>>      
>>>
>>It clears them without failing, yes?  Not sure I like that.
>>    
>>
>
>Hm, why should it fail?
>
>Bye,
>    Oleg
>
>
>  
>
Incompatible options should fail as they represent error.  Feel free to 
argue with that.

-- 
Hans

Re: Getopt improvements

[Oleg Drokin]

Hello!

On Fri, Feb 28, 2003 at 03:04:26PM +0300, Hans Reiser wrote:
> >>>  For the simple cases (which also happen to be all we have right 
> >>>now), yes, I think that my implementation is cleaner. It allows the 
> >>>simple use of mutually exclusive options, through the "no" prefix, and 
> >>>clearing of the other bits in a multivalue option.  For now, that's 
> >>>all we need - and it's a valid argument for using my code. However, 
> >>>what I like about Oleg's implementation is that if you have an option 
> >>>that excludes other options (even when it's not multivalue), it can 
> >>>clear those bits as well.
> >>It clears them without failing, yes?  Not sure I like that.
> >Hm, why should it fail?
> Incompatible options should fail as they represent error.  Feel free to 
> argue with that.

Hm, I am not going to argue. But we never had this kind of logic.
Usually the latest-specified option was taking effect.

Bye,
    Oleg

Re: Getopt improvements

[Hans Reiser]

Oleg Drokin wrote:

>Hello!
>
>On Fri, Feb 28, 2003 at 03:04:26PM +0300, Hans Reiser wrote:
>  
>
>>>>> For the simple cases (which also happen to be all we have right 
>>>>>now), yes, I think that my implementation is cleaner. It allows the 
>>>>>simple use of mutually exclusive options, through the "no" prefix, and 
>>>>>clearing of the other bits in a multivalue option.  For now, that's 
>>>>>all we need - and it's a valid argument for using my code. However, 
>>>>>what I like about Oleg's implementation is that if you have an option 
>>>>>that excludes other options (even when it's not multivalue), it can 
>>>>>clear those bits as well.
>>>>>          
>>>>>
>>>>It clears them without failing, yes?  Not sure I like that.
>>>>        
>>>>
>>>Hm, why should it fail?
>>>      
>>>
>>Incompatible options should fail as they represent error.  Feel free to 
>>argue with that.
>>    
>>
>
>Hm, I am not going to argue. But we never had this kind of logic.
>Usually the latest-specified option was taking effect.
>
>Bye,
>    Oleg
>
>
>
>  
>
Conflicts in the same command line should fail, latest command line 
should override.  Yes?

-- 
Hans

Re: Getopt improvements

[Oleg Drokin]

Hello!

On Fri, Feb 28, 2003 at 03:52:17PM +0300, Hans Reiser wrote:

> >>Incompatible options should fail as they represent error.  Feel free to 
> >>argue with that.
> >Hm, I am not going to argue. But we never had this kind of logic.
> >Usually the latest-specified option was taking effect.
> Conflicts in the same command line should fail, latest command line 
> should override.  Yes?

Ok, I will think on how to implement that.

Bye,
    Oleg

Re: Getopt improvements

[Jeff Mahoney]

Oleg Drokin wrote:
> Hello!
> 
> On Fri, Feb 28, 2003 at 03:52:17PM +0300, Hans Reiser wrote:
> 
> 
>>>>Incompatible options should fail as they represent error.  Feel free to 
>>>>argue with that.
>>>
>>>Hm, I am not going to argue. But we never had this kind of logic.
>>>Usually the latest-specified option was taking effect.
>>
>>Conflicts in the same command line should fail, latest command line 
>>should override.  Yes?
> 
> 
> Ok, I will think on how to implement that.
> 
> Bye,
>     Oleg

No, just the opposite. I'm suggesting that if smalltail and largetail 
were _not_ mutually exclusive they should be first level options. Since 
they _are_ mutually exclusive, then they are well suited for a second 
level option.

	I think that the appearance of assignment involved in specifying a 
second level option makes it intuitive to believe that only the most 
recently used option is the active one.

	-Jeff

-- 
jeffm@suse.com
jeffm@csh.rit.edu

Re: Getopt improvements

[Oleg Drokin]

Hello!

On Fri, Feb 28, 2003 at 08:25:33AM -0500, Jeff Mahoney wrote:

> >>	Well, the more possible values a given multivalue option has, the more 
> >>values that need to be explicitly negated for a mutually exclusive 
> >>option. The descriptor lines get quite long with more than 2 options 
> >>when you need to specify every value you need to clear.
> >That's true. Not sure we can do something about that.
> >>	I also got to thinking that perhaps the idea of being able to specify 
> >>multiple values for multivalue options isn't that great. If more than 
> >>one value is possible, the option should probably be a first level 
> >>option (ie: -ooption) rather than a second level option (ie: 
> >>-oname=value). With that in mind, I'd prefer to see my original 
> >>implementation used.
> >Well, we have two kinds of tails now, large and small.
> >So do you propose we should have:
> >notail
> >smalltail
> >largetail options?
> >please keep in mind that smalltail and largetail are mutually exclusive.
> 	No, just the opposite. I'm suggesting that if smalltail and largetail 
> were _not_ mutually exclusive they should be first level options. Since 
> they _are_ mutually exclusive, then they are well suited for a second 
> level option.

Ok, probably I understand. You are against grouping not exclusive options
under one 1st level one, like we have now with balloc, is that right?

Still I do not see how that invalidates the need for "clear mask" for options
that are mutually exclusive with something.

> 	I think that the appearance of assignment involved in specifying a 
> second level option makes it intuitive to believe that only the most 
> recently used option is the active one.

I think that when you able to specify several options at a time (like
alloc=opt1:opt2:opt3) is still intuitive enough.

Bye,
    Oleg

Re: Getopt improvements

[Jeff Mahoney]

Jeff Mahoney wrote:
> Oleg Drokin wrote:
> 
>> Hello!
>>
>> On Fri, Feb 28, 2003 at 03:52:17PM +0300, Hans Reiser wrote:
>>
>>
>>>>> Incompatible options should fail as they represent error.  Feel 
>>>>> free to argue with that.
>>>>
>>>>
>>>> Hm, I am not going to argue. But we never had this kind of logic.
>>>> Usually the latest-specified option was taking effect.
>>>
>>>
>>> Conflicts in the same command line should fail, latest command line 
>>> should override.  Yes?
>>
>>
>>
>> Ok, I will think on how to implement that.
>>
>> Bye,
>>     Oleg
> 
> 
> No, just the opposite. I'm suggesting that if smalltail and largetail 
> were _not_ mutually exclusive they should be first level options. Since 
> they _are_ mutually exclusive, then they are well suited for a second 
> level option.
> 
>     I think that the appearance of assignment involved in specifying a 
> second level option makes it intuitive to believe that only the most 
> recently used option is the active one.
> 
>     -Jeff
> 

	Sorry guys, that'll teach me to skip breakfast. I accidentally replied 
to Oleg as "Reply", rather than "Reply All", and grabbed the wrong 
message to use for "Reply All" for this list.

	-Jeff


-- 
jeffm@suse.com
jeffm@csh.rit.edu