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

rbd: 'image-meta remove' for missing key does not return error #16393

Merged
merged 1 commit into from Aug 7, 2017

Conversation

Projects
None yet
5 participants
@PCzhangPC

PCzhangPC commented Jul 18, 2017

src/tools/rbd/action/ImageMeta.cc:static int do_metadata_set(librbd::Image& image, const char *key,
'rbd image-meta remove' of missing key does not return error
http://tracker.ceph.com/issues/16990
Signed-off-by: PCzhangPC pengcheng.zhang@easystack.cn

std::cerr << "no existing metadata key name " << key << std::endl;
return -ENOENT;
}
r = image.metadata_remove(key);

This comment has been minimized.

@dillaman

dillaman Jul 18, 2017

Contributor

Nit: you just need to look for a -ENOENT error here and output a "rbd: no existing metadata key" error

This comment has been minimized.

@PCzhangPC

PCzhangPC Jul 19, 2017

you mean the cerr "failed to search existing keys" is not necessary here ?

This comment has been minimized.

@dillaman

dillaman Jul 19, 2017

Contributor

I think it would be good to fix the API method to ensure it returns -ENOENT (if it doesn't already) and then provide a nicer "key doesn't exist" message.

This comment has been minimized.

@PCzhangPC

PCzhangPC Jul 19, 2017

thanks, and i made some changes now

@PCzhangPC PCzhangPC closed this Jul 19, 2017

@PCzhangPC PCzhangPC reopened this Jul 19, 2017

@PCzhangPC PCzhangPC closed this Jul 19, 2017

@PCzhangPC PCzhangPC reopened this Jul 19, 2017

@joscollin

Please update the tracker with the PR URL and assign it to yourself. This is to avoid duplicate PRs.

There is a spelling mistake in the commit message too Fixs.

@@ -284,6 +302,15 @@ int execute_remove(const po::variables_map &vm) {
return r;
}
r = do_matadata_keycheck(image, key.c_str());
if(r<0){
if(r==-ENOENT)

This comment has been minimized.

@joscollin

joscollin Jul 19, 2017

Member

Put space between operators.

if(strcmp(it->first.c_str(),key)==0)
break;
}
if(it==pairs.end()){

This comment has been minimized.

@joscollin

joscollin Jul 19, 2017

Member

Put space between operators and after each statement ; in for

This comment has been minimized.

@PCzhangPC

@joscollin joscollin changed the title from rbd:'rbd image-meta remove' of missing key does not return error to rbd: '$rbd image-meta remove' of missing key does not return error Jul 19, 2017

if(r == -ENOENT)
std::cerr << "rbd: no existing metadata key " << cpp_strerror(r) << std::endl;
else
std::cerr << "rbd: checking metadata key failed:" << cpp_strerror(r) << std::endl;

This comment has been minimized.

@Songweibin

Songweibin Jul 19, 2017

Contributor

better add a space after :

This comment has been minimized.

@PCzhangPC

@dillaman dillaman changed the title from rbd: '$rbd image-meta remove' of missing key does not return error to rbd: 'image-meta remove' for missing key does not return error Jul 19, 2017

@@ -94,6 +94,24 @@ static int do_metadata_set(librbd::Image& image, const char *key,
return r;
}
static int do_matadata_keycheck(librbd::Image& image, const char *key){

This comment has been minimized.

@dillaman

dillaman Jul 19, 2017

Contributor

This logic should be moved into librbd so that the API can return -ENOENT. The place to put the similar logic is about here [1].

[1] https://github.com/ceph/ceph/blob/master/src/librbd/Operations.cc#L1430

static int do_matadata_keycheck(librbd::Image& image, const char *key){
std::map<std::string, bufferlist> pairs;
int r;
r = image.metadata_list("", 0, &pairs);

This comment has been minimized.

@dillaman

dillaman Jul 19, 2017

Contributor

Once moved, use the internally available librbd::cls_client::metadata_get function to retrieve just the specific key

This comment has been minimized.

@PCzhangPC

PCzhangPC Jul 20, 2017

ok ,i'll try right now,thanks

This comment has been minimized.

@PCzhangPC

PCzhangPC Jul 20, 2017

now i'm using cls_client::metadata_get() function to test whether the key exists in operation.cc::metadata_remove,is that what you mean?

@@ -98,6 +98,8 @@ static int do_metadata_remove(librbd::Image& image, const char *key)
{
int r = image.metadata_remove(key);
if (r < 0) {
if( r== -ENOENT)

This comment has been minimized.

@dillaman

dillaman Jul 20, 2017

Contributor

Nit: move this outside the nested if so that it doesn't print two error messages

This comment has been minimized.

@PCzhangPC
@joscollin

This comment has been minimized.

Member

joscollin commented Jul 21, 2017

@PCzhangPC @Songweibin @dillaman
I have been testing this PR. I don't find any difference in the output (shown below) after applying this PR. Am I missing something here ?

$ ./rbd image-meta remove
2017-07-21 11:48:27.832985 7f51f69db000 -1 did not load config file, using default settings.
2017-07-21 11:48:27.835526 7f51f69db000 -1 Errors while parsing config file!
2017-07-21 11:48:27.835557 7f51f69db000 -1 parse_file: cannot open /etc/ceph/ceph.conf: (2) No such file or directory
2017-07-21 11:48:27.835565 7f51f69db000 -1 parse_file: cannot open ~/.ceph/ceph.conf: (2) No such file or directory
2017-07-21 11:48:27.835566 7f51f69db000 -1 parse_file: cannot open ceph.conf: (2) No such file or directory
2017-07-21 11:48:27.836441 7f51f69db000 -1 Errors while parsing config file!
2017-07-21 11:48:27.836486 7f51f69db000 -1 parse_file: cannot open /etc/ceph/ceph.conf: (2) No such file or directory
2017-07-21 11:48:27.836491 7f51f69db000 -1 parse_file: cannot open ~/.ceph/ceph.conf: (2) No such file or directory
2017-07-21 11:48:27.836491 7f51f69db000 -1 parse_file: cannot open ceph.conf: (2) No such file or directory
rbd: image name was not specified
@PCzhangPC

This comment has been minimized.

PCzhangPC commented Jul 21, 2017

@joscollin this command is act on an metadata key. you should create a pool and an image, then try to remove a key that do not exist. such as:

  1. ceph osd pool create mypool 24
  2. rbd -p mypool create myimage --size 100
  3. Then try to remove the 'testkey' that we have not set yet
    rbd image-meta remove mypool/myimage testkey

And the result will show " rbd: no existing metadata key testkey of image : (2) No such file or directory "
In the issure16990 ,the problem of operation on an image that does not exist has been fix already. and what this pr does is to indicate the err if the key does not exist.

@Songweibin

This comment has been minimized.

Contributor

Songweibin commented Jul 21, 2017

looks fine to me, my local test:
root@s248:/home/ceph-dev/build# rbd image-meta remove i1 xxx
rbd: no existing metadata key xxx of image : (2) No such file or directory
rbd: removing metadata failed: (2) No such file or directory
root@s248:/home/ceph-dev/build#

<< cpp_strerror(r) << std::endl;
else
std::cerr << "failed to remove metadata " << key << " of image : "
<< cpp_strerror(r) << std::endl;
}

This comment has been minimized.

@Songweibin

Songweibin Jul 21, 2017

Contributor

Nit: move this outside the nested if so that it doesn't print two error messages

Nit: I think @joscollin means that? without nested:
if (r < 0 && r != -ENOENT) {
} else if (r == -ENOENT) { }

This comment has been minimized.

@PCzhangPC

PCzhangPC Jul 21, 2017

thanks, and I've changed it already.

std::cerr << "failed to remove metadata " << key << " of image : "
<< cpp_strerror(r) << std::endl;
}
else if(r == -ENOENT){

This comment has been minimized.

@dillaman

dillaman Jul 21, 2017

Contributor

Nit: move the -ENOENT check to be the first check so you don't need to exclude it from the other if conditional. Also, the braces should be on the same line as the else (i.e. } else if (xyz) {)

This comment has been minimized.

@PCzhangPC

PCzhangPC Jul 23, 2017

Oh,ok,i'll do this tomorrow. Thx

@dillaman

lgtm

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jul 25, 2017

@PCzhangPC Note that the unit test TestLibRBD.MetadataPP fails and needs to be updated

PCzhangPC
rbd:'rbd image-meta remove' of missing key does not return error
Fixes:http://tracker.ceph.com/issues/16990
Signed-off-by: PCzhangPC <pengcheng.zhang@easystack.cn>
@PCzhangPC

This comment has been minimized.

PCzhangPC commented Aug 7, 2017

@dillaman Is this commit ready to merge?

@dillaman dillaman merged commit bde2f9e into ceph:master Aug 7, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment