tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: svn commit: r1239522 - in /tomcat/trunk/java/org/apache/catalina: Server.java core/ContainerBase.java core/StandardEngine.java core/StandardServer.java mbeans/MBeanFactory.java startup/Tomcat.java
Date Fri, 03 Feb 2012 01:28:30 GMT
2012/2/2  <markt@apache.org>:
> Author: markt
> Date: Thu Feb  2 10:32:34 2012
> New Revision: 1239522
>
> URL: http://svn.apache.org/viewvc?rev=1239522&view=rev
> Log:
> Extend clean-up to Server
>
> Modified:
>    tomcat/trunk/java/org/apache/catalina/Server.java
>    tomcat/trunk/java/org/apache/catalina/core/ContainerBase.java
>    tomcat/trunk/java/org/apache/catalina/core/StandardEngine.java
>    tomcat/trunk/java/org/apache/catalina/core/StandardServer.java
>    tomcat/trunk/java/org/apache/catalina/mbeans/MBeanFactory.java
>    tomcat/trunk/java/org/apache/catalina/startup/Tomcat.java
>

Several comments regarding the changes in Tomcat.initBaseDir():

0. Trivial note: one typo was already fixed in
http://svn.apache.org/viewvc?view=revision&revision=1239784

1. Tomcat#setBaseDir() Javadoc says that if system properties are
unset the value used is $HOME/tomcat.$PORT.

Though further it says "TODO: better default ? Maybe current dir ?"

The actual implementation of iniBaseDir() uses
System.getProperty("user.dir") which is $PWD. The home directory
property name would be "user.home".

2. Old implementation of initBaseDir() updated Tomcat#basedir field.
> basedir = home.getCanonicalPath();

That is no longer done. It could be
basedir = baseFile.getPath();
though honestly that field has no getters (but it has protected
visibility). It was String because the property that it was used to
set was String.


3. There are two further bugs in setting catalinaHome besides the one
fixed by r1239784:

1) s/setCatalinaBase()/setCatalinaHome()/

2) If catalina home value is null it should fallback to the value of
catalina base. In the old code it was:
> System.setProperty(Globals.CATALINA_HOME_PROP, basedir);


4. if (!homeFile.isAbsolute()) checks when setting base and home:

I wonder whether "isAbsolute()" check is needed. Though it matches
with what the old code was doing.

The Bootstrap class (as updated in r1239527) always converts both
paths to canonical form unconditionally.  The old code in
Catalina#initDirs() (removed in r1239527) did conversion
conditionally.


I think it is safer to convert it to canonical form here as well, and
that would be more consistent with bootstrap. I wonder whether there
is a use case that requires absolute but non-canonical value here.

Anyway we are already using canonical paths in many places internally.
If someone expects that he can change absolute->canonical mapping
while tomcat is running (e.g. change a symlink) he wouldn't go far
with that already.  SecurityManager also operates on canonical paths
IIRC.

So I think it is OK to convert to canonical form unconditionally,
though it is slight change in behaviour.


Best regards,
Konstantin Kolinko


> Modified: tomcat/trunk/java/org/apache/catalina/Server.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/Server.java?rev=1239522&r1=1239521&r2=1239522&view=diff
> > --- tomcat/trunk/java/org/apache/catalina/startup/Tomcat.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/startup/Tomcat.java Thu Feb  2 10:32:34 2012
> @@ -459,11 +459,12 @@ public class Tomcat {
>             return server;
>         }
>
> -        initBaseDir();
> -
>         System.setProperty("catalina.useNaming", "false");
>
>         server = new StandardServer();
> +
> +        initBaseDir();
> +
>         server.setPort( -1 );
>
>         service = new StandardService();
> @@ -591,20 +592,31 @@ public class Tomcat {
>             // Create a temp dir.
>             basedir = System.getProperty("user.dir") +
>                 "/tomcat." + port;
> -            File home = new File(basedir);
> -            home.mkdir();
> -            if (!home.isAbsolute()) {
> +        }
> +
> +        File baseFile = new File(basedir);
> +        baseFile.mkdirs();
> +        if (!baseFile.isAbsolute()) {
> +            try {
> +                baseFile = baseFile.getCanonicalFile();
> +            } catch (IOException e) {
> +                baseFile = baseFile.getAbsoluteFile();
> +            }
> +        }
> +        server.setCatalinaBase(baseFile);
> +
> +        if (catalinaHome == null) {
> +            File homeFile = new File(catalinaHome);
> +            homeFile.mkdirs();
> +            if (!homeFile.isAbsolute()) {
>                 try {
> -                    basedir = home.getCanonicalPath();
> +                    homeFile = homeFile.getCanonicalFile();
>                 } catch (IOException e) {
> -                    basedir = home.getAbsolutePath();
> +                    homeFile = homeFile.getAbsoluteFile();
>                 }
>             }
> +            server.setCatalinaBase(homeFile);
>         }
> -        if (catalinaHome == null) {
> -            System.setProperty(Globals.CATALINA_HOME_PROP, basedir);
> -        }
> -        System.setProperty(Globals.CATALINA_BASE_PROP, basedir);
>     }
>
>     static final String[] silences = new String[] {
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message