Ubuntu

Merge lp:~er-abhinav-upadhyay/ubuntu/natty/tomcat6/bug707405 into lp:ubuntu/natty/tomcat6

Proposed by Abhinav Upadhyay on 2011-03-07
Status: Merged
Merged at revision: 33
Proposed branch: lp:~er-abhinav-upadhyay/ubuntu/natty/tomcat6/bug707405
Merge into: lp:ubuntu/natty/tomcat6
Diff against target: 43 lines (+20/-2) 2 files modified
To merge this branch: bzr merge lp:~er-abhinav-upadhyay/ubuntu/natty/tomcat6/bug707405
Reviewer Review Type Date Requested Status
Dave Walker Approve on 2011-03-09
Barry Warsaw Needs Fixing on 2011-03-07
Clint Byrum 2011-03-08 Pending
Ubuntu branches 2011-03-07 Pending
Review via email: mp+52379@code.launchpad.net

Commit Message

tomcat6-instance-create should accept -1 as the value of -c option
  as per http://tomcat.apache.org/tomcat-6.0-doc/config/server.html (LP: #707405)

Description of the Change

As per the bug #707405, tomcat6-instance-create should accept -1 as the value of the -c option.

I have modified the tomcat6-instance-create script so that it will now accept -1 as a value for -c option. It will configure the server alright, but it will give the user a warning message, that he will have to kill the server manually.

To post a comment you must log in.
Clint Byrum (clint-fewbar) wrote :

As an alternative one could use this for number detection:

if ! echo $port | grep -Eq '^\-?[0-9]*$' ; then

Advantages:
 * more obvious match of exact *type* of number expected (integers only with optional leading minus)
 * no redirect of errors to /dev/null (in case something else goes wrong not involving the failure you're trying to trigger)

Barry Warsaw (barry) wrote :

Hi Abhinav,

Thanks for taking the time to improve Ubuntu!

Shouldn't that regexp be:

'^\-?[0-9]+$'

IOW, there must be at least one digit? Otherwise '-' alone will match, but it's not a number so the -eq will fail later on.

Other than that, I think Clint's approach makes sense. Would you consider making this change?

review: Needs Fixing

> Hi Abhinav,
>
> Thanks for taking the time to improve Ubuntu!
>
> Shouldn't that regexp be:
>
> '^\-?[0-9]+$'
>
> IOW, there must be at least one digit? Otherwise '-' alone will match, but
> it's not a number so the -eq will fail later on.
>
> Other than that, I think Clint's approach makes sense. Would you consider
> making this change?

Yes, I am making the changes and will be pushing the changes.
Thanks for review it :-)

34. By Abhinav Upadhyay on 2011-03-08

tomcat6-instance-create: Now, using a regex test to check if port numbers are valid numbers or not.

Hi,
Thanks to both of you for review my merge proposal :-)

I have replaced the previous port number test with the regex test as both of you had suggested. I also tested it with all kinds of inputs, and it worked ok.
I request for another review of the patch.

Thanks
Abhinav

Dave Walker (davewalker) wrote :

Thanks for fixing this and improving the change based on comments. I have now uploaded it, so thank you for your contribution.

I spoke with one of the Debian maintainers of this package the other day, and he agreed with fixing it. I don't believe you've submitted it there, so doing that now.

Thanks.

review: Approve

Dave: Thanks a lot for reviewing and approving it. Yes, I did not submit it upstream, thank you for doing that as well. :-)

Preview Diff

1=== modified file 'debian/changelog'
2--- debian/changelog 2011-02-09 21:49:33 +0000
3+++ debian/changelog 2011-03-08 04:17:10 +0000
4@@ -1,3 +1,10 @@
5+tomcat6 (6.0.28-10ubuntu1) natty; urgency=low
6+
7+ * tomcat6-instance-create should accept -1 as the value of -c option
8+ as per http://tomcat.apache.org/tomcat-6.0-doc/config/server.html (LP: #707405)
9+
10+ -- Abhinav Upadhyay <er.abhinav.upadhyay@gmail.com> Mon, 07 Mar 2011 13:38:05 +0530
11+
12 tomcat6 (6.0.28-10) unstable; urgency=medium
13
14 * Team upload.
15
16=== modified file 'debian/tomcat6-instance-create'
17--- debian/tomcat6-instance-create 2010-05-21 13:51:15 +0000
18+++ debian/tomcat6-instance-create 2011-03-08 04:17:10 +0000
19@@ -23,11 +23,22 @@
20 type=$1
21 port=$2
22 # Fail if port is non-numeric
23- num=`expr ${port} + 1 2> /dev/null`
24- if [ $? != 0 ] || [ $num -lt 2 ]; then
25+ if ! echo $port | grep -Eq '^\-?[0-9]+$' ; then
26 echo "Error: ${type} port '${port}' is not a valid TCP port number."
27 exit 1
28 fi
29+
30+ # If Control port is -1 , no need to check any further.
31+ if [ "$type" = "Control" ] && [ $port -eq -1 ]; then
32+ echo "Warning: Control port disabled. You will have to shutdown the server manually, by using OS signals."
33+ return
34+ fi
35+
36+ # Fail if port is 0 or negative
37+ if [ $port -le 0 ]; then
38+ echo "Error: ${type} port '${port}' is not a valid TCP port number."
39+ exit 1
40+ fi
41
42 # Fail if port is above 65535
43 if [ ${port} -gt 65535 ]; then

Subscribers

People subscribed via source and target branches