crushtool: do not dump core with non-unique bucket IDs

Return -EEXIST on duplicate ID
BUG FIX: crush_add_bucket() mixes error returns and IDs
  Add optional argument to return generated ID

Fixes: #6246

Signed-off-by: David Zafman <david.zafman@inktank.com>
Reviewed-by: Sage Weil <sage@inktank.com>

8 files changed, 49 insertions, 17 deletions

diff --git a/src/crush/CrushCompiler.cc b/src/crush/CrushCompiler.cc
index aee621d8e32..5f92bf7e4ec 100644
--- a/src/crush/CrushCompiler.cc
+++ b/src/crush/CrushCompiler.cc
@@ -527,9 +527,17 @@ int CrushCompiler::parse_bucket(iter_t const& i)
   item_id[name] = id;
   item_weight[id] = bucketweight;
   
-  crush.add_bucket(id, alg, hash, type, size, &items[0], &weights[0]);
-  crush.set_item_name(id, name.c_str());
-  return 0;
+  assert(id != 0);
+  int r = crush.add_bucket(id, alg, hash, type, size, &items[0], &weights[0], NULL);
+  if (r < 0) {
+    if (r == -EEXIST)
+      err << "Duplicate bucket id " << id << std::endl;
+    else
+      err << "add_bucket failed " << strerror(-r) << std::endl;
+    return r;
+  }
+  r = crush.set_item_name(id, name.c_str());
+  return r;
 }
 
 int CrushCompiler::parse_rule(iter_t const& i)
diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc
index e96e6123aab..bab9f9a817e 100644
--- a/src/crush/CrushWrapper.cc
+++ b/src/crush/CrushWrapper.cc
@@ -363,9 +363,15 @@ int CrushWrapper::insert_item(CephContext *cct, int item, float weight, string n
 
     if (!name_exists(q->second)) {
       ldout(cct, 5) << "insert_item creating bucket " << q->second << dendl;
-      int empty = 0;
-      cur = add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, p->first, 1, &cur, &empty);
-      set_item_name(cur, q->second);
+      int empty = 0, newid;
+      int r = add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, p->first, 1, &cur, &empty, &newid);
+      if (r < 0) {
+        char buf[128]; 
+        ldout(cct, 1) << "add_bucket failure error: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+        return r;
+      }
+      set_item_name(newid, q->second);
+      cur = newid;
       continue;
     }
 
diff --git a/src/crush/CrushWrapper.h b/src/crush/CrushWrapper.h
index 3d07a281956..80906e4fe18 100644
--- a/src/crush/CrushWrapper.h
+++ b/src/crush/CrushWrapper.h
@@ -720,10 +720,10 @@ public:
 
   /* modifiers */
   int add_bucket(int bucketno, int alg, int hash, int type, int size,
-		 int *items, int *weights) {
+		 int *items, int *weights, int *idout) {
     crush_bucket *b = crush_make_bucket(alg, hash, type, size, items, weights);
     assert(b);
-    return crush_add_bucket(crush, bucketno, b);
+    return crush_add_bucket(crush, bucketno, b, idout);
   }
   
   void finalize() {
diff --git a/src/crush/builder.c b/src/crush/builder.c
index 2eb6ff5fc1e..9bfde0bd8e2 100644
--- a/src/crush/builder.c
+++ b/src/crush/builder.c
@@ -123,7 +123,8 @@ int crush_get_next_bucket_id(struct crush_map *map)
 
 int crush_add_bucket(struct crush_map *map,
 		     int id,
-		     struct crush_bucket *bucket)
+		     struct crush_bucket *bucket,
+		     int *idout)
 {
 	int pos;
 
@@ -148,13 +149,16 @@ int crush_add_bucket(struct crush_map *map,
 		memset(map->buckets + oldsize, 0, (map->max_buckets-oldsize) * sizeof(map->buckets[0]));
 	}
 
-	assert(map->buckets[pos] == 0);
+	if (map->buckets[pos] != 0) {
+		return -EEXIST;
+	}
 
         /* add it */
 	bucket->id = id;
 	map->buckets[pos] = bucket;
 
-	return id;
+	if (idout) *idout = id;
+	return 0;
 }
 
 int crush_remove_bucket(struct crush_map *map, struct crush_bucket *bucket)
