Bug 242641

Created attachment 156190 [details]
dslimit script

(This is LTC attachment id 28366)

The attached script contains an IBM copyright, and does not have a licsense
statement providing for redistribution. I think we need an explicitly
redistributable version.

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|OPEN                        |ASSIGNED




------- Additional Comments From nivedita.com (prefers email at niv.com)  2007-06-26 16:17 EDT -------
Yep, this should be GPL, but the licensing stuff was sitting independently in
our tree. Copyright also needs an update. I'll post an updated version here
by tmrw. --Nivedita 

Created attachment 158080 [details]
dslimit

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28366|0                           |1
        is obsolete|                            |




------- Additional Comments From nivedita.com (prefers email at niv.com)  2007-06-27 20:01 EDT -------
 
Updated dslimit script

Updated dslimit patch with later version, and added GPL verbage. 

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |FIXEDAWAITINGTEST
         Resolution|                            |FIX_BY_IBM




------- Additional Comments From nivedita.com (prefers email at niv.com)  2007-06-27 20:03 EDT -------
Attached an updated dslimit script which is our current version,
and has license info etc. Please let me know if anything else is
needed. 

I'm not clear on what dslimit with a single argument is supposed to do. What is
the purpose of these lines?

elif [ $# -eq 1 ]; then
        ($@)

----- Additional Comments From jatriple.com  2007-06-29 14:23 EDT -------
Without an argument, dslimit just runs the command in a subshell.  The existing
soft CPU limit will apply to the command.

However, it looks like dslimit has a quoting bug when running commands; it uses
$@ rather than "$@", so it breaks when trying to run a script that needs
arguments.  For example:

$ cat ./countargs.sh
#!/bin/sh
echo $#
$ ./countargs.sh 1 2 3
3
$ ./countargs.sh "1 2 3"
1
$ dslimit 1 ./countargs.sh 1 2 3
3
$ dslimit 1 ./countargs.sh "1 2 3"
3 

Created attachment 158242 [details]
dslimit

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28915 [details]|0                           |1
        is obsolete|                            |




------- Additional Comments From mauery.com (prefers email at vernux.com)  2007-06-29 14:25 EDT -------
 
new version of dslimit with fixes

The usage messages shows that all arguments are optional.

With no arguments specified, dslimit merely executes ulimit -St, which prints
the current cpu time limit.

With one argument specified, dslimit executes the specified program with the
default cpu time limit (or in other words, it just executes prog [program
args...]

With more than one argument, it uses the first argument as the limit and
executes the remaining args as the program.

As I inspected this code, I feel that this is not a really good interface.  For
example, it is not possible to execute a program with one argument at the
default cpu limits.

dslimit top -d1

This would try to use 'top' as the argument for setting the cpu limit and fail.
 I feel the best way to deal with this would be to either do some fancier
parameter parsing to separate programs from arguments or get rid of the default
cpu limit case altogether.  It is not needed -- if you want to execute with the
default cpu limits, just execute the program, don't run dslimit.

I vote to remove the clause in question:
elif [ $# -eq 1 ]; then
	"$@"

I also changed the if statement to the usage clause to be more concise.  Now it
responds to /^[-]+h.*/ as a call for help, rather than just -h --h and -help (I
was going to add --help since that is my favorite, but that was just getting
too verbose). 

Created attachment 158244 [details]
dslimit

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28963|0                           |1
        is obsolete|                            |




------- Additional Comments From mauery.com (prefers email at vernux.com)  2007-06-29 14:34 EDT -------
 
dslimit fixed take 2

This version of dslimit fixes the quoting problems that josh pointed out and
uses a much simpler expression for usage (also pointed out by josh). 

----- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-06-29 14:47 EDT -------
I will write and attach a dslimit manpage this afternoon. 

Created attachment 158281 [details]
dslimit.1

----- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-06-29 22:44 EDT -------
 
manpage for dslimit

Manpage attached. 

As written, when the command excceds its CPU limit, it prints

CPU time limit exceeded "$@"

This change will make it print the commandline:

--- dslimit~    2007-07-02 15:26:10.000000000 -0500
+++ dslimit     2007-07-02 15:28:27.000000000 -0500
@@ -47,7 +47,10 @@
 if [ $# -eq 0 ]; then
        ulimit -St
 else
-       (ulimit -St $1; shift; "$@")
+       a="$1"
+       shift
+       b="$@"
+       eval "ulimit -St $a; $b"
 fi
 
 exit 0


I'm not the shell expert, so there may be a better way to accomplish it

Some more suggestions:

eval "ulimit -St $a && $b"

will cause the script to exit if ulimit fails. And changing the last line to

exit $?

will cause the script to exit with the exit status of the command (or ulimit, if
ulimit fails).

Created attachment 158379 [details]
proposed patch

A single argument should be treated as a syntax error. Here is my complete
proposed change.

Created attachment 158381 [details]
dslimit.patch

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29005|0                           |1
        is obsolete|                            |




------- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-02 17:30 EDT -------
 
No need for a subshell; print usage with one argument; set -e.

The patch in attachment 29005 breaks quoting:
$ ./dslimit 1 /tmp/countargs.sh "1 2 3"
3

This patch should address the same issues and not break quoting.  It uses set
-e to exit if any command fails, to avoid the need for &&.  Also, rather than
using a subshell, it just runs the commands in dslimit directly.  The dslimit
shell script itself provides the subshell to avoid running ulimit in the parent
shell.	Finally, this runs the command with exec rather than leaving the
dslimit shell script running.

With this patch:
$ ./dslimit 1 /tmp/countargs.sh 1 2 3
3
$ ./dslimit 1 /tmp/countargs.sh "1 2 3"
1
$ ./dslimit 1 /tmp/spin.sh
CPU time limit exceeded 

----- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-02 17:34 EDT -------
Also with the patched dslimit, to show that set -e works:

$ ./dslimit ... true
./dslimit: line 54: ulimit: ...: invalid number
$ echo $?
1 

One last suggestion regarding the usage text. How about:

dslimit [LIMIT PROG [ARGS ...]]

Where LIMIT is seconds or "unlimited"
etc.

----- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-10 13:37 EDT -------
I've implemented that suggested change to usage, along with a couple of minor
cleanups: removing a redundant descriptive comment in dslimit in favor of the
usage and the manpage, and exiting unsuccessfully when given incorrect
arguments.  Will attach new dslimit and manpage. 

Created attachment 158874 [details]
dslimit

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28964|0                           |1
        is obsolete|                            |
  Attachment #29006|0                           |1
        is obsolete|                            |




------- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-10 13:39 EDT -------
 
New dslimit with requested usage fix and other cleanups. 

Created attachment 158875 [details]
dslimit.1

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28968|0                           |1
        is obsolete|                            |




------- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-10 13:42 EDT -------
 
Manpage for dslimit 

Created attachment 159371 [details]
RPM-packaged dslimit.

This is my first pass at a src rpm for the dslimit package.

changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |FIXEDAWAITINGTEST
         Resolution|                            |FIX_BY_IBM




------- Additional Comments From jstultz.com (prefers email at johnstul.com)  2007-07-23 16:03 EDT -------
dslimit package is available in rh's repo. marking this as fixed awaiting test. 

----- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-23 16:12 EDT -------
ftp://partners.redhat.com/f227cee111ab00ba82e0e42dc49c63a2/x86_64/src/dslimit-1-1.el5rt.src.rpm
seems a bit broken; it contains a file named .tar.bz2 which actually contains a
*gzipped* tarball:
dslimit.tar.bz2: gzip compressed data, from Unix, last modified: Thu Jul 12
13:53:54 2007 

I believe you are mistaken. I downloaded the file from the URL and looked at it:

$ rpm2cpio dslimit-1-1.el5rt.src.rpm |cpio -tv
-rw-r--r--   1 root     root          723 Jul 17 14:48 dslimit.spec
-rw-r--r--   1 root     root         1265 Jul 17 14:48 dslimit.tar.bz2

I installed it with rpm -i and looked in the SOURCES directory:

$ ll SOURCES/
total 8
-rw-r--r-- 1 streeter streeter 1265 Jul 17 14:48 dslimit.tar.bz2

----- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-23 16:51 EDT -------
The output you just showed doesn't contradict what I said.  The rpm does have a
file named .tar.bz2, as you point out, but the contents of that file actually
have a gzipped tarball, not a bzip2ed tarball as the filename would suggest:

$ rpm2cpio dslimit-1-1.el5rt.src.rpm | cpio -iv
dslimit.spec
dslimit.tar.bz2
5 blocks
$ file dslimit.tar.bz2
dslimit.tar.bz2: gzip compressed data, from Unix, last modified: Thu Jul 12
13:53:54 2007
$ tar tvvjf dslimit.tar.bz2
bzip2: (stdin) is not a bzip2 file.
tar: Child returned status 2
tar: Error exit delayed from previous errors
$ tar tvvzf dslimit.tar.bz2
drwxrwxr-x streeter/streeter 0 2007-07-12 13:53 dslimit-1/
-rw-r--r-- streeter/streeter 123 2007-07-12 13:53 dslimit-1/Makefile
-rw-r--r-- streeter/streeter 1200 2007-07-12 12:09 dslimit-1/dslimit
-rw-r--r-- streeter/streeter 1199 2007-07-12 12:09 dslimit-1/dslimit.1 

I misunderstood. When you said there was a file named .tar.bz2, I though you
meant there was a file named .tar.bz2, not a filename ending in .tar.bz2

I'll try to figure out the best way to fix that. It isn't a bad bug, since rpm
does the right thing with it, but we may as well get it right.

----- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-23 18:02 EDT -------
Ah, sorry.  Poor choice of words; a file named with a .tar.bz2 extension, yes. :) 

I have pushed dslimit-1-2, which has a properly bzipped file.

----- Additional Comments From jatriple.com (prefers email at josht.ibm.com)  2007-07-30 16:09 EDT -------
Confirmed; dslimit-1-2 includes dslimit.tar.bz2 as a properly bzipped tarball. 
Thanks! 

------- Comment From sripathi.com 2008-01-31 05:13 EDT-------
Closing.