Bug 11224

Summary: setting module arguments needs to be tested/documented
Product: systemtap Reporter: David Smith <dsmith>
Component: documentationAssignee: Charley <chwang>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: Short test framework for module arguments
Sample stp file for modargs testing
Sample stp file for modargs testing
Brief module arguments test
Module arguments test exp file
Short script
Documentation patch

Description David Smith 2010-01-26 22:15:02 UTC
Systemtap can set module arguments on the command line, in certain situations. 
This is not documented (well) in a man page or tested by the testsuite.  Here's
what I've discovered about this after some investigation.

Here's an example script (called mod_opt1.stp):

  global name
  probe begin { printf("name=%s\n", name); exit() }

Global variables get converted into module options. To set 'name' from the
command line, you have to do this:

  # stap -p4 -m mod_opt1 mod_opt1.stp
  # staprun mod_opt1.ko name=foo
  name=foo

Note that you can't do the following, since there doesn't seem to be a way to
set module options from the 'stap' command line:

  # stap -m mod_opt1 mod_opt1.stp name=foo

Things get a bit more confusing when you add in script arguments (which can only
be specified on the 'stap' command line).

Here's an example script using script arguments (called mod_opt2.stp):

  global name=@1
  probe begin { printf("name=%s\n", name); exit() }

To use script arguments, you'd do this:

  # stap -m mod_opt2 mod_opt2.stp foo
  name=foo

If you re-run the module, you'll get the same output.

  # staprun mod_opt2.ko
  name=foo

Using that same module, you can override the script argument setting using
module arguments.

  # staprun mod_opt2.ko name=bar
  name=bar

(It is possible that this last behavior has changed at some point.  Systemtap
used to synthesize begin probes to set global variables, and the global variable
setting might have overridden the module argument.)

Note that this bug is related to, but not the same thing as, bug #3849 and bug
#5642.
Comment 1 Frank Ch. Eigler 2010-01-26 22:28:25 UTC
Now that I think about it a little more, it makes more and more sense to
have the translator detect that globals are initialized at the script
level, and to block potential setting via module options (by omitting
the MODULE_PARAM goo for those variables).  This would remove any ambiguity.  

Further, it would help with a background security concern, giving script
authors the ability to *block* staprun users (such as restricted-privileged
staprun people) from overriding selected globals that could change the
precompiled module's behavior.
Comment 2 David Smith 2010-01-26 22:37:54 UTC
(In reply to comment #1)
> Now that I think about it a little more, it makes more and more sense to
> have the translator detect that globals are initialized at the script
> level, and to block potential setting via module options (by omitting
> the MODULE_PARAM goo for those variables).  This would remove any ambiguity.  

I see your point, but I also wonder if we lose something here.  For instance,
assume the following pseudo-script:

  global debug=0
  probe PROBE_POINT {
     if (debug) {
        # print interesting debug information
     }
  }

This allows a default 'debug' value of 0, which we then can override from the
command line using module arguments when we want the extra debug output.

If we omit the MODULE_PARAM goo for 'debug', we wouldn't be able to override the
default value from the command line (without recompiling).

Comment 3 Frank Ch. Eigler 2010-01-26 22:41:51 UTC
> I see your point, but I also wonder if we lose something here.  For instance,
> assume the following pseudo-script:
> 
>   global debug=0
>   probe PROBE_POINT {
>      if (debug) {
>         # print interesting debug information
>      }
>   }
> 
> This allows a default 'debug' value of 0, which we then can override from the
> command line using module arguments when we want the extra debug output.

One doesn't have to explicitly initialize anything to 0.  By leaving out
the =0, in my scheme one would permit runtime overrides.
Comment 4 Charley 2010-02-03 16:22:57 UTC
Created attachment 4570 [details]
Short test framework for module arguments

Wrote a quick little test framework for module arguments.

Workflow:

Create testsuite/modargs/test.stp (see attached one.stp for example)
In the .stp file:
Echo expected result to modargs/result
Echo arguments to modargs/args
Use stap -p4 -m test to create a module called test.ko

Then just run modargs.exp and it should handle the rest. Will check that module
can be created and then that the module returns the expected result.
Comment 5 Charley 2010-02-03 16:30:47 UTC
Created attachment 4571 [details]
Sample stp file for modargs testing

Does very, very, very basic testing of modargs -- setting a number/string and
printing it.

WRT to the discussion about default values and overwriting them, I think it's
fine either way. But there should be an easy way to mark values as
'no-overwrite,' and it may be a good idea to make globals 'no-overwrite' by
default (to avoid accidental or intentional tampering with values)? I'm not
sure, but it seems like if we are going to make this feature optional it would
be safer as opt-in rather than opt-out.

Also, it would be nice from a user/scripting perspective if staprun one.ko
nonexistant_variable=a would run with a warning instead of terminating with an
error :)
Comment 6 Charley 2010-02-03 21:22:31 UTC
Created attachment 4572 [details]
Sample stp file for modargs testing

