Re: [RFC] Framework for easy distribution of SystemTap scripts.

[fche at redhat dot com (Frank Ch. Eigler)]

anithra wrote:

> diff --git a/scripts/build_pkg/README.stap-buildpkg b/scripts/build_pkg/README.stap-buildpkg
> new file mode 100644
> index 0000000..d87a3de
> --- /dev/null
> +++ b/scripts/build_pkg/README.stap-buildpkg

Could this be a normal man page (stap-buildpkg.1) ?

> +stap-buildpkg scriptfile [options] [stap-options]
> +Options:
> +       -n pkg_name   		: specify a package name
> +       -v pkg_version		: specify a package version
> +       -c pkg_config  		: provide config parameters through a file
> +       -p post_process script   : post processing script
> +       -h help_config		: help text for config parameters
> +       All additonal options will be passed to the stap command

> +pkg_name and pkg_version will be used to set the name and version
> +of the resulting executable.

Why does there need to be a separate 'version'?

> +An optional config file that contains script specific config
> +options and corresponding help entries can also be specified using
> +the -c and -h options respectively.

You need to elaborate what script-specific config options can be, what
they do, how they work.

> +A post processing script can be provided using the -p option.

You should specify what exactly this means.  A '/bin/sh -c' command
line to pipe stdout through?

> [...]
> +<package_name> [options] [parameters]
> +
> +Options:
> +   * --run -r   Runs the scripts
> +                for n minutes where n can be passed as a parameter. The default
> +                value is 10 minutes. Post processing is performed after the 
> +                script completes.
> +   * --start -s Invokes the script as a background process.
> +   * --stop -x  Stops the script and performs post processing.
> +   * --all      Same as --run
> +   * --help     Displays this usage text.
> +
> +Parameters:
> +
> +   * time=[x]   x is in minutes. Runs the script for x minutes. valid for
> +                --run(-r) o --start(-s) or --all(-a) options only

How does time=NNN relate to -r 'n'?

(We could add a stap and/or staprun option for a simple time-based
exit().)


> diff --git a/scripts/build_pkg/stap-buildpkg b/scripts/build_pkg/stap-buildpkg
> [...]
> +#Checking if script is run by root
> +if [ $EUID -ne 0 ]
> +then
> +	echo "ERROR :You need to be root to run this !"
> +	exit
> +fi

Why?


> +prog=stap-buildpkg
> +
> +echo_usage () {
> +  echo $"Usage: $prog scriptfile [options] [stap-options]"
> +  echo $"Options:"
> +  echo $"       -n pkg_name   		: specify a package name"
> +  echo $"       -v pkg_version		: specify a package version"
> +  echo $"       -c pkg_config  		: provide config parameters through a file"
> +  echo $"       -p post_process script   : post processing script"
> +  echo $"       -h help_config		: help text for config parameters"
> +  echo $"       All additonal options will be passed to the stap command"
> +}
> +
> +parse_args() {
> +while [ -n "$1" ]; do
> +     case "$1" in
> + [...]
> +        *)                     STAP_OPTIONS=$@ 
> +				echo $STAP_OPTIONS

You'll need to watch the quoting in these $foo variables.
They could contain spaces etc.


> +module_gen() {
> +stap -p4 -k -m $PKG_NAME.ko $script $STAP_OPTIONS
> +if [ -f $PKG_NAME.ko ]
> +then
> +        echo " Module $PKG_NAME.ko generated successfully ";
> +else
> +        echo " Module $PKG_NAME.ko generation failed. Exiting now... "
> +        rm -rf $TEMPDIR
> +        exit;
> +fi
> +}

Why are you running stap with '-k'?  If you're only saving the .ko result,
then let stap give it to you in the present working directory.


> [...]
> +echo "using systemtap runtime executables from $runtime rpm"
> +cp `rpm -ql $runtime | grep staprun | grep -v staprun.8.gz` $TEMPDIR/.
> +cp `rpm -ql $runtime | grep stapio` $TEMPDIR/.

Have you observed systemtap being installed as a relocatable rpm somewhere
other than /usr/* ?  If not, you could just copy /usr/bin/staprun etc. by
normal path name.


> [...]
> +if [ ! -f "$script" ]; then
> + echo "Script file does not exist"
> + echo "Please provide a valid script name"
> + echo_usage
> + exit
> +fi

(Could a user not specify the script contents with   stap-foo  -e 'SCRIPT' ?)

> [...]
> +#Cleanup the mess in temp dir
> +#rm -rf $TEMPDIR

I assume not actually cleaning up the temporary directory is temporary.


> diff --git a/scripts/build_pkg/template.buildpkg b/scripts/build_pkg/template.buildpkg
> [...]

FWIW, you could include this template within the main buildpkg script, as in
 
echo > $PKG << 'END'
function run() 
....
END



> @@ -0,0 +1,153 @@
> +#!/bin/bash
> +SCRIPT_LOC=/usr/tapsetrpms/TEMPLATE_PKG_NAME-TEMPLATE_PKG_VERSION

What is SCRIPT_LOC for?

> [...]
> +$RUN_DIR/staprun $RUN_DIR/TEMPLATE_PKG_NAME.ko > $RUN_DIR/TEMPLATE_PKG_NAME.out 2>&1 &
> +sleep $time
> +echo "Completed"
> +mv $RUN_DIR/TEMPLATE_PKG_NAME.out $RUN_DIR/TEMPLATE_PKG_NAME_${TS}.out
> +echo "Output file TEMPLATE_PKG_NAME_${TS}.out is in $RUN_DIR directory"

(Is all this echoing necessary verbiage?)

> +ps -ef | grep TEMPLATE_PKG_NAME | grep stapio | awk '{print $2}' | xargs kill -SIGINT

You wouldn't need this is you stored $! after the "staprun ... &" a few lines ago.


> +function helptext()
> [...]
> +# echo " ipaddress=[ip]	ip is a standard IPv4 address. The output will be filtered for this address"

Really?  Perhaps the framework-maker should have a filter-command
option such as -P 'grep 127.0.0.1' that it could pass through.


> +  FILE=$RUN_DIR/TEMPLATE_PKG_NAME.config
> +   # make sure file exist and readable
> +   if [ ! -f $FILE ]; then
> +  	echo "$FILE : does not exist"
> +   elif [ ! -r $FILE ]; then
> +  	echo "$FILE: cannot read"
> +   fi

Why does this template need to exist?  The framework maker script
could transcribe all of the options right into the output script.


- FChE