From: Ilya Dryomov Date: Thu, 7 Apr 2022 14:02:46 +0000 (+0200) Subject: librbd/cache/pwl: handle invalid ImageCacheState json X-Git-Tag: v18.0.0~1081^2~4 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=7678ee2490965a8a73c02a47283adaa5036dbcab;p=ceph.git librbd/cache/pwl: handle invalid ImageCacheState json get_json_format() and create_image_cache_state() attempt to get particular keys which could result in an unhandled std::runtime_error exception. Conversely, ImageCacheState constructor just swallows that exception which could leave the newly constructed object incorrectly initialized. Avoid doing parsing in the constructor and introduce init_from_config() and init_from_metadata() methods instead. While at it, move everything out from under "persistent_cache" key. Also fix init_state_json_write test case which stopped working now that types are enforced by json_spirit. Signed-off-by: Ilya Dryomov --- diff --git a/src/librbd/cache/pwl/ImageCacheState.cc b/src/librbd/cache/pwl/ImageCacheState.cc index 13fc28f87621d..1dd81e6480c9b 100644 --- a/src/librbd/cache/pwl/ImageCacheState.cc +++ b/src/librbd/cache/pwl/ImageCacheState.cc @@ -23,37 +23,26 @@ namespace pwl { using namespace std; -namespace { -bool get_json_format(const std::string& s, json_spirit::mObject *o) { - json_spirit::mValue json_root; - if (!json_spirit::read(s.c_str(), json_root)) { - return false; - } else { - auto cache_state_root = json_root.get_obj()["persistent_cache"]; - *o = cache_state_root.get_obj(); - } - return true; -} -} // namespace - template -ImageCacheState::ImageCacheState(I *image_ctx, plugin::Api& plugin_api) : - m_image_ctx(image_ctx), m_plugin_api(plugin_api) { - ldout(image_ctx->cct, 20) << "Initialize RWL cache state with config data. " - << dendl; - - ConfigProxy &config = image_ctx->config; +void ImageCacheState::init_from_config() { + ldout(m_image_ctx->cct, 20) << dendl; + + present = false; + empty = true; + clean = true; + host = ""; + path = ""; + ConfigProxy &config = m_image_ctx->config; mode = config.get_val("rbd_persistent_cache_mode"); + size = 0; } template -ImageCacheState::ImageCacheState( - I *image_ctx, json_spirit::mObject &o, plugin::Api& plugin_api) : - m_image_ctx(image_ctx), m_plugin_api(plugin_api) { - ldout(image_ctx->cct, 20) << "Initialize RWL cache state with data from " - << "server side"<< dendl; +bool ImageCacheState::init_from_metadata(json_spirit::mValue& json_root) { + ldout(m_image_ctx->cct, 20) << dendl; try { + auto& o = json_root.get_obj(); present = o["present"].get_bool(); empty = o["empty"].get_bool(); clean = o["clean"].get_bool(); @@ -62,9 +51,12 @@ ImageCacheState::ImageCacheState( mode = o["mode"].get_str(); size = o["size"].get_uint64(); } catch (std::runtime_error& e) { - lderr(image_ctx->cct) << "failed to parse cache state: " << e.what() - << dendl; + lderr(m_image_ctx->cct) << "failed to parse cache state: " << e.what() + << dendl; + return false; } + + return true; } template @@ -89,11 +81,7 @@ void ImageCacheState::write_image_cache_state(Context *on_finish) { o["misses"] = misses; o["hit_bytes"] = hit_bytes; o["miss_bytes"] = miss_bytes; - json_spirit::mObject cache_state_obj; - cache_state_obj["persistent_cache"] = o; - stringstream json_string; - json_spirit::write(cache_state_obj, json_string); - std::string image_state_json = json_string.str(); + std::string image_state_json = json_spirit::write(o); ldout(m_image_ctx->cct, 20) << __func__ << " Store state: " << image_state_json << dendl; @@ -139,22 +127,23 @@ ImageCacheState* ImageCacheState::create_image_cache_state( r = -EINVAL; }else if ((!dirty_cache || cache_state_str.empty()) && cache_desired) { cache_state = new ImageCacheState(image_ctx, plugin_api); + cache_state->init_from_config(); } else { ceph_assert(!cache_state_str.empty()); - json_spirit::mObject o; - bool success = get_json_format(cache_state_str, &o); - if (!success) { - lderr(image_ctx->cct) << "failed to parse cache state: " - << cache_state_str << dendl; + json_spirit::mValue json_root; + if (!json_spirit::read(cache_state_str.c_str(), json_root)) { + lderr(image_ctx->cct) << "failed to parse cache state" << dendl; r = -EINVAL; return nullptr; } - - bool cache_exists = o["present"].get_bool(); - if (!cache_exists) { - cache_state = new ImageCacheState(image_ctx, plugin_api); - } else { - cache_state = new ImageCacheState(image_ctx, o, plugin_api); + cache_state = new ImageCacheState(image_ctx, plugin_api); + if (!cache_state->init_from_metadata(json_root)) { + delete cache_state; + r = -EINVAL; + return nullptr; + } + if (!cache_state->present) { + cache_state->init_from_config(); } } return cache_state; @@ -168,13 +157,13 @@ ImageCacheState* ImageCacheState::get_image_cache_state( cls_client::metadata_get(&image_ctx->md_ctx, image_ctx->header_oid, IMAGE_CACHE_STATE, &cache_state_str); if (!cache_state_str.empty()) { - json_spirit::mObject o; - bool success = get_json_format(cache_state_str, &o); - if (!success) { + // ignore errors, best effort + cache_state = new ImageCacheState(image_ctx, plugin_api); + json_spirit::mValue json_root; + if (!json_spirit::read(cache_state_str.c_str(), json_root)) { lderr(image_ctx->cct) << "failed to parse cache state" << dendl; - cache_state = new ImageCacheState(image_ctx, plugin_api); } else { - cache_state = new ImageCacheState(image_ctx, o, plugin_api); + cache_state->init_from_metadata(json_root); } } return cache_state; diff --git a/src/librbd/cache/pwl/ImageCacheState.h b/src/librbd/cache/pwl/ImageCacheState.h index 512e8dc1b4001..c2fd4b778253f 100644 --- a/src/librbd/cache/pwl/ImageCacheState.h +++ b/src/librbd/cache/pwl/ImageCacheState.h @@ -46,10 +46,8 @@ public: uint64_t hit_bytes = 0; uint64_t miss_bytes = 0; - ImageCacheState(ImageCtxT* image_ctx, plugin::Api& plugin_api); - - ImageCacheState(ImageCtxT* image_ctx, json_spirit::mObject& f, - plugin::Api& plugin_api); + ImageCacheState(ImageCtxT* image_ctx, plugin::Api& plugin_api) + : m_image_ctx(image_ctx), m_plugin_api(plugin_api) {} ~ImageCacheState() {} @@ -62,6 +60,9 @@ public: return IMAGE_CACHE_TYPE_UNKNOWN; } + void init_from_config(); + bool init_from_metadata(json_spirit::mValue& json_root); + void write_image_cache_state(Context *on_finish); void clear_image_cache_state(Context *on_finish); diff --git a/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc b/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc index 180dd094097f4..d4b1079b5c265 100644 --- a/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc +++ b/src/test/librbd/cache/pwl/test_mock_ReplicatedWriteLog.cc @@ -132,34 +132,22 @@ TEST_F(TestMockCacheReplicatedWriteLog, init_state_write) { ASSERT_EQ(0, finish_ctx.wait()); } -static void get_jf(const string& s, json_spirit::mObject *f) -{ - json_spirit::mValue json_root; - bool result = json_spirit::read(s.c_str(), json_root); - if (!result) { - cout << "failed to parse: '" << s << "'" << std::endl; - } else { - auto cache_state_root = json_root.get_obj()["persistent_cache"]; - *f = cache_state_root.get_obj(); - } - ASSERT_EQ(true, result); -} - TEST_F(TestMockCacheReplicatedWriteLog, init_state_json_write) { librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); MockImageCtx mock_image_ctx(*ictx); - - json_spirit::mObject f; - string strf = "{ \"present\": \"1\", \"empty\": \"0\", \"clean\": \"0\", \ - \"pwl_host\": \"testhost\", \ - \"pwl_path\": \"/tmp\", \ - \"pwl_size\": \"1024\" }"; - get_jf(strf, &f); MockApi mock_api; - MockImageCacheStateRWL image_cache_state(&mock_image_ctx, f, mock_api); + MockImageCacheStateRWL image_cache_state(&mock_image_ctx, mock_api); + string strf = "{ \"present\": true, \"empty\": false, \"clean\": false, \ + \"host\": \"testhost\", \ + \"path\": \"/tmp\", \ + \"mode\": \"rwl\", \ + \"size\": 1024 }"; + json_spirit::mValue json_root; + ASSERT_TRUE(json_spirit::read(strf.c_str(), json_root)); + ASSERT_TRUE(image_cache_state.init_from_metadata(json_root)); validate_cache_state(ictx, image_cache_state, true, false, false, "testhost", "/tmp", 1024); diff --git a/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc b/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc index 9150fa3e8346c..81a248b8e8c54 100644 --- a/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc +++ b/src/test/librbd/cache/pwl/test_mock_SSDWriteLog.cc @@ -134,34 +134,22 @@ TEST_F(TestMockCacheSSDWriteLog, init_state_write) { ASSERT_EQ(0, finish_ctx.wait()); } -static void get_jf(const string& s, json_spirit::mObject *f) -{ - json_spirit::mValue json_root; - bool result = json_spirit::read(s.c_str(), json_root); - if (!result) { - cout << "failed to parse: '" << s << "'" << std::endl; - } else { - auto cache_state_root = json_root.get_obj()["persistent_cache"]; - *f = cache_state_root.get_obj(); - } - ASSERT_EQ(true, result); -} - TEST_F(TestMockCacheSSDWriteLog, init_state_json_write) { librbd::ImageCtx *ictx; ASSERT_EQ(0, open_image(m_image_name, &ictx)); MockImageCtx mock_image_ctx(*ictx); - - json_spirit::mObject f; - string strf = "{ \"present\": \"1\", \"empty\": \"0\", \"clean\": \"0\", \ - \"pwl_host\": \"testhost\", \ - \"pwl_path\": \"/tmp\", \ - \"pwl_size\": \"1024\" }"; - get_jf(strf, &f); MockApi mock_api; - MockImageCacheStateSSD image_cache_state(&mock_image_ctx, f, mock_api); + MockImageCacheStateSSD image_cache_state(&mock_image_ctx, mock_api); + string strf = "{ \"present\": true, \"empty\": false, \"clean\": false, \ + \"host\": \"testhost\", \ + \"path\": \"/tmp\", \ + \"mode\": \"ssd\", \ + \"size\": 1024 }"; + json_spirit::mValue json_root; + ASSERT_TRUE(json_spirit::read(strf.c_str(), json_root)); + ASSERT_TRUE(image_cache_state.init_from_metadata(json_root)); validate_cache_state(ictx, image_cache_state, true, false, false, "testhost", "/tmp", 1024);