admin@glassfish.java.net

Re: code review for issue 694

From: Jane Young-Lau <Jane.Young_at_Sun.COM>
Date: Wed, 20 Sep 2006 14:59:52 -0700

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