tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <>
Subject Re: svn commit: r1239522 - in /tomcat/trunk/java/org/apache/catalina: core/ core/ core/ mbeans/ startup/
Date Fri, 03 Feb 2012 09:44:55 GMT
On 03/02/2012 01:28, Konstantin Kolinko wrote:
> Several comments regarding the changes in Tomcat.initBaseDir():
> 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".

Comment corrected to align with implementation.

> 2. Old implementation of initBaseDir() updated Tomcat#basedir field.
>> basedir = home.getCanonicalPath();
> That is no longer done. It could be
> basedir = baseFile.getPath();

Agreed. Fixed.

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

Both fixed.

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

Consistency and minimal breakage from the existing behaviour was what I
was looking for. There were so many places base and home were being
calculated I was beginning to lose track of where I was. Making both
unconditional seems the right way to go to me.

> I wonder whether there
> is a use case that requires absolute but non-canonical value here.

Not that I can think of.

> 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
> So I think it is OK to convert to canonical form unconditionally,
> though it is slight change in behaviour.

This series of patches is full of slight changes in behaviour - the main
reason I have no intention of back-porting it to 7.0.x. One more won't
hurt and the consistency is worth it.

Thanks for your review comments.


To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message