diff --git a/src/crush/builder.h b/src/crush/builder.h
index 7d30c882343..1003c353e60 100644
--- a/src/crush/builder.h
+++ b/src/crush/builder.h
@@ -15,7 +15,7 @@ extern int crush_add_rule(struct crush_map *map, struct crush_rule *rule, int ru
 extern int crush_get_next_bucket_id(struct crush_map *map);
 extern int crush_add_bucket(struct crush_map *map,
 			    int bucketno,
-			    struct crush_bucket *bucket);
+			    struct crush_bucket *bucket, int *idout);
 struct crush_bucket *crush_make_bucket(int alg, int hash, int type, int size, int *items, int *weights);
 extern int crush_bucket_add_item(struct crush_bucket *bucket, int item, int weight);
 extern int crush_bucket_adjust_item_weight(struct crush_bucket *bucket, int item, int weight);
diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc
index 03f70a05a35..5450532e46d 100644
--- a/src/mon/OSDMonitor.cc
+++ b/src/mon/OSDMonitor.cc
@@ -2699,9 +2699,15 @@ bool OSDMonitor::prepare_command(MMonCommand *m)
       err = -EINVAL;
       goto reply;
     }
-    int bucketno = newcrush.add_bucket(0, CRUSH_BUCKET_STRAW,
+    int bucketno;
+    err = newcrush.add_bucket(0, CRUSH_BUCKET_STRAW,
 				       CRUSH_HASH_DEFAULT, type, 0, NULL,
-				       NULL);
+				       NULL, &bucketno);
+    if (err < 0) {
+      char buf[128];
+      ss << "add_bucket error: '" << strerror_r(-err, buf, sizeof(buf)) << "'";
+      goto reply;
+    }
     err = newcrush.set_item_name(bucketno, name);
     if (err < 0) {
       ss << "error setting bucket name to '" << name << "'";
diff --git a/src/osd/OSDMap.cc b/src/osd/OSDMap.cc
index 4b35b0c48ea..8007d613b8c 100644
--- a/src/osd/OSDMap.cc
+++ b/src/osd/OSDMap.cc
@@ -1845,7 +1845,9 @@ void OSDMap::build_simple_crush_map(CephContext *cct, CrushWrapper& crush,
   crush.set_type_name(6, "root");
 
   // root
-  int rootid = crush.add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, 6 /* pool */, 0, NULL, NULL);
+  int rootid;
+  int r = crush.add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, 6 /* pool */, 0, NULL, NULL, &rootid);
+  assert(r == 0);
   crush.set_item_name(rootid, "default");
 
   for (int o=0; o<nosd; o++) {
@@ -1975,7 +1977,9 @@ void OSDMap::build_simple_crush_map_from_conf(CephContext *cct, CrushWrapper& cr
   set<string> hosts, racks;
 
   // root
-  int rootid = crush.add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, 6 /* pool */, 0, NULL, NULL);
+  int rootid;
+  int r = crush.add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_DEFAULT, 6 /* pool */, 0, NULL, NULL, &rootid);
+  assert(r == 0);
   crush.set_item_name(rootid, "default");
 
   // add osds
diff --git a/src/tools/crushtool.cc b/src/tools/crushtool.cc
index 75c26c098b6..03c83f24156 100644
--- a/src/tools/crushtool.cc
+++ b/src/tools/crushtool.cc
@@ -569,7 +569,11 @@ int main(int argc, const char **argv)
 
 	crush_bucket *b = crush_make_bucket(buckettype, CRUSH_HASH_DEFAULT, type, j, items, weights);
 	assert(b);
-	int id = crush_add_bucket(crush.crush, 0, b);
+	int id;
+	int r = crush_add_bucket(crush.crush, 0, b, &id);
+	if (r < 0) {
+		dout(0) << "Couldn't add root bucket: " << strerror(-r) << dendl;
+	}
 	rootid = id;
 
 	char format[20];