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: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org