Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

librados: rectify the pool name caching in IoCtxImpl. #3507

Closed
wants to merge 1 commit into from

Conversation

liewegas
Copy link
Member

Fixes: #10458 Signed-off-by: Radoslaw Zarzynski rzarzynski@mirantis.com

Fixes: #10458
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
@@ -1019,8 +1019,6 @@ namespace librados
void set_assert_version(uint64_t ver);
void set_assert_src_version(const std::string& o, uint64_t ver);

const std::string& get_pool_name() const;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think changing the signature for this function is problematic if we want to avoid breaking linkage.

We could keep an internal string list of all unique names we've seen for the pool and return const refs to those. pretty lame but ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is annoying... I guess we need to keep the member var after all, just for return values for this version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was afraid about linking problems too, so added the const modifier to std::string get_pool_name() -- please refer line 626. C++ does not respect the returned type during method overloading, I suppose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't add const to the other version either, as that's also an ABI change

@liewegas
Copy link
Member Author

@jdurgin ?

@loic-bot
Copy link

SUCCESS: the output of run-make-check.sh on centos-centos7 for 1833627 is http://paste2.org/_7cXn0LCG

:octocat: Sent from GH.

ls.push_back(io_ctx_impl->pool_name);
std::string pool_name;

io_ctx_impl->client->pool_get_name(io_ctx_impl->get_id(), &pool_name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should check return value of this - the pool could have been deleted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants