* It's too bad that the chmod method is declared as protected
rather than as nothing (package-private). If it was -- you could
just change the caller code (3 places in the package) to pass in
an array of args instead of doing all of that painful processing.
But, since it is protected there could be another caller of the
chmod anywhere in the other millions of lines of code, so forget
this idea! Moral for interested readers: don't use protected
unless it is necessary.
* Note that you may change the current behavior with the 2 tests for
null args -- in the previous version this condition would be
ignored. Perhaps you should change it to logging a warning to be
safe -- and adding this test as well:
if(!file.exists())
log a warning message here (throwing an Exception would break the
previous behavior which ignores this error)
Jane Young wrote:
> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>
>