diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc index 6ac90ccf657..a250d47fb64 100644 --- a/src/crush/CrushWrapper.cc +++ b/src/crush/CrushWrapper.cc @@ -336,16 +336,8 @@ int CrushWrapper::insert_item(CephContext *cct, int item, float weight, string n if (!is_valid_crush_name(name)) return -EINVAL; - - for (map::const_iterator l = loc.begin(); l != loc.end(); l++) { - if (!is_valid_crush_name(l->second)) { - ldout(cct, 1) << "insert_item with loc[" - << l->first << "] = '" - << l->second << "' is not a valid crush name ([A-Za-z0-9_-.]+)" - << dendl; - return -ENFILE; - } - } + if (!is_valid_crush_loc(cct, loc)) + return -EINVAL; if (name_exists(name)) { if (get_item_id(name) != item) { @@ -517,6 +509,9 @@ int CrushWrapper::update_item(CephContext *cct, int item, float weight, string n if (!is_valid_crush_name(name)) return -EINVAL; + if (!is_valid_crush_loc(cct, loc)) + return -EINVAL; + // compare quantized (fixed-point integer) weights! int iweight = (int)(weight * (float)0x10000); int old_iweight; @@ -1145,3 +1140,19 @@ bool CrushWrapper::is_valid_crush_name(const string& s) } return true; } + +bool CrushWrapper::is_valid_crush_loc(CephContext *cct, + const map loc) +{ + for (map::const_iterator l = loc.begin(); l != loc.end(); l++) { + if (!is_valid_crush_name(l->first) || + !is_valid_crush_name(l->second)) { + ldout(cct, 1) << "loc[" + << l->first << "] = '" + << l->second << "' not a valid crush name ([A-Za-z0-9_-.]+)" + << dendl; + return false; + } + } + return true; +} diff --git a/src/crush/CrushWrapper.h b/src/crush/CrushWrapper.h index d665496b88f..9f5cbdfd3a6 100644 --- a/src/crush/CrushWrapper.h +++ b/src/crush/CrushWrapper.h @@ -797,6 +797,8 @@ public: static bool is_valid_crush_name(const string& s); + static bool is_valid_crush_loc(CephContext *cct, + const map loc); }; WRITE_CLASS_ENCODER(CrushWrapper) diff --git a/src/test/crush/TestCrushWrapper.cc b/src/test/crush/TestCrushWrapper.cc index 255ed648e98..1c6670eb3c9 100644 --- a/src/test/crush/TestCrushWrapper.cc +++ b/src/test/crush/TestCrushWrapper.cc @@ -29,6 +29,35 @@ #include "crush/CrushWrapper.h" +TEST(CrushWrapper, update_item) { + CrushWrapper *c = new CrushWrapper; + c->create(); + +#define ROOT_TYPE 3 + c->set_type_name(ROOT_TYPE, "root"); +#define RACK_TYPE 2 + c->set_type_name(RACK_TYPE, "rack"); +#define HOST_TYPE 1 + c->set_type_name(HOST_TYPE, "host"); +#define OSD_TYPE 0 + c->set_type_name(OSD_TYPE, "osd"); + + int rootno; + c->add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_RJENKINS1, + ROOT_TYPE, 0, NULL, NULL, &rootno); + c->set_item_name(rootno, "default"); + + int item = 0; + + // invalid names anywhere in loc trigger an error + { + map loc; + loc["rack"] = "\001"; + EXPECT_EQ(-EINVAL, c->update_item(g_ceph_context, item, 1.0, + "osd." + stringify(item), loc)); + } +} + TEST(CrushWrapper, insert_item) { CrushWrapper *c = new CrushWrapper; c->create(); @@ -48,6 +77,15 @@ TEST(CrushWrapper, insert_item) { c->set_item_name(rootno, "default"); int item = 0; + + // invalid names anywhere in loc trigger an error + { + map loc; + loc["rack"] = "\001"; + EXPECT_EQ(-EINVAL, c->insert_item(g_ceph_context, item, 1.0, + "osd." + stringify(item), loc)); + } + // insert an item in an existing bucket { map loc; @@ -71,16 +109,6 @@ TEST(CrushWrapper, insert_item) { EXPECT_EQ(0, c->insert_item(g_ceph_context, item, 1.0, "osd." + stringify(item), loc)); } - // implicit creation of a bucket with an invalid name fails - { - map loc; - loc["root"] = "default"; - loc["rack"] = "\001"; - - item++; - EXPECT_EQ(-ENFILE, c->insert_item(g_ceph_context, item, 1.0, - "osd." + stringify(item), loc)); - } // adding to an existing item name that is not associated with a bucket { std::string name = "ITEM_WITHOUT_BUCKET"; @@ -200,6 +228,23 @@ TEST(CrushWrapper, is_valid_crush_name) { EXPECT_FALSE(CrushWrapper::is_valid_crush_name("\001")); } +TEST(CrushWrapper, is_valid_crush_loc) { + map loc; + EXPECT_TRUE(CrushWrapper::is_valid_crush_loc(g_ceph_context, loc)); + loc["good"] = "better"; + EXPECT_TRUE(CrushWrapper::is_valid_crush_loc(g_ceph_context, loc)); + { + map loc; + loc["\005"] = "default"; + EXPECT_FALSE(CrushWrapper::is_valid_crush_loc(g_ceph_context, loc)); + } + { + map loc; + loc["rack"] = "\003"; + EXPECT_FALSE(CrushWrapper::is_valid_crush_loc(g_ceph_context, loc)); + } +} + int main(int argc, char **argv) { vector args; argv_to_vec(argc, (const char **)argv, args);