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 <idryomov@gmail.com>

diff --git a/src/librbd/cache/pwl/ImageCacheState.cc b/src/librbd/cache/pwl/ImageCacheState.cc
index 13fc28f87621de03925bc29747f4ebc7ee046dfa..1dd81e6480c9b39f33875fc44ab0b4f538779d9b 100644 (file)
--- 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 <typename I>
-ImageCacheState<I>::ImageCacheState(I *image_ctx, plugin::Api<I>& 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<I>::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<std::string>("rbd_persistent_cache_mode");
+  size = 0;
 }
 
 template <typename I>
-ImageCacheState<I>::ImageCacheState(
-    I *image_ctx, json_spirit::mObject &o, plugin::Api<I>& 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<I>::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<I>::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 <typename I>
@@ -89,11 +81,7 @@ void ImageCacheState<I>::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<I>* ImageCacheState<I>::create_image_cache_state(
     r = -EINVAL;
   }else if ((!dirty_cache || cache_state_str.empty()) && cache_desired) {
     cache_state = new ImageCacheState<I>(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<I>(image_ctx, plugin_api);
-    } else {
-      cache_state = new ImageCacheState<I>(image_ctx, o, plugin_api);
+    cache_state = new ImageCacheState<I>(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<I>* ImageCacheState<I>::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<I>(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<I>(image_ctx, plugin_api);
     } else {
-      cache_state = new ImageCacheState<I>(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 512e8dc1b4001b8995ddea9151a0f83f78a8e6a5..c2fd4b778253f8d89fe0b5430ad2ad1b162a4b78 100644 (file)
--- 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<ImageCtxT>& plugin_api);
-
-  ImageCacheState(ImageCtxT* image_ctx, json_spirit::mObject& f,
-                  plugin::Api<ImageCtxT>& plugin_api);
+  ImageCacheState(ImageCtxT* image_ctx, plugin::Api<ImageCtxT>& 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 180dd094097f450120ec01a620ce0c6886c648ed..d4b1079b5c2655655d37fd22932d1d3ba1c1827a 100644 (file)
--- 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 9150fa3e8346c9aef179dcaf61995651aca7e8bb..81a248b8e8c54366c7008d37bbef531951150a2f 100644 (file)
--- 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);