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.

diff --git a/runtime/dyninst/session_attributes.c b/runtime/dyninst/session_attributes.c
index 5004a26bc2eca37b0e696c99a090a7ea886012dd..4713d0b1d12b40ba484e330f40bf91b73c25f487 100644 (file)
--- 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 acdc687114cf7193f0ddb26837bcb8e4b8977d5d..9a73747401937ca7568695be07b81093879c9431 100644 (file)
--- 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<string>::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 (file)
index 0000000..50ffb7c
--- /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 (file)
index 0000000..b6fbd19
--- /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}
+}
+