This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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.


Attachment: distributionframework_v2.patch
Description: Text document

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]