[RFC] Framework for easy distribution of SystemTap scripts (V2)

[Anithra P Janakiraman]

On 04/08/2010 09:17 PM, Frank Ch. Eigler wrote:

Hi Frank,

Thanks for the review! I've attached a V2 of the patch.

>
> 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) ?

We have placed the script in the scripts directory, We dont want to 
install the script, so didnt see the use for a man page.

>
>> +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'?
>

To be able to distinguish between packages built for different versions 
of the same script. We initially had a 'release' too :)

>> +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.
>

Will do.  The optional config could be pid, ip in case of networking 
scripts... Have added an example. The parameters will be passed to the 
post-processing script to filter the output.

>> +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?

The post processing script can do anything the administrator creating 
the package wishes to do, maybe even fwd the output to a support team, 
or provide suggestions based on the output. Have added an explanation.

>
>> [...]
>> +<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'?

Text changed

>
> (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?

hmmm. this was there for the initial rpm version, we had to check for 
root because rpmbuild was being used in the script. Have removed it now.
>
>
>> +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.

Didn't understand the comment. stap-buildpkg script.stp -n "package 
name" -v 1  would work.

>
>
>> +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.
>

changed

>
>> [...]
>> +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.
>
>
stapio can be installed in different locations, which is the reason for 
the above check. We do the same for staprun for the same of uniformity

>> [...]
>> +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' ?)
>

We are currently not providing this option.  Didnt think abt it 
actually. I'm assuming that if a package needs to be built for a script 
it would most likely not be a one-liner.

>> [...]
>> +#Cleanup the mess in temp dir
>> +#rm -rf $TEMPDIR
>
> I assume not actually cleaning up the temporary directory is temporary.

yes, uncommented.

>
>
>> 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
>

True. This is for simplicity of design. It is intuitively a template and 
a separate file, so we decided to keep it like that esp since we had no 
intentions of installing the build-pkg script.

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

Redundant, this was another carry over from the prev version of the 
script,  removed.

>
>> [...]
>> +$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?)

I think so :). When we demonstrated this we were actually asked for more 
verbosity

>
>> +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.

Yes. We had done it this way keeping the older versions of SystemTap in 
mind, the $! returns pid of 'staprun' in older versions. I've changed 
the template

>
>
>> +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.

The above help option was commented. Deleted it now.

>
>> +  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.

We want to provide a set of 'default' options. This framework could be 
used by administrators to create packages to diagnose problems on 
customer machines, in which case the admins might need to put in default 
values which they could use to filter the output. The config file will 
always exist because there would be atleast one default option (time).

Regards,
Anithra.


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: distributionframework_v2.patch
URL: <http://sourceware.org/pipermail/systemtap/attachments/20100409/c142a728/attachment.ksh>