Apologize... another diff. I had an extra "+" in the regexp:
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
21 Sep 2006 03:47:23 -0000
@@ -45,6 +45,10 @@
import java.io.File;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+
/**
* @author kebbs
*/
@@ -427,8 +431,18 @@
protected void chmod(String args, File file) throws IOException
{
if (OS.isUNIX()) {
- Runtime.getRuntime().exec("/bin/chmod " + args + " " +
- file.getAbsolutePath());
+ //args and file should never be null.
+ //throw IOException since I don't want to change the
signature of this api.
+ if (args == null || file == null) throw new
IOException(_strMgr.getString("nullArg"));
+
+ // " +" regular expression for 1 or more spaces
+ final String[] argsString = args.split(" +");
+ List<String> cmdList = new ArrayList<String>();
+ cmdList.add("/bin/chmod");
+ for (String arg : argsString)
+ cmdList.add(arg);
+ cmdList.add(file.getAbsolutePath());
+ new ProcessBuilder(cmdList).start();
}
}
}
Jane Young wrote:
> Byron,
>
> I take that as you are okay with the changes to the template files.
>
> Here's the diff of KeystoreManager.java with your suggested change:
>
> Index:
> 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
> ---
> servermgmt/src/java/com/sun/enterprise/admin/servermgmt/KeystoreManager.java
> 25 Dec 2005 03:44:02 -0000 1.4
> +++
> servermgmt/src/java/com/sun/enterprise/admin/servermgmt/KeystoreManager.java
> 21 Sep 2006 01:42:38 -0000
> @@ -45,6 +45,10 @@
> import java.io.File;
> import java.io.IOException;
>
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +
> /**
> * @author kebbs
> */
> @@ -427,8 +431,18 @@
> protected void chmod(String args, File file) throws IOException
> {
> if (OS.isUNIX()) {
> - Runtime.getRuntime().exec("/bin/chmod " + args + " " +
> - file.getAbsolutePath());
> + //args and file should never be null.
> + //throw IOException since I don't want to change the
> signature of this api.
> + if (args == null || file == null) throw new
> IOException(_strMgr.getString("nullArg"));
> +
> + // " ++" regular expression for 1 or more spaces
> + final String[] argsString = args.split(" ++");
> + List<String> cmdList = new ArrayList<String>();
> + cmdList.add("/bin/chmod");
> + for (String arg : argsString)
> + cmdList.add(arg);
> + cmdList.add(file.getAbsolutePath());
> + new ProcessBuilder(cmdList).start();
> }
> }
> }
>
>
>
> Thanks for reviewing!
>
> Jane
>
>
>
> Byron Nevins wrote:
>
>> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>