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

ceph-kvstore-tool: allow 'bluestore-kv' as kvdb type; add escaping, compaction #14718

Merged
merged 8 commits into from Apr 25, 2017

Conversation

Projects
None yet
3 participants
@liewegas
Member

liewegas commented Apr 21, 2017

Also some clenaup to wip-bluestore-tool in here.

This is enough to let you manipulate a rocksdb instance embedded in bluestore (and bluefs)
for debugging/repair purposes.

It leaks memory. I think this is fine.

Show outdated Hide outdated src/common/url_escape.cc
case '%':
{
char v;
if (*c >= '0' && *c <= '9') {

This comment has been minimized.

@ifed01

ifed01 Apr 24, 2017

Contributor

*c == '%' at this point, need increment

@ifed01

ifed01 Apr 24, 2017

Contributor

*c == '%' at this point, need increment

Show outdated Hide outdated src/common/url_escape.cc
{
std::string out;
for (auto c : s) {
if (c < 0x20) {

This comment has been minimized.

@ifed01

ifed01 Apr 24, 2017

Contributor

wondering if we need to escape space and percent sign itself?

@ifed01

ifed01 Apr 24, 2017

Contributor

wondering if we need to escape space and percent sign itself?

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

for percent encoding, instead of isprint() (which is basically c < 0x20 does), we need to have something like

if (std::isalnum(c) || c == '-' || c == '.' || c == '_' || c == '~' || c == '/') {
  out.append(ritoa<unsigned char, 16>(c, t));
}
@tchaikov

tchaikov Apr 24, 2017

Contributor

for percent encoding, instead of isprint() (which is basically c < 0x20 does), we need to have something like

if (std::isalnum(c) || c == '-' || c == '.' || c == '_' || c == '~' || c == '/') {
  out.append(ritoa<unsigned char, 16>(c, t));
}
Show outdated Hide outdated src/common/url_escape.cc
}
v <<= 4;
++c;
if (*c >= '0' && *c <= '9') {

This comment has been minimized.

@ifed01

ifed01 Apr 24, 2017

Contributor

replace with a loop?
for(i=0; i<2; ++i)

@ifed01

ifed01 Apr 24, 2017

Contributor

replace with a loop?
for(i=0; i<2; ++i)

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

i'd recommend wrap it in a helper function.

@tchaikov

tchaikov Apr 24, 2017

Contributor

i'd recommend wrap it in a helper function.

@tchaikov

i was just reading the percent encoding/decoding part.

Show outdated Hide outdated src/common/url_escape.cc
{
std::string out;
for (auto c : s) {
if (c < 0x20) {

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

for percent encoding, instead of isprint() (which is basically c < 0x20 does), we need to have something like

if (std::isalnum(c) || c == '-' || c == '.' || c == '_' || c == '~' || c == '/') {
  out.append(ritoa<unsigned char, 16>(c, t));
}
@tchaikov

tchaikov Apr 24, 2017

Contributor

for percent encoding, instead of isprint() (which is basically c < 0x20 does), we need to have something like

if (std::isalnum(c) || c == '-' || c == '.' || c == '_' || c == '~' || c == '/') {
  out.append(ritoa<unsigned char, 16>(c, t));
}
Show outdated Hide outdated src/common/url_escape.cc
std::string url_unescape(const std::string& s)
{
std::string out;
for (const char *c = s.c_str(); *c; ++c) {

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

might need to check if c is out of range instead of comparing it with \0.

@tchaikov

tchaikov Apr 24, 2017

Contributor

might need to check if c is out of range instead of comparing it with \0.

Show outdated Hide outdated src/common/url_escape.cc
throw std::runtime_error(ss.str());
}
v <<= 4;
++c;

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

might need to raise std::out_of_range() if c is out of bound.

@tchaikov

tchaikov Apr 24, 2017

Contributor

might need to raise std::out_of_range() if c is out of bound.

liewegas added some commits Mar 31, 2017

os/bluestore: BLUEFS_START -> SUPER_RESERVED
We reserve the first 8k for bluestore and bluefs superblocks even if
bluefs isn't enabled at mkfs time.

Signed-off-by: Sage Weil <sage@redhat.com>
ceph-bluestore-tool: add error handling to bluefs-export
Signed-off-by: Sage Weil <sage@redhat.com>
ceph-kvstore-tool: nicer failure message on open failure
I have been hating this assert for a long time.

Signed-off-by: Sage Weil <sage@redhat.com>
ceph-kvstore-tool: tabs instead of : as delimiter
Signed-off-by: Sage Weil <sage@redhat.com>
ceph-kvstore-tool: 'bluestore-kv' to open bluestore's embedded kv
This is usually rocksdb on bluefs, but it should work with whatever kv
backend bluestore is configured to use.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 24, 2017

Member

fixed escaping code and added unit tests

Member

liewegas commented Apr 24, 2017

fixed escaping code and added unit tests

@@ -139,6 +139,13 @@ add_executable(unittest_safe_io
add_ceph_unittest(unittest_safe_io ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/unittest_safe_io)
target_link_libraries(unittest_safe_io global)
# unittest_url_escape
add_executable(unittest_url_escape
test_url_escape.cc

This comment has been minimized.

@tchaikov

tchaikov Apr 24, 2017

Contributor

i suppose test_url_escape.cc is included in this commit?

@tchaikov

tchaikov Apr 24, 2017

Contributor

i suppose test_url_escape.cc is included in this commit?

Show outdated Hide outdated src/common/url_escape.cc
}
break;
default:
if (*c < 0x20) {

This comment has been minimized.

@ifed01

ifed01 Apr 24, 2017

Contributor

the same check as in url_escape?

@ifed01

ifed01 Apr 24, 2017

Contributor

the same check as in url_escape?

liewegas added some commits Apr 21, 2017

common/url_escape: add simple url_[un]escape() methods
Signed-off-by: Sage Weil <sage@redhat.com>
ceph-kvstore-tool: (url) escape prefixes and keys
Escape output; unescape input.

Signed-off-by: Sage Weil <sage@redhat.com>
ceph-kvstore-tool: compact{,-prefix,-range}
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Apr 24, 2017

Member
Member

liewegas commented Apr 24, 2017

@ifed01

This comment has been minimized.

Show comment
Hide comment
@ifed01

ifed01 Apr 24, 2017

Contributor

LGTM

Contributor

ifed01 commented Apr 24, 2017

LGTM

@tchaikov

the url escape methods look good to me

@ifed01

ifed01 approved these changes Apr 25, 2017

@liewegas liewegas merged commit 0d31587 into ceph:master Apr 25, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@liewegas liewegas deleted the liewegas:wip-bluestore-tool branch Apr 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment