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-nbd: display pool/image/snap information in list output #15317

Merged
merged 3 commits into from Jun 22, 2017

Conversation

Projects
None yet
4 participants
@liupan1111
Contributor

liupan1111 commented May 26, 2017

a nbd device.

In my local test, The result will be shown like below:
$./rbd-nbd list-mapped
id device pool image snap
0 /dev/nbd0 rbd image
1 /dev/nbd1 rbd image1
2 /dev/nbd2 rbd image
3 /dev/nbd3 rbd image_snap snap1
4 /dev/nbd4 rbd image11 snap1

Signed-off-by: Pan Liu wanjun.lp@alibaba-inc.com

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented May 26, 2017

@dillaman @trociny , as we talked before, I would improve rbd-nbd list-mapped as your suggestion. Please help take a look. Thanks.

@trociny

Can we add --verbose or --long option and output this extended info only when it is set?

for(unsigned int index = 0; index < cmdline.size(); index++) {
if(((unsigned char)cmdline[index]) < ' ') {
cmdline[index] = ' ';

This comment has been minimized.

@trociny

trociny May 26, 2017

Contributor

I would suggest to create vector<const char*> args, and fill it with pointers to command line parameters.
Then you can reuse the code from rbd_nbd() to parse command line arguments. That is you will need extract that code to a separate function.

For parsing image spec you can reuse parse_imgpath() function (after some modification). And actually, for me it would be ok if we leave the image spec unparsed, as it is provided by a user, but I don't have a strong opinion here.

This comment has been minimized.

@liupan1111

liupan1111 May 27, 2017

Contributor

I would suggest to create vector<const char*> args, and fill it with pointers to command line parameters.
Then you can reuse the code from rbd_nbd() to parse command line arguments. That is you will need extract that code to a separate function.

Agree, let me try.

For parsing image spec you can reuse parse_imgpath() function (after some modification). And actually, for me it would be ok if we leave the image spec unparsed, as it is provided by a user, but I don't have a strong opinion here.

Cool, I will reuse parse_imgpath. Actually, "rbd showmapped" parse image spec, I also want to keep.

if(pid > 0) {
std::string pool_name, image_name, snap_name;
get_mapped_info(pid, pool_name, image_name, snap_name);

This comment has been minimized.

@trociny

trociny May 26, 2017

Contributor

Probably a good idea for get_mapped_info to return a error code (whether it succeeded) so we could filter out nbd devices created by other (not rbd-nbd) processes.

This comment has been minimized.

@liupan1111

liupan1111 May 27, 2017

Contributor

yes, I think just a bool returned-value is enough for this case. Do you agree?

This comment has been minimized.

@trociny

trociny May 27, 2017

Contributor

ok

if(pid > 0) {
std::string pool_name, image_name, snap_name;
get_mapped_info(pid, pool_name, image_name, snap_name);
tbl << stringify(id) << "/dev/nbd" + stringify(index) << pool_name << image_name << snap_name << TextTable::endrow;

This comment has been minimized.

@trociny

trociny May 26, 2017

Contributor

I would like to see pid in this output too.
nit: break lines exceeding 80 characters.

This comment has been minimized.

@trociny

trociny May 26, 2017

Contributor

Also, I am not sure how id is useful in the output. So may be just pid instead of id?

This comment has been minimized.

@liupan1111

liupan1111 May 27, 2017

Contributor

Yes, I agree, let me change item id to pid. In rbd showmapped, it displayed id, but it is totally kernel based. For userspace rbd-nbd, pid is better.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented May 27, 2017

Can we add --verbose or --long option and output this extended info only when it is set?

I do not like this option very much. I tried rbd showmapped:

$rbd showmapped
id pool image      snap device
0  rbd  image_test -    /dev/rbd0

No any extra options. I prefer rbd-nbd list-mapped to keep consistent with rbd showmapped.

@trociny

This comment has been minimized.

Contributor

trociny commented May 27, 2017

@liupan1111 I am ok about being consistent with rbd showmapped. I was worrying that user might have already started to use rbd nbd list in their scripts and that change may break them. I am not sure this is a big issue though.

BTW, have you tried qa/workunits/rbd/rbd-nbd.sh? It uses list-mapped and might need update. You could also extend it to test the new list-mapped functionality.

TextTable tbl;
tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);

This comment has been minimized.

@trociny

trociny May 27, 2017

Contributor

If you want to be consistent with rbd showmapped, "device" should be the last column?

This comment has been minimized.

@liupan1111

liupan1111 May 27, 2017

Contributor

OK, thanks.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented May 27, 2017

@liupan1111 I am ok about being consistent with rbd showmapped. I was worrying that user might have already started to use rbd nbd list in their scripts and that change may break them. I am not sure this is a big issue though.

Thanks. I agree their scripts may be changed, but should not a big issue: old version list-mapped, will only show /dev/nbdX, and new version will have more information. If they only need nbd name, just change the script and use awk to print device column. It is hard to avoid any change when upgrade a newer version. :)

BTW, have you tried qa/workunits/rbd/rbd-nbd.sh? It uses list-mapped and might need update. You could also extend it to test the new list-mapped functionality.

I will re golden it, and one question: will rbd-nbd.sh run in teuthology? Indeed, I am not very clear about ceph test cases...

@trociny

This comment has been minimized.

Contributor

trociny commented May 27, 2017

rbd-nbd.sh is run by teuthology. But you can also run it locally, just make sure your built binaries are in $PATH and ceph.conf is accessible (i.e. either in /etc/ceph or in current directory) and valid. So if you are in ceph/build directory, something like below should work:

PATH=$(pwd)/bin:$PATH
../src/vstart -n
../qa/workunits/rbd/rbd-nbd.sh

It expects sudo is installed and configured (or run it from root).

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented May 27, 2017

@trociny very clear! Thanks!

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented May 28, 2017

@trociny updated as your suggestion, thanks.

while (pch != NULL) {
args.push_back(pch);
pch = strtok(NULL, " ");
}

This comment has been minimized.

@trociny

trociny May 28, 2017

Contributor

I don't think this manual parsing is needed. What I meant was to take cmdline, which contains arguments separated by '\0', and split it into args, using something like below (this is just a not tested example, may be you will find a better way):

   for (int i = 0; i < cmdline.size(); i++) {
      args.push_back(&cmdline[i]);
      while(cmdline[i] != '\0') {
        i++;
      }
   }

Then you just pass args to parse_args(). And your parse_args() should do md_config_t().parse_argv(args) at start.

This comment has been minimized.

@liupan1111

liupan1111 May 28, 2017

Contributor

ok, let me try.

return 0;
}
static int parse_args(vector<const char*>& args)

This comment has been minimized.

@trociny

trociny May 28, 2017

Contributor

Add an argument to suppress output error message (or usage) when it is called by list-mapped.

This comment has been minimized.

@liupan1111

liupan1111 May 28, 2017

Contributor

Is it possible any error? in my opinion, if mapped successfully, the cmdline in /proc/pid/cmdline would not have any error. I also added a check for rbd-nbd command name .

This comment has been minimized.

@trociny

trociny May 28, 2017

Contributor

Theoretically an nbd device can be created by some other (not rbd-nbd) process. We should silently filter out this.

This comment has been minimized.

@liupan1111

liupan1111 May 29, 2017

Contributor

yes, so I added the check in my code:

if (strstr(pch, "rbd-nbd") != NULL) { 
   pch = strtok(NULL, " ");
   break;
}

As your suggestion above, I could remove strtok parse here, but I could use find "rbd-nbd" in cmdline instead. If not found, ignore this nbd device. I don't prefer to add an argument to parse_args, which may change original logic for parsing argument.

This comment has been minimized.

@trociny

trociny May 29, 2017

Contributor

Actually, when building args vector from cmdline you just need to check the first arg, that basename(arg) is 'rbd-nbd' and skip pushing it in args.

This comment has been minimized.

@liupan1111

liupan1111 May 29, 2017

Contributor

yes

vector<const char*> args;
argv_to_vec(argc, argv, args);
md_config_t().parse_argv(args);

This comment has been minimized.

@trociny

trociny May 28, 2017

Contributor

Move md_config_t().parse_argv(args) to parse_args(args).

This comment has been minimized.

@liupan1111

liupan1111 May 29, 2017

Contributor

ok

continue;
}
should_print = true;
tbl << stringify(pid) << poolname << imgname << snapname << "/dev/nbd" + stringify(index) << TextTable::endrow;