Does very, very, very basic testing of modargs -- setting a number/string and
printing it.

WRT to the discussion about default values and overwriting them, I think it's
fine either way. But there should be an easy way to mark values as
'no-overwrite,' and it may be a good idea to make globals 'no-overwrite' by
default (to avoid accidental or intentional tampering with values)? I'm not
sure, but it seems like if we are going to make this feature optional it would
be safer as opt-in rather than opt-out.

Also, it would be nice from a user/scripting perspective if staprun one.ko
nonexistant_variable=a would run with a warning instead of terminating with an
error :)
Comment 7 Josh Stone 2010-02-03 21:36:29 UTC
(In reply to comment #6)
> [...] it seems like if we are going to make this feature optional it would
> be safer as opt-in rather than opt-out.

I agree, and I would prefer for this to be more explicit than just whether it
was initialized or not.  One possibility is a new keyword to augment or replace
the global keyword for parameterized variables. (e.g. extern/param/...)

> Also, it would be nice from a user/scripting perspective if staprun one.ko
> nonexistant_variable=a would run with a warning instead of terminating with an
> error :)

I disagree, because then you get the class of bug reports like "Why doesn't
setting this parameter change anything?  Oh, because I used the wrong name..."

I think this is not under our control anyway, because we're using the kernel's
module-parameter facility.  We could perhaps improve the error message though.
Comment 8 Frank Ch. Eigler 2010-02-04 19:57:08 UTC
(In reply to comment #4)
> Created an attachment (id=4570)
> Short test framework for module arguments

Consider skipping the framework and just running
stap -p4 / staprun on some one-liner script, 
right in the .exp file.  Something like the
pr10854 test case, but even smaller.

As for documentation, a good start would be 
a few sentences in staprun.8.in regarding MODULE-OPTIONS,
and a blurb in stap.1.in to identify this aspect of global
variables.
Comment 9 Charley 2010-02-24 17:20:25 UTC
Created attachment 4625 [details]
Brief module arguments test

I made the result string "name=value number=value", so on a successful run the
return value should be the same as the module arguments. See also modargs.stp.
Comment 10 Charley 2010-02-24 17:27:11 UTC
Created attachment 4626 [details]
Module arguments test exp file

Expanded test slightly to test setting initialized/uninitialized values
(passing -w in the stap -p4 command to ignore warnings about uninitialized
variables). Also added a negative number.

Sets these values:
initializedname=foo initializednumber=999 name=charley number=-1
Comment 11 Charley 2010-02-24 17:28:15 UTC
Created attachment 4627 [details]
Short script

Short script for testing module argument setting. Has 2 initialized and 2
uninitialized variables.
Comment 12 Frank Ch. Eigler 2010-02-25 18:46:41 UTC
Charley, this test case is fine to commit.
FWIW, I'd make the .exp file less verbose, and simplify the pass / fail
messages to the bare essentials:

  pass "$test compilation"
  fail "$test execution"

etc..  Likewise, there is little need to log the expected/actual data
with so much frill.  If they don't already show up in the logs automatically, 
just print the strings.  (The expected one isn't really necessary since one
can simply inspect the test case.)  So basically KISS.
Comment 13 Charley 2010-02-25 20:58:46 UTC
Created attachment 4630 [details]
Documentation patch

Blurb in Variables section of stap.1.in to briefly state that globals can be
changed on the command line as a module argument. Added to ARGUMENTS and
OPTIONS sections of staprun.8.in. If there are no major objections I will go
ahead and push :)
Comment 14 Frank Ch. Eigler 2010-02-25 22:50:27 UTC
thanks