From: Sage Weil <sage@redhat.com>
Date: Mon, 17 Jul 2017 16:10:45 +0000 (-0400)
Subject: crush: do not allow creating a rule with ruleset != rule id
X-Git-Tag: v12.1.2~216^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=ccca7a54b666fd80a4d115780f21b71d8ea9e23d;p=ceph.git

crush: do not allow creating a rule with ruleset != rule id

Consolidate the add_rule rule id and ruleset arguments to be the same and
fix the callers.

Signed-off-by: Sage Weil <sage@redhat.com>
---

diff --git a/src/crush/CrushCompiler.cc b/src/crush/CrushCompiler.cc
index e01d089acbc..b7c7a389047 100644
--- a/src/crush/CrushCompiler.cc
+++ b/src/crush/CrushCompiler.cc
@@ -770,7 +770,7 @@ int CrushCompiler::parse_rule(iter_t const& i)
     start = 3;
   }
 
-  int ruleset = int_node(i->children[start]);
+  int ruleno = int_node(i->children[start]);
 
   string tname = string_node(i->children[start+2]);
   int type;
@@ -786,8 +786,17 @@ int CrushCompiler::parse_rule(iter_t const& i)
   
   int steps = i->children.size() - start - 8;
   //err << "num steps " << steps << std::endl;
-  
-  int ruleno = crush.add_rule(steps, ruleset, type, minsize, maxsize, -1);
+
+  if (crush.rule_exists(ruleno)) {
+    err << "rule " << ruleno << " already exists" << std::endl;
+    return -1;
+  }
+  int r = crush.add_rule(ruleno, steps, type, minsize, maxsize);
+  if (r != ruleno) {
+    err << "unable to add rule id " << ruleno << " for rule '" << rname
+	<< "'" << std::endl;
+    return -1;
+  }
   if (rname.length()) {
     crush.set_rule_name(ruleno, rname.c_str());
     rule_id[rname] = ruleno;
diff --git a/src/crush/CrushWrapper.h b/src/crush/CrushWrapper.h
index 5fd35a88ad9..5da29e44a53 100644
--- a/src/crush/CrushWrapper.h
+++ b/src/crush/CrushWrapper.h
@@ -1005,9 +1005,10 @@ public:
   int get_rule_weight_osd_map(unsigned ruleno, map<int,float> *pmap);
 
   /* modifiers */
-  int add_rule(int len, int ruleset, int type, int minsize, int maxsize, int ruleno) {
+
+  int add_rule(int ruleno, int len, int type, int minsize, int maxsize) {
     if (!crush) return -ENOENT;
-    crush_rule *n = crush_make_rule(len, ruleset, type, minsize, maxsize);
+    crush_rule *n = crush_make_rule(len, ruleno, type, minsize, maxsize);
     assert(n);
     ruleno = crush_add_rule(crush, n, ruleno);
     return ruleno;
diff --git a/src/erasure-code/lrc/ErasureCodeLrc.cc b/src/erasure-code/lrc/ErasureCodeLrc.cc
index 05cd6ed7580..c81588a52a3 100644
--- a/src/erasure-code/lrc/ErasureCodeLrc.cc
+++ b/src/erasure-code/lrc/ErasureCodeLrc.cc
@@ -71,20 +71,18 @@ int ErasureCodeLrc::create_rule(const string &name,
     root = crush.class_bucket[root][c];
   }
 
-  int rule = 0;
   int rno = 0;
   for (rno = 0; rno < crush.get_max_rules(); rno++) {
     if (!crush.rule_exists(rno) && !crush.ruleset_exists(rno))
        break;
   }
-  rule = rno;
 
   int steps = 4 + rule_steps.size();
   int min_rep = 3;
   int max_rep = get_chunk_count();
   int ret;
-  ret = crush.add_rule(steps, rule, pg_pool_t::TYPE_ERASURE,
-		  min_rep, max_rep, rno);
+  ret = crush.add_rule(rno, steps, pg_pool_t::TYPE_ERASURE,
+		       min_rep, max_rep);
   assert(ret == rno);
   int step = 0;
 
@@ -112,7 +110,7 @@ int ErasureCodeLrc::create_rule(const string &name,
   ret = crush.set_rule_step(rno, step++, CRUSH_RULE_EMIT, 0, 0);
   assert(ret == 0);
   crush.set_rule_name(rno, name);
-  return rule;
+  return rno;
 }
 
 int ErasureCodeLrc::layers_description(const ErasureCodeProfile &profile,
diff --git a/src/test/cli/crushtool/check-overlapped-rules.crushmap b/src/test/cli/crushtool/check-overlapped-rules.crushmap
new file mode 100644
index 00000000000..a3cbfd16ff9
Binary files /dev/null and b/src/test/cli/crushtool/check-overlapped-rules.crushmap differ
diff --git a/src/test/cli/crushtool/check-overlapped-rules.t b/src/test/cli/crushtool/check-overlapped-rules.t
index acad57d14cb..41d9c9de270 100644
--- a/src/test/cli/crushtool/check-overlapped-rules.t
+++ b/src/test/cli/crushtool/check-overlapped-rules.t
@@ -1,6 +1,7 @@
-  $ crushtool -c "$TESTDIR/check-overlapped-rules.crushmap.txt" -o "$TESTDIR/check-overlapped-rules.crushmap"
   $ crushtool -i "$TESTDIR/check-overlapped-rules.crushmap" --check
   overlapped rules in ruleset 0: rule-r0, rule-r1, rule-r2
   overlapped rules in ruleset 0: rule-r0, rule-r2, rule-r3
   overlapped rules in ruleset 0: rule-r0, rule-r3
-  $ rm -f "$TESTDIR/check-overlapped-rules.crushmap"
+  $ crushtool -c "$TESTDIR/check-overlapped-rules.crushmap.txt" -o "$TESTDIR/check-overlapped-rules.crushmap.new"
+  rule 0 already exists
+  [1]
diff --git a/src/test/crush/CrushWrapper.cc b/src/test/crush/CrushWrapper.cc
index e0a327ffff3..f4752f17a83 100644
--- a/src/test/crush/CrushWrapper.cc
+++ b/src/test/crush/CrushWrapper.cc
@@ -1407,7 +1407,7 @@ TEST(CrushWrapper, try_remap_rule) {
   // choose + choose
   {
     cout << "take + choose + choose + choose + emit" << std::endl;
-    int rule = c.add_rule(5, 2, 0, 1, 10, 2);
+    int rule = c.add_rule(2, 5, 0, 1, 10);
     ASSERT_EQ(2, rule);
     c.set_rule_step_take(rule, 0, bno);
     c.set_rule_step_choose_indep(rule, 1, 2, 2);
diff --git a/src/test/crush/crush.cc b/src/test/crush/crush.cc
index 4a5fc63d6a3..dba3bb584dc 100644
--- a/src/test/crush/crush.cc
+++ b/src/test/crush/crush.cc
@@ -52,9 +52,7 @@ std::unique_ptr<CrushWrapper> build_indep_map(CephContext *cct, int num_rack,
   }
   int ret;
   int ruleno = 0;
-  int rule = 0;
-  ruleno = rule;
-  ret = c->add_rule(4, rule, 123, 1, 20, ruleno);
+  ret = c->add_rule(ruleno, 4, 123, 1, 20);
   assert(ret == ruleno);
   ret = c->set_rule_step(ruleno, 0, CRUSH_RULE_SET_CHOOSELEAF_TRIES, 10, 0);
   assert(ret == 0);