[Ocfs2-tools-devel] [PATCH 01/24] libocfs2: Clearing features should clear their dependancies.

[Joel Becker]

The ocfs2_parse_feature() API was not symmetric.  If asked to set a
feature, it would set the feature and its dependencies.  However, when
asked to clear a feature, it would not clear the other features that
depended on it.  Thus, a caller looking at the list of features to clear
would have to know the reverse dependencies as well.

This change modifies ocfs2_parse_feature() to include the reverse
dependencies.  In addition, ocfs2_merge_feature_with_level() takes
advantage of the change to become simpler.

Finally, tunefs.ocfs2 now expects the reverse dependencies.  Instead of
adding them by hand, it makes sure they exist and behaves appropriately.

Signed-off-by: Joel Becker <joel.becker at oracle.com>
Signed-off-by: Tao Ma <tao.ma at oracle.com>
---
 libocfs2/feature_string.c |   50 +++++++++++++++++++++++++++-----------------
 tunefs.ocfs2/features.c   |   15 +++++++++---
 2 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/libocfs2/feature_string.c b/libocfs2/feature_string.c
index 4e44fc1..359e021 100644
--- a/libocfs2/feature_string.c
+++ b/libocfs2/feature_string.c
@@ -151,6 +151,28 @@ static int check_feature_flags(ocfs2_fs_options *fs_flags,
 }
 
 /*
+ * If we are asked to clear a feature, we also need to clear any other
+ * features that depend on it.
+ */
+static void ocfs2_feature_clear_deps(ocfs2_fs_options *reverse_set)
+{
+	int i;
+
+	for(i = 0; ocfs2_supported_features[i].ff_str; i++) {
+		if ((reverse_set->opt_compat &
+			ocfs2_supported_features[i].ff_flags.opt_compat) ||
+		    (reverse_set->opt_incompat &
+			ocfs2_supported_features[i].ff_flags.opt_incompat) ||
+		    (reverse_set->opt_ro_compat &
+			ocfs2_supported_features[i].ff_flags.opt_ro_compat)) {
+			merge_features(reverse_set,
+				       ocfs2_supported_features[i].ff_own_flags);
+		}
+	}
+
+}
+
+/*
  * Check and Merge all the diffent features set by the user.
  *
  * index: the feature level.
@@ -162,7 +184,6 @@ errcode_t ocfs2_merge_feature_flags_with_level(ocfs2_fs_options *dest,
 					       ocfs2_fs_options *feature_set,
 					       ocfs2_fs_options *reverse_set)
 {
-	int i;
 	ocfs2_fs_options level_set = feature_level_defaults[level];
 	/*
 	 * "Check whether the user asked for a flag to be set and cleared,
@@ -177,26 +198,16 @@ errcode_t ocfs2_merge_feature_flags_with_level(ocfs2_fs_options *dest,
 	merge_features(dest, *feature_set);
 
 	/*
-	 * We have to remove all the features in the reverse set
-	 * and other features which depend on them.
+	 * Ensure that all dependancies are correct in the reverse set.
+	 * A reverse set from ocfs2_parse_feature() will be correct, but
+	 * a hand-built one might not be.
 	 */
-	for(i = 0; ocfs2_supported_features[i].ff_str; i++) {
-		if ((reverse_set->opt_compat &
-			ocfs2_supported_features[i].ff_flags.opt_compat) ||
-		    (reverse_set->opt_incompat &
-			ocfs2_supported_features[i].ff_flags.opt_incompat) ||
-		    (reverse_set->opt_ro_compat &
-			ocfs2_supported_features[i].ff_flags.opt_ro_compat)) {
-			dest->opt_compat &=
-			~ocfs2_supported_features[i].ff_own_flags.opt_compat;
+	ocfs2_feature_clear_deps(reverse_set);
 
-			dest->opt_incompat &=
-			~ocfs2_supported_features[i].ff_own_flags.opt_incompat;
-
-			dest->opt_ro_compat &=
-			~ocfs2_supported_features[i].ff_own_flags.opt_ro_compat;
-		}
-	}
+	/* Now clear the reverse set from our destination */
+	dest->opt_compat &= ~reverse_set->opt_compat;
+	dest->opt_ro_compat &= ~reverse_set->opt_ro_compat;
+	dest->opt_incompat &= ~reverse_set->opt_incompat;
 
 	return 0;
 }
@@ -255,6 +266,7 @@ errcode_t ocfs2_parse_feature(const char *opts,
 	}
 
 	free(options);
+	ocfs2_feature_clear_deps(reverse_flags);
 
 	return 0;
 }
diff --git a/tunefs.ocfs2/features.c b/tunefs.ocfs2/features.c
index c754f06..268e5d9 100644
--- a/tunefs.ocfs2/features.c
+++ b/tunefs.ocfs2/features.c
@@ -84,11 +84,18 @@ errcode_t feature_check(ocfs2_filesys *fs)
 
 		/*
 		 * Turning off sparse files means we must also turn
-		 * off unwritten extents.
+		 * off unwritten extents.  Check to make sure the clear
+		 * list includes them.
 		 */
-		if (ocfs2_writes_unwritten_extents(OCFS2_RAW_SB(fs->fs_super)))
-			opts.clear_feature.opt_ro_compat |=
-				OCFS2_FEATURE_RO_COMPAT_UNWRITTEN;
+		if (!(opts.clear_feature.opt_ro_compat &
+		      OCFS2_FEATURE_RO_COMPAT_UNWRITTEN))
+			goto bail;
+
+		/* But if we don't have unwritten extents, we don't need
+		 * to clear them. */
+		if (!ocfs2_writes_unwritten_extents(OCFS2_RAW_SB(fs->fs_super)))
+			opts.clear_feature.opt_ro_compat &=
+				~OCFS2_FEATURE_RO_COMPAT_UNWRITTEN;
 
 		sparse_on = 0;
 		ret = clear_sparse_file_check(fs, opts.progname, 0);
-- 
1.5.6.3