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

common: fix daemon abnormal exit at parsing invalid arguments #17664

Merged
merged 2 commits into from Sep 15, 2017

Conversation

Projects
None yet
3 participants
@Yan-waller
Copy link
Contributor

commented Sep 12, 2017

@@ -492,7 +492,7 @@ int md_config_t::parse_argv(std::vector<const char*>& args)
set_val_or_die("client_mountpoint", val.c_str());
}
else {
parse_option(args, i, NULL);
parse_option(args, i, &cerr);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 12, 2017

Contributor

i don't think it is advisable to pass cerr here. if the caller does not expect error message on stderr, we should not. and stderr is not always available. instead, i'd suggest return -EINVAL here. and add a simple test in src/test/daemon_config.cc.

This comment has been minimized.

Copy link
@Yan-waller

Yan-waller Sep 12, 2017

Author Contributor

As the original logic, cerr is finally passed in to va_ceph_argparse_witharg(,,,cerr,), I'm not sure is this a minimal change that conform to original intent ?..

parse_option(args, i, NULL); ->
ceph_argparse_witharg(args, i, &val,
                      as_option.c_str(), (char*)NULL)); ->
va_ceph_argparse_witharg(args, i, ret, cerr, ap);
@@ -536,9 +536,11 @@ int md_config_t::parse_option(std::vector<const char*>& args,
std::string as_option("--");
as_option += "debug_";
as_option += subsys.get_name(o);
if (ceph_argparse_witharg(args, i, &val,
if (ceph_argparse_witharg(args, i, &val, *oss,

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 12, 2017

Contributor

instead i think we should update all ceph_argparse_witharg()/ceph_argparse_binary_flag() and their caller sites, so it returns an integer instead of a boolean.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 12, 2017

Contributor

@trociny and @jdurgin what do you think?

This comment has been minimized.

Copy link
@trociny

trociny Sep 12, 2017

Contributor

Yes, ceph_argparse_witharg()/ceph_argparse_binary_flag() interface looks unfortunate. Still I am not sure it worth changing at this stage.

Although we can't use the return value to tell if parsing failed, it looks it is safe to assume it failed if something was printed to stream provides as std::ostream &oss param.

I.e. I think the reported problem could be fixed by something like this:

    ostringstream err; 
    if (ceph_argparse_witharg(args, i, &val, err,
                             as_option.c_str(), (char*)NULL)) {
      if (!err.str().empty()) {
        if (oss) {
          *oss << err.str(); 
        }
        ret = -EINVAL;
        break;
      }

@Yan-waller Yan-waller force-pushed the Yan-waller:wip-walle-0912moninjectargs branch 3 times, most recently from 6118a27 to 102b76e Sep 12, 2017

@Yan-waller

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

@tchaikov I have fixed it as you and trociny's suggestions, could you help to take a look again? thanks.

as_option.c_str(), (char*)NULL)) {
if (!err.str().empty()) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 13, 2017

Contributor

nit, use tellp() instead of err.str().empty()

// We should complain about the missing arguments.
std::string injection3("--log-graylog-port 28 --debug_ms");
ret = g_ceph_context->_conf->injectargs(injection3, &cout);
ASSERT_EQ(ret, -EINVAL);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 13, 2017

Contributor
ASSERT_EQ(-EINVAL, ret);

as the 1st param is the expected value, the second one is the actual value.

common: fix daemon abnormal exit at parsing invalid arguments
Signed-off-by: Yan Jun <yan.jun8@zte.com.cn>

@Yan-waller Yan-waller force-pushed the Yan-waller:wip-walle-0912moninjectargs branch from 102b76e to ccaba81 Sep 13, 2017

test/config: regulate parameter order of ASSERT_EQ
Signed-off-by: Yan Jun <yan.jun8@zte.com.cn>
@Yan-waller

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

@tchaikov thanks, have done it. and incidentally, I adjust all ASSERT_EQ in daemon_config.cc file as another commit.

@tchaikov tchaikov added the needs-qa label Sep 13, 2017

@Yan-waller

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2017

retest this please

@tchaikov tchaikov merged commit 3643063 into ceph:master Sep 15, 2017

5 checks passed

Docs: build check OK - docs built
Details
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
You can’t perform that action at this time.