This comment has been minimized.

@trociny

trociny May 28, 2017

Contributor

Split the line on 80 characters.
Do you need to stringify pid? May be yes, I just don't remember if TextTable supports it.

This comment has been minimized.

@liupan1111

liupan1111 May 29, 2017

Contributor

I just tried, and found stringify is not needed, thanks

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented May 29, 2017

@trociny updated, thanks.

char* arg;
for (unsigned i = 0; i < cmdline.size(); i++) {
arg = &cmdline[i];

This comment has been minimized.

@trociny

trociny May 29, 2017

Contributor

Do arg declaration here: char *arg = &cmdline[i];

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

ok

arg = &cmdline[i];
if (i == 0) {
if (strcmp(basename(arg) , "rbd-nbd") != 0) {
return EXIT_FAILURE;

This comment has been minimized.

@trociny

trociny May 29, 2017

Contributor

Make your function return a negative error code, e.g. -EINVAL, to follow the convention. The rbd_nbd() is the only exception because its value it returned by main, though I would not mind if you change it too, and modify main to:

int r = rbd_nbd(argc, argv);
if (r < 0) {
  return EXIT_FAILURE;
}
...

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

Yes, I agree, I will change them totally.

args.push_back(arg);
while (cmdline[i] != '\0') {
i++;
}

This comment has been minimized.

@trociny

trociny May 29, 2017

Contributor

I think you can avoid additional while loop, e.g. doing like below in pseudo code:

for (i = 0; i < cmdline; i++) {
  ...
  if  (i == 0) {
    if (arg != "rbd-nbd") {
      return -EINVAL;
    }
  } else {
    args.push_back(arg);
  }
  while (cmdline[i] != 0) {
    i++
  }
}

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

let me try

_sudo rbd-nbd list-mapped | grep "^${DEV}$"
_sudo rbd-nbd list-mapped | grep "${DEV} $"
_sudo rbd-nbd list-mapped | grep "${POOL}"
_sudo rbd-nbd list-mapped | grep "${IMAGE}"

This comment has been minimized.

@trociny

trociny May 29, 2017

Contributor

Theoretically, it might give false positive, e.g. if "$POOL" is matched for some other mapped image. I would prefer if you test for all values simultaneously. Additionally you could extract pid:

PID=`_sudo rbd-nbd list-mapped | awk '$2 == '${DEV}' && $3 == '${POOL}' && $4 == '${IMAGE}' {print $1}'`
test -n "${PID}"

Then you could use PID to check it matches rbd-nbd process:

ps -p ${PID} -o cmd | grep rbd-nbd

and also add a test that would kill rbd-nbd process by PID and test the process has died and the device is properly cleaned up.

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

cool!

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

just one thing: sudo is not needed for list-mapped any more.

This comment has been minimized.

@trociny

trociny May 30, 2017

Contributor

ok

argv_to_vec(argc, argv, args);
r = parse_args(args);
if (r == EXIT_FAILURE) {

This comment has been minimized.

@trociny

trociny May 29, 2017

Contributor

parse_args() should also return a negative error code instead of EXIT_FAILURE.

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

yes

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented May 30, 2017

@trociny done.

}
should_print = true;
if (snapname.empty()) {
snapname = "-";

This comment has been minimized.

@dillaman

dillaman May 30, 2017

Contributor

Nit: why not leave the snap name blank? "-" is a perfectly valid snapshot name but it's being used here to represent a non-snapshot.

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

for the result of "rbd showmapped", if an image was mapped without specified snapshot, it would display "-", I prefer to keep consistent:

$rbd showmapped
id pool image          snap device
0  rbd  image_test. -        /dev/nbd0

In addition, "device" is the fifth column in the list-mapped result. But when I modified qa/workunits/rbd-nbd.sh, if have to use $4 for blank name, because the value was empty.

For the two reasons above, I prefer "-" instead of blank name

return 0;
}
static int rbd_nbd(int argc, const char *argv[])
static int parse_args(vector<const char*>& args)

This comment has been minimized.

@dillaman

dillaman May 30, 2017

Contributor

I never really liked the use of global variables by this function, but I could previously ignore it since it was only used once. Now that this function is being re-used (which is great), I would prefer to see the arguments properly passed via parameters instead of global variables.

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

which global variables?

This comment has been minimized.

@trociny

trociny Jun 1, 2017

Contributor

devpath, poolname, imagename, nbds_max, readonly, cmd...

I think @dillaman meant something like this:

int parse_args(vector<const char*>& args, int *cmd, std::string *devpath, bool *readonly, ...);

And you can pass nullptr for params you are not interested in. Or (because the param list is rather long) you could create a Config structure and pass it to parse_args:

struct Config {
  std::string devpath;
  std::string poolname;
  ....
}
int parse_args(vector<const char*>& args, Config *config);

This comment has been minimized.

@dillaman

This comment has been minimized.

@liupan1111

liupan1111 Jun 5, 2017

Contributor

ok

ps -p ${PID} -o cmd | grep rbd-nbd
_sudo kill ${PID}
sleep 1
rbd-nbd list-mapped | expect_false grep "${DEV}"

This comment has been minimized.

@trociny

trociny May 30, 2017

Contributor

sleep 1 might not be enough. I would suggest a while loop, something like below:

for i in `seq 10`; do
  rbd-nbd list-mapped | expect_false grep ... && break
  sleep 1
done
rbd-nbd list-mapped | expect_false grep ...

Also, the grep pattern could be stricter, eg: "^ *${PID} .* ${DEV} " (I suppose TextTable uses spaces for tabbing).

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

rbd-nbd.sh may call cleanup and quit when meet any error, so “rbd-nbd list-mapped | expect_false grep ...” may make the test quit in first loop.

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

maybe we could change sleep 1 to sleep 10 directly? Since max 9 second extra wait is not a big deal for a test in my opinion.

This comment has been minimized.

@trociny

trociny May 30, 2017

Contributor

I wouldn't like to wait 10 sec when most of times it will need less then 1 sec. What if it has turned out that on teuthology 10 sec is not enough and we need 20 sec? Or if we want to add several test cases like this?

In my opinion the loop should work fine, I can't understand your concern about cleanup on error case.

This comment has been minimized.

@liupan1111

liupan1111 May 31, 2017

Contributor

In the beginning of rbd-nbd.sh, there is:
!/bin/bash -ex
That means if there is any error happens during the shell excution, it will quit. "cleanup" is a function name defined in rbd-nbd.sh, in order to unmap nbd and rm image before quit.

So I am afraid:
rbd-nbd list-mapped | expect_false grep ...
This will cause the shell quit in first iteration. What is your opinion?

This comment has been minimized.

@trociny

trociny May 31, 2017

Contributor

It would have caused the shell to quit if it had not had && break part.

The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test following the if or elif reserved words, part of any command executed in a && or || list except the command following the final && or ||, any command in a pipeline but the last, or if the command's return value is being inverted with !.

This comment has been minimized.

@liupan1111

liupan1111 May 31, 2017

Contributor

cool

ifs >> cmdline;
for (unsigned i = 0; i < cmdline.size(); i++) {
char *arg = &cmdline[i];

This comment has been minimized.

@trociny

trociny May 30, 2017

Contributor

Why not const char *?

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

ok

if (should_print) {
cout << tbl;

This comment has been minimized.

@trociny

trociny May 30, 2017

Contributor

I have not a strong opinion, but why not always output table if it is even empty?

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

also from the behavior of rbd showmapped...

The option for my customers is: rbd showmmaped is good enough for them, just keep rbd-nbd list-mapped same as rbd showmmaped if possible.

int r = rbd_nbd(argc, argv);
if (r < 0) {
return EXIT_FAILURE;
}

This comment has been minimized.

@trociny

trociny May 30, 2017

Contributor

Add return 0; at the end (success).

This comment has been minimized.

@liupan1111

liupan1111 May 30, 2017

Contributor

yes.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 1, 2017

@trociny updated.

@@ -849,20 +906,20 @@ static int rbd_nbd(int argc, const char *argv[])
} else if (ceph_argparse_witharg(args, i, &nbds_max, err, "--nbds_max", (char *)NULL)) {
if (!err.str().empty()) {
cerr << err.str() << std::endl;

This comment has been minimized.

@trociny

trociny Jun 1, 2017

Contributor

I still think it is a good idea to suppress error message when parsing cmdline. It could be a boolean param to parse_args to tell if it needs to print an error message, or std::ostream *err, so the message will be stored there, and you can print it or ignore after the function returns.

This comment has been minimized.

@liupan1111

liupan1111 Jun 5, 2017

Contributor

ok

std::vector<const char*>::iterator i;
std::ostringstream err;
md_config_t().parse_argv(args);
for (i = args.begin(); i != args.end(); ) {
if (ceph_argparse_flag(args, i, "-h", "--help", (char*)NULL)) {
usage();

This comment has been minimized.

@trociny

trociny Jun 1, 2017

Contributor

Calling usage() would be better to do outside. E.g you can return a special error here (e.g. -ENOENT or -ENODATA) to recognize this case.

This comment has been minimized.

@liupan1111

liupan1111 Jun 5, 2017

Contributor

I didn't catch it very clear. For this "if" branch, usage() will be called when the user input "ceph -h" or ceph "ceph --help". So I think it is right to be inside.

This comment has been minimized.

@tchaikov

tchaikov Jun 5, 2017

Contributor

@liupan1111 i am not sure if this is what @trociny 's opinion. but i guess what he suggests is, let parse_args() return -ENOENT or -ENODATA in case of -h or --help. and handle that errno at where parse_args() is called.

This comment has been minimized.

@liupan1111

liupan1111 Jun 5, 2017

Contributor

@tchaikov thanks for comment. does it need to return -ENOENT or -ENODATA in case of -h or ---help? The user only want to get help info in my thought...

This comment has been minimized.

@dillaman

dillaman Jun 5, 2017

Contributor

@liupan1111 Those errors codes were just an example.

This comment has been minimized.

@trociny

trociny Jun 5, 2017

Contributor

@liupan1111 Yes I meant exactly what @tchaikov and @dillaman say. The idea is to suppress in some way output when parse_args() is used to parse command line of already running process (yes, it is highly unlikely to have such situation, still it is not impossible).

For this I would prefer if usage() was moved out from parse_args(), but any other solution (like passing a bool parameter) would be ok to me too.

This comment has been minimized.

@trociny

trociny Jun 6, 2017

Contributor

No. I meant like below:

int parse_args(...) {
  ...
     if (ceph_argparse_flag(args, i, "-h", "--help", (char*)NULL)) {
       return -ENODATA;
  ...
}

int rbd_nbd(...) {
  ...
  r = parse_args(args);
  if (r == -ENODATA) {
    usage();
    return 0;
  }
  if (r < 0) {
     return r;
  }
  ...
}

If you don't like returning a negative error code in this case, you can return a positive value and check for it instead (make sure to update do_list_mapped_devices to handles this properly too then).

This comment has been minimized.

@liupan1111

liupan1111 Jun 6, 2017

Contributor

Please let me know if I didnot catch your idea:
Just move usage() outside parse_args, and return an error code if user input help(Such as ENODATA), and handle The return value of parse_args, if return ENODATA, print usage()

am i right?

This comment has been minimized.

@liupan1111

This comment has been minimized.

@liupan1111

liupan1111 Jun 6, 2017

Contributor

@trociny agree, thanks.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 7, 2017

@trociny , updated, thanks.

return 0;
}
static int rbd_nbd(int argc, const char *argv[])
static int parse_args(vector<const char*>& args, std::string& err_msg)

This comment has been minimized.

@trociny

trociny Jun 7, 2017

Contributor

The convention is to pass modified parameters by pointer, so please use std::string *err_msg. Then you could also just pass nullptr as an argument when you don't need a error message.

And you could consider using std::ostream *err_msg instead. Then you wouldn't need constructions like err_msg = std::string(...) + .

I will not object if you don't want to change this though.

This comment has been minimized.

@liupan1111

liupan1111 Jun 7, 2017

Contributor

std::ostream is better, I'd like to change.

std::string err_msg;
r = parse_args(args, err_msg);
if (r == -ENODATA) {

This comment has been minimized.

@trociny

trociny Jun 7, 2017

Contributor

usage();

This comment has been minimized.

@liupan1111

liupan1111 Jun 7, 2017

Contributor

oh, sorry.

} else if (ceph_argparse_witharg(args, i, &devpath, "--device", (char *)NULL)) {
} else if (ceph_argparse_witharg(args, i, &nbds_max, err, "--nbds_max", (char *)NULL)) {
if (!err.str().empty()) {
cerr << err.str() << std::endl;
return EXIT_FAILURE;
err_msg = err.str();

This comment has been minimized.

@trociny

trociny Jun 7, 2017

Contributor

While you are here, it would be good to add "rbd-nbd: " prefix here and when parsing "--max_part" below. It looks like these are only error messages without this prefix. Alternatively you could remove "rbd-nbd: " prefix from all messages in this function and add it when printing the error message in rbd_nbd().

This comment has been minimized.

@liupan1111

liupan1111 Jun 7, 2017

Contributor

agree.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 8, 2017

retest this please

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 8, 2017

@trociny updated, thanks.

}
}

This comment has been minimized.

@trociny

trociny Jun 8, 2017

Contributor

minor: extra empty line

This comment has been minimized.

@liupan1111

liupan1111 Jun 8, 2017

Contributor

updated, thanks.

@trociny

@liupan1111 Recently the hardcoded default pool was removed (#15518) and now you have empty name in the pool column if the pool is not explicitly specified. Could you please update?

And what about to get rid of globals in parse_args as it has been suggested by @dillaman?

if (i == 0) {
if (strcmp(basename(arg) , "rbd-nbd") != 0) {
return -EINVAL;
}

This comment has been minimized.

@trociny

trociny Jun 9, 2017

Contributor

@liupan1111 Noticed in my editor, there is an extra space at the end of line here and below. Could you please remove.

This comment has been minimized.

@liupan1111

liupan1111 Jun 9, 2017

Contributor

Sure

if (strcmp(basename(arg) , "rbd-nbd") != 0) {
return -EINVAL;
}
} else {

This comment has been minimized.

@trociny

trociny Jun 9, 2017

Contributor

extra space

return -EINVAL;
}
} else {
args.push_back(arg);

This comment has been minimized.

@trociny

trociny Jun 9, 2017

Contributor

extra space

} else {
args.push_back(arg);
}

This comment has been minimized.

@trociny

trociny Jun 9, 2017

Contributor

extra space

}

This comment has been minimized.

@trociny

trociny Jun 9, 2017

Contributor

extra space

if (r < 0) {
index++;
continue;
}

This comment has been minimized.

@trociny

trociny Jun 9, 2017

Contributor

extra space

if (snapname.empty()) {
snapname = "-";
}
tbl << pid << poolname << imgname << snapname

This comment has been minimized.

@trociny

trociny Jun 9, 2017

Contributor

extra space

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 9, 2017

@trociny for global vars, i looked up The code, and found may need to change many places, maybe My new changes will also have new coding styles issue, format issue. I really want to ensure this list mapped function works fine in this PR. I will remove these global vars with another PR(closed before, I will reopen later)

@trociny

This comment has been minimized.

Contributor

trociny commented Jun 9, 2017

@liupan1111 I don't see a problem to do global cleanup in this PR, but will not object if @dillaman agrees.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jun 9, 2017

@liupan1111 I'd like the see the global variables cleaned up since otherwise this change just makes the current situation worse. You can separate the it into two commits (cleanup and then new functionality).

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 9, 2017

ok

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 12, 2017

@liupan1111 Recently the hardcoded default pool was removed (#15518) and now you have empty name in the pool column if the pool is not explicitly specified. Could you please update?

Sure, I will update it in this PR with a separate commit

@trociny

This comment has been minimized.

Contributor

trociny commented Jun 12, 2017

Sure, I will update it in this PR with a separate commit

why a separate commit?

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 12, 2017

Because I think it is different feature for this PR: remove default pool name. So same PR, but different commit. I am fine if you think it should be done in same commit of 69f17b1

@trociny

This comment has been minimized.

Contributor

trociny commented Jun 12, 2017

You just need to make your code work the same way as it had worked before #15518 was merged: properly display pool name in pool column (assuming default pool name if it was not specified in the command line). The difference only is that before #15518 was merged the default pool name had always been "rbd", and now it may be anything specified in config.

rbd-nbd: remove global variables.
Signed-off-by: Pan Liu <wanjun.lp@alibaba-inc.com>
@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 17, 2017

@dillaman @trociny updated as your requests, please take a look, thanks.

argv_to_vec(argc, argv, args);
md_config_t().parse_argv(args);
r = rados.conf_get("rbd_default_pool", default_pool_name);

This comment has been minimized.

@trociny

trociny Jun 17, 2017

Contributor

I think you don't need to initialise rados (moreover connect to the cluster) just to get "rbd_default_pool" config value. Also, global_init is not very good idea. Some time ago we changed the code to do global_init only for map command, to avoid error messages due to admin socket path collision.

You can use md_config_t directly, something like below:

md_config_t config();
config.parse_config_files(nullptr, nullptr, 0);
default_pool_name = config.rbd_default_pool;

There is still a possibility of incorrect result, if rbd-nbd map caller specified a different value for "rbd_default_pool" via CEPH_ARGS environment or command line args. This can be improved a little if you will look for "rbd_default_pool" not here but in parse_args(). See the comment below.

std::vector<const char*>::iterator i;
std::ostringstream err;
md_config_t().parse_argv(args);

This comment has been minimized.

@trociny

trociny Jun 17, 2017

Contributor

Instead of this you can do:

md_config_t config();
config.parse_config_files(nullptr, nullptr, 0);
config.parse_env();
config.parse_argv(args);
cfg->pool = config.rbd_default_pool;

And then if (cfg->poolname.empty()) ... code in do_map() can be removed as redundant.

This comment has been minimized.

@liupan1111

liupan1111 Jun 17, 2017

Contributor

cool! Thanks.

@@ -466,19 +484,19 @@ class NBDWatchCtx : public librbd::UpdateWatchCtx
}
};
static int open_device(const char* path, bool try_load_module = false)
static int open_device(const char* path, Config *cfg = NULL, bool try_load_module = false)

This comment has been minimized.

@trociny

trociny Jun 17, 2017

Contributor

The default to NULL looks like unnecessary and actually it will crash with this default.

This comment has been minimized.

@liupan1111

liupan1111 Jun 17, 2017

Contributor

I add this default value for do_unmap, which will call open_device. I tested and not crash

This comment has been minimized.

@trociny

trociny Jun 18, 2017

Contributor

Ok, it can't crash in do_unmap case. But it in general, if you allow a pointer to be null (and setting this as default suggests it is a valid use case), you need to check it every time you do pointer dereference.

open_device(path, nullptr, true) will crash, and although this combination is unlikely to be ever used this looks not very good. Probably I would try to rewrite open_device to check cfg at early stage and return if it is null.

But leaving it as it is now is ok to me too. I suggest only to use nullptr instead of NULL in a new code.

This comment has been minimized.

@liupan1111

liupan1111 Jun 18, 2017

Contributor

what is The difference between nullptr and NULL? I see many NULL are used in this file

This comment has been minimized.

@trociny

trociny Jun 18, 2017

Contributor

A short answer: with modern C++ use nullptr. For more details see e.g [1].

There is no much sense in changing the current code just to convert NULL to nullptr, but in a new code use nullptr.

[1] http://en.cppreference.com/w/cpp/language/nullptr

This comment has been minimized.

@liupan1111

liupan1111 Jun 18, 2017

Contributor

Got it.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 17, 2017

@trociny updated, thanks.

@trociny

Otherwise LGTM (before testing)

Config cfg;
vector<const char*> args;

This comment has been minimized.

@trociny

trociny Jun 18, 2017

Contributor

This looks like too many unnecessary empty lines too me. I think we can group these declarations in one section (not separating by empty lines).

This comment has been minimized.

@liupan1111

liupan1111 Jun 18, 2017

Contributor

agree

liupan1111 added some commits Jun 18, 2017

rbd-nbd: support to display pool/image/snap information which mapped to
a nbd device.

Signed-off-by: Pan Liu <wanjun.lp@alibaba-inc.com>
qa/workunits: update rbd-nbd.sh for the changes of rbd-nbd list-mapped.
Signed-off-by: Pan Liu <wanjun.lp@alibaba-inc.com>
@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 18, 2017

@trociny updated.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 21, 2017

@trociny Thanks!

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 22, 2017

@dillaman ping.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jun 22, 2017

lgtm

@trociny trociny changed the title from rbd-nbd: support to display pool/image/snap information which mapped to to rbd-nbd: display pool/image/snap information in list output Jun 22, 2017

@trociny trociny merged commit 0d26ae3 into ceph:master Jun 22, 2017

4 checks passed

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