admin@glassfish.java.net

Re: code review for issue 694

From: Byron Nevins <Byron.Nevins_at_Sun.COM>
Date: Wed, 20 Sep 2006 15:49:02 -0700

Oh -- I didn't notice that it was a UNIX template. Sorry for the
annoying review I gave.

Also, since you have the hood open anyways, java.lang.ProcessBuilder is
a cool replacement for Runtime.getRuntime()


Jane Young-Lau wrote:
> Hi Byron,
>
> Thanks for the review.
> I'll change it to use Runtime.getRuntime().exec(Sting[]).
> The changes that I made will also work but your suggestion is better.
>
> The script change (startserv.Tomcat.template and
> stopserv.Tomcat.template *NOT* startserv.Tomcat.bat.template and
> stopserv.Tomcat.bat.template) is for Unix, not for Windows. I've
> tested the change on Unix and Mac. Since the bug is reported on Mac
> and the same script is used on both Unix and Mac that is why I tested
> this on both OSs except for Windows.
>
> In Unix it's safe to declare:
> ASENV_CONF_LOCATION="/export/a b/glassfish"
>
> . "$ASENV_CONF_LOCATION/asenv.conf"
>
> Thanks,
>
> Jane
>
>
>
>
> Byron Nevins wrote:
>> Note to everyone:
>>
>> 90% of our problems with spaces in dir-names are because of calling
>> Runtime.exec(String) instead of Runtime.exec(String[])
>>
>> The reason that (String) doesn't work and (String[]) does work is
>> that the OS (both UNIX and Windows) treat a space in a OS level
>> command as a separator.
>> On the other hand, if the space is embedded inside a String in an
>> array, it is treated like any other character.
>>
>> I think the following ought to work better (if "args" is more than 1
>> arg, you'll have to break it into strings too)
>>
>> Runtime.getRuntime().exec(new String[] {"/bin/chmod", args,
>> file.getAbsolutePath()} );
>>
>> =============================================================
>> I don't like the looks of the script changes. Have you exhaustively
>> tested it?
>>
>> Below are the 4, and only 4, places that use ASENV_CONF_LOCATION in
>> startserv.bat in my installation:
>>
>> set ASENV_CONF_LOCATION=c:/ee/config
>> if exist "%ASENV_CONF_LOCATION%\asenv.bat" goto gotCmdPath
>> call "%ASENV_CONF_LOCATION%\asenv.bat"
>> -Dcom.sun.aas.configRoot="%ASENV_CONF_LOCATION%"
>>
>> ----------
>> Say AS is installed in "c:/a b". The current template will produce:
>> set ASENV_CONF_LOCATION=c:/a b
>> if exist "c:/a b\asenv.bat" goto gotCmdPath
>> call "c:/a b\asenv.bat"
>> -Dcom.sun.aas.configRoot="c:/a b"
>>
>> Your changes will produce this:
>>
>> set ASENV_CONF_LOCATION="c:/a b"
>> if exist ""c:/a b"\asenv.bat" goto gotCmdPath
>> call ""c:/a b"\asenv.bat"
>> -Dcom.sun.aas.configRoot="c:/a b"
>>
>> -- that lookjs like trouble to me!
>> =========================================================================
>>
>> Jane Young wrote:
>>> Hi,
>>>
>>> Please review fix for Issue 694 and CR 6443427:
>>>
>>> Diffs:
>>>
>>> Index:
>>> admin/servermgmt/src/java/com/sun/enterprise/admin/servermgmt/KeystoreManager.java
>>> ===================================================================
>>> RCS file:
>>> /cvs/glassfish/admin/servermgmt/src/java/com/sun/enterprise/admin/servermgmt/KeystoreManager.java,v
>>> retrieving revision 1.4
>>> diff -u -r1.4 KeystoreManager.java
>>> ---
>>> admin/servermgmt/src/java/com/sun/enterprise/admin/servermgmt/KeystoreManager.java
>>> 25 Dec 2005 03:44:02 -0000 1.4
>>> +++
>>> admin/servermgmt/src/java/com/sun/enterprise/admin/servermgmt/KeystoreManager.java
>>> 20 Sep 2006 19:11:40 -0000
>>> @@ -427,8 +427,11 @@
>>> protected void chmod(String args, File file) throws IOException
>>> {
>>> if (OS.isUNIX()) {
>>> + //fix issue 694: Runtime.getRunTime().exec(String) does not
>>> + //work if there is a space in the directory path, so
>>> need to specify the
>>> + //directory in Runtime.getRuntime().exec(String, null,
>>> directory_path).
>>> Runtime.getRuntime().exec("/bin/chmod " + args + " " +
>>> - file.getAbsolutePath());
>>> + file.getName(), null,
>>> file.getParentFile());
>>> }
>>> }
>>> }
>>>
>>> Index: admin/templates/pe80/startserv.tomcat.template
>>> ===================================================================
>>> RCS file:
>>> /cvs/glassfish/admin/templates/pe80/startserv.tomcat.template,v
>>> retrieving revision 1.3
>>> diff -u -r1.3 startserv.tomcat.template
>>> --- admin/templates/pe80/startserv.tomcat.template 22 Aug 2005
>>> 05:24:33 -0000 1.3
>>> +++ admin/templates/pe80/startserv.tomcat.template 20 Sep 2006
>>> 19:11:40 -0000
>>> @@ -8,7 +8,7 @@
>>>
>>> # Resolve links - $0 may be a softlink
>>> PRG="$0"
>>> -ASENV_CONF_LOCATION=%%%CONFIG_HOME%%%
>>> +ASENV_CONF_LOCATION="%%%CONFIG_HOME%%%"
>>>
>>> SERVER_NAME=%%%SERVER_NAME%%%
>>> DOMAIN_NAME=%%%DOMAIN_NAME%%%
>>> @@ -30,7 +30,7 @@
>>> . "$ASENV_CONF_LOCATION/asenv.conf"
>>> fi
>>>
>>> -INSTANCE_ROOT=%%%INSTANCE_ROOT%%%
>>> +INSTANCE_ROOT="%%%INSTANCE_ROOT%%%"
>>>
>>> JAVA_HOME="$AS_JAVA"
>>> # Make sure prerequisite environment variables are set
>>>
>>> Index: admin/templates/pe80/stopserv.tomcat.template
>>> ===================================================================
>>> RCS file: /cvs/glassfish/admin/templates/pe80/stopserv.tomcat.template,v
>>> retrieving revision 1.3
>>> diff -u -r1.3 stopserv.tomcat.template
>>> --- admin/templates/pe80/stopserv.tomcat.template 22 Aug 2005
>>> 05:24:33 -0000 1.3
>>> +++ admin/templates/pe80/stopserv.tomcat.template 20 Sep 2006
>>> 19:58:45 -0000
>>> @@ -9,7 +9,7 @@
>>>
>>> # Resolve links - $0 may be a softlink
>>> PRG="$0"
>>> -ASENV_CONF_LOCATION=%%%CONFIG_HOME%%%
>>> +ASENV_CONF_LOCATION="%%%CONFIG_HOME%%%"
>>>
>>> SERVER_NAME=%%%SERVER_NAME%%%
>>>
>>> @@ -30,7 +30,7 @@
>>> . "$ASENV_CONF_LOCATION/asenv.conf"
>>> fi
>>>
>>> -INSTANCE_ROOT=%%%INSTANCE_ROOT%%%
>>> +INSTANCE_ROOT="%%%INSTANCE_ROOT%%%"
>>>
>>> JAVA_HOME="$AS_JAVA"
>>> # Make sure prerequisite environment variables are set
>>>
>>>
>>>
>>> Index: admin-ee/templates/ee80/startserv.tomcat.template
>>> ===================================================================
>>> RCS file:
>>> /cvs/glassfish/admin-ee/templates/ee80/startserv.tomcat.template,v
>>> retrieving revision 1.1.1.1
>>> diff -u -r1.1.1.1 startserv.tomcat.template
>>> --- templates/ee80/startserv.tomcat.template 8 Aug 2006 19:48:40
>>> -0000 1.1.1.1
>>> +++ templates/ee80/startserv.tomcat.template 20 Sep 2006 19:13:12
>>> -0000
>>> @@ -8,7 +8,7 @@
>>>
>>> # Resolve links - $0 may be a softlink
>>> PRG="$0"
>>> -ASENV_CONF_LOCATION=%%%CONFIG_HOME%%%
>>> +ASENV_CONF_LOCATION="%%%CONFIG_HOME%%%"
>>>
>>> SERVER_NAME=%%%SERVER_NAME%%%
>>> DOMAIN_NAME=%%%DOMAIN_NAME%%%
>>> @@ -30,7 +30,7 @@
>>> . "$ASENV_CONF_LOCATION/asenv.conf"
>>> fi
>>>
>>> -INSTANCE_ROOT=%%%INSTANCE_ROOT%%%
>>> +INSTANCE_ROOT="%%%INSTANCE_ROOT%%%"
>>>
>>> JAVA_HOME="$AS_JAVA"
>>> # Make sure prerequisite environment variables are set
>>>
>>>
>>> Index: admin-ee/templates/ee80/stopserv.tomcat.template
>>> ===================================================================
>>> RCS file:
>>> /cvs/glassfish/admin-ee/templates/ee80/stopserv.tomcat.template,v
>>> retrieving revision 1.1.1.1
>>> diff -u -r1.1.1.1 stopserv.tomcat.template
>>> --- admin-ee/templates/ee80/stopserv.tomcat.template 8 Aug 2006
>>> 19:48:40 -0000 1.1.1.1
>>> +++ admin-ee/templates/ee80/stopserv.tomcat.template 20 Sep 2006
>>> 19:19:06 -0000
>>> @@ -8,7 +8,7 @@
>>>
>>> # Resolve links - $0 may be a softlink
>>> PRG="$0"
>>> -ASENV_CONF_LOCATION=%%%CONFIG_HOME%%%
>>> +ASENV_CONF_LOCATION="%%%CONFIG_HOME%%%"
>>>
>>> SERVER_NAME=%%%SERVER_NAME%%%
>>>
>>> @@ -29,7 +29,7 @@
>>> . "$ASENV_CONF_LOCATION/asenv.conf"
>>> fi
>>>
>>> -INSTANCE_ROOT=%%%INSTANCE_ROOT%%%
>>> +INSTANCE_ROOT="%%%INSTANCE_ROOT%%%"
>>>
>>> JAVA_HOME="$AS_JAVA"
>>> # Make sure prerequisite environment variables are set
>>>
>>>
>>> *Description:*
>>>
>>> KeystoreManager.java:
>>> Modified code to handle space in the directory path.
>>>
>>> startserv.tomcat.template, stopserv.tomcat.template:
>>> added quotes for ASENV_CONF_LOCATION and INSTANCE_ROOT.
>>>
>>> Thanks,
>>> Jane
>>>
>>>
>>>
>>>
>>>
>>>
>