From 8f34d871e1c5382fe4cb59a4b474303281586d65 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Fri, 23 Jan 2015 14:46:03 -0800 Subject: [PATCH] PR17858: Just warn and continue for stapdyn unknown globals With staprun modules, the kernel just noted the unknown module option in dmesg, and everything continues. For stapdyn, we were getting a warning which set enough of an error state to skip the rest of initialization, including begin probes, but not enough to exit the script. Now unknown options just warn without setting error state, but invalid options like bad number formats will set an error and properly exit. --- runtime/dyninst/session_attributes.c | 69 ++++++++----------- stapdyn/mutator.cxx | 25 +++++-- .../systemtap.base/global_opt_invalid.exp | 30 ++++++++ .../systemtap.base/global_opt_unknown.exp | 30 ++++++++ 4 files changed, 109 insertions(+), 45 deletions(-) create mode 100644 testsuite/systemtap.base/global_opt_invalid.exp create mode 100644 testsuite/systemtap.base/global_opt_unknown.exp diff --git a/runtime/dyninst/session_attributes.c b/runtime/dyninst/session_attributes.c index 5004a26bc..4713d0b1d 100644 --- a/runtime/dyninst/session_attributes.c +++ b/runtime/dyninst/session_attributes.c @@ -31,47 +31,38 @@ static int stp_session_attribute_setter(const char *name, const char *value) { // Note that We start all internal variables with '@', since // that can't start a "real" variable. - if (strcmp(name, "@log_level") == 0) { - _stp_init_session_attributes.log_level = strtoul(value, NULL, - 10); - return 0; - } - else if (strcmp(name, "@suppress_warnings") == 0) { - _stp_init_session_attributes.suppress_warnings - = strtoul(value, NULL, 10); - return 0; - } - else if (strcmp(name, "@stp_pid") == 0) { - _stp_init_session_attributes.stp_pid - = strtoul(value, NULL, 10); - return 0; - } - else if (strcmp(name, "@tz_gmtoff") == 0) { - _stp_init_session_attributes.tz_gmtoff - = strtol(value, NULL, 10); - return 0; - } - else if (strcmp(name, "@tz_name") == 0) { - strlcpy(_stp_init_session_attributes.tz_name, - value, MAXSTRINGLEN); - return 0; - } - else if (strcmp(name, "@target") == 0) { - _stp_init_session_attributes.target - = strtoul(value, NULL, 10); - return 0; - } - else if (strcmp(name, "@module_name") == 0) { - strlcpy(_stp_init_session_attributes.module_name, - value, MAXSTRINGLEN); - return 0; + + struct _stp_session_attributes *init = &_stp_init_session_attributes; + +#define set_num(field, type) \ + if (strcmp(name, "@" #field) == 0) { \ + char *endp = NULL; \ + errno = 0; \ + init->field = strto##type(value, &endp, 0); \ + return (endp == value || *endp != '\0') ? \ + -EINVAL : -errno; \ } - else if (strcmp(name, "@outfile_name") == 0) { - strlcpy(_stp_init_session_attributes.outfile_name, - value, PATH_MAX); - return 0; + +#define set_string(field) \ + if (strcmp(name, "@" #field) == 0) { \ + size_t size = sizeof(init->field); \ + size_t len = strlcpy(init->field, value, size); \ + return (len < size) ? 0 : -EOVERFLOW; \ } - return -EINVAL; + + set_num(log_level, ul); + set_num(suppress_warnings, ul); + set_num(stp_pid, ul); + set_num(target, ul); + set_num(tz_gmtoff, l); + set_string(tz_name); + set_string(module_name); + set_string(outfile_name); + +#undef set_num +#undef set_string + + return -ENOENT; } #endif // SESSION_ATTRIBUTES_C diff --git a/stapdyn/mutator.cxx b/stapdyn/mutator.cxx index acdc68711..9a7374740 100644 --- a/stapdyn/mutator.cxx +++ b/stapdyn/mutator.cxx @@ -314,7 +314,7 @@ mutator::init_modoptions() { // Hypothetical backwards compatibility with older stapdyn: stapwarn() << "Compiled module does not support -G globals" << endl; - return false; + return true; // soft warning; let it go on anyway } for (vector::iterator it = modoptions.begin(); @@ -327,17 +327,23 @@ mutator::init_modoptions() string::size_type separator = modoption.find('='); if (separator == string::npos) { - stapwarn() << "Could not parse module option '" << modoption << "'" << endl; + staperror() << "Could not parse module option '" << modoption << "'" << endl; return false; // XXX: perhaps ignore the option instead? } string name = modoption.substr(0, separator); string value = modoption.substr(separator+1); int rc = global_setter(name.c_str(), value.c_str()); - if (rc != 0) + if (rc == -ENOENT) { - stapwarn() << "Incorrect module option '" << modoption << "'" << endl; - return false; // XXX: perhaps ignore the option instead? + stapwarn() << "Ignoring unknown module option '" << name << "'" << endl; + continue; // soft warning; let it go on anyway + } + else if (rc != 0) + { + staperror() << "module option '" << modoption << "': " + << strerror(abs(rc)) << endl; + return false; } } @@ -557,7 +563,14 @@ mutator::run () stapwarn() << "process probes require a target (-c or -x)" << endl; // Get the stap module ready... - run_module_init(); + if (!run_module_init()) + { + // Detach from everything + target_mutatee.reset(); + mutatees.clear(); + + return false; + } // And away we go! if (target_mutatee) diff --git a/testsuite/systemtap.base/global_opt_invalid.exp b/testsuite/systemtap.base/global_opt_invalid.exp new file mode 100644 index 000000000..50ffb7cab --- /dev/null +++ b/testsuite/systemtap.base/global_opt_invalid.exp @@ -0,0 +1,30 @@ +# Check stap global variable option (-G) with an invalid number + +set test "global_opt_invalid" +if {![installtest_p]} { + untested $test + return +} + +set script "global var1=9 ; probe begin { if (var1 == 9) println(\"systemtap test success\") else println(\"systemtap test failure\") ; exit() }" + +foreach runtime [get_runtime_list] { + set test "global_opt_invalid" + if {$runtime != ""} { + set test "${test}_${runtime}" + spawn stap -G var1=z29 -e $script --runtime=$runtime + } else { + spawn stap -G var1=z29 -e $script + } + # staprun says "Invalid parameters"; stapdyn says "Invalid argument" + expect { + -timeout 60 + -re {ERROR.*Invalid} { pass $test } + "systemtap test success" { fail $test } + "systemtap test failure" { fail $test } + timeout {fail "$test: unexpected timeout"} + eof {fail "$test: unexpected EOF"} + } + catch {close}; catch {wait} +} + diff --git a/testsuite/systemtap.base/global_opt_unknown.exp b/testsuite/systemtap.base/global_opt_unknown.exp new file mode 100644 index 000000000..b6fbd1948 --- /dev/null +++ b/testsuite/systemtap.base/global_opt_unknown.exp @@ -0,0 +1,30 @@ +# Check stap global variable option (-G) for unknown names + +set test "global_opt_unknown" +if {![installtest_p]} { + untested $test + return +} + +set script "global var1=9 ; probe begin { if (var1 == 9) println(\"systemtap test success\") else println(\"systemtap test failure\") ; exit() }" + +foreach runtime [get_runtime_list] { + set test "global_opt_unknown" + if {$runtime != ""} { + set test "${test}_${runtime}" + spawn stap -G var2=29 -e $script --runtime=$runtime + } else { + spawn stap -G var2=29 -e $script + } + # stapdyn prints a warning on stderr, but staprun modules print into dmesg. + # Either is fine as long as we still get the expected output from probe begin. + expect { + -timeout 60 + "systemtap test success" { pass $test } + "systemtap test failure" { fail $test } + timeout {fail "$test: unexpected timeout"} + eof {fail "$test: unexpected EOF"} + } + catch {close}; catch {wait} +} + -- 2.43.5