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
> IIRC.
>
> 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.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
