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>
This commit is contained in:
Ilya Dryomov 2022-04-07 16:02:46 +02:00
parent 769f3a06ec
commit 7678ee2490
4 changed files with 58 additions and 92 deletions

View File

@ -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;
void ImageCacheState<I>::init_from_config() {
ldout(m_image_ctx->cct, 20) << dendl;
ConfigProxy &config = image_ctx->config;
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;

View File

@ -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);

View File

@ -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);

View File

@ -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);