Hi Volker, Masayoshi, I made another patch which fixed the problems mentioned in last mail,
http://cr.openjdk.java.net/~luchsh/JDK-8034220.v3/ Could you pls help to take a look? Many thanks Jonathan On Thu, Apr 3, 2014 at 12:34 AM, Jonathan Lu <luc...@linux.vnet.ibm.com>wrote: > Hi Volker, > > > On 2014年04月02日 23:07, Volker Simonis wrote: > > Hi Jonathan, > > thanks for updating the change. Please find my comments inline: > > On Tue, Apr 1, 2014 at 4:52 PM, Jonathan Lu <luc...@linux.vnet.ibm.com> > <luc...@linux.vnet.ibm.com> wrote: > > Hi Volker, Masayoshi, > > Thanks a lot for your review, here's the updated patch, could you pls take a > look? > http://cr.openjdk.java.net/~luchsh/JDK-8034220.v2/ > > > On Thu, Mar 27, 2014 at 1:48 AM, Volker Simonis <volker.simo...@gmail.com> > <volker.simo...@gmail.com> > wrote: > > Hi Jonathan, > > thanks for doing this change. Please find some comments below: > > 1. please update the copyright year to 2014 in every file you touch > > Updated in new patch. > > > 2. in CopyFiles.gmk you can simplify the change by joining the windows > and aix cases because on Windows OPENJDK_TARGET_OS is the same like > OPENJDK_TARGET_OS_API_DIR. So you can write: > > ifneq ($(findstring $(OPENJDK_TARGET_OS), windows aix), ) > > TZMAPPINGS_SRC := $(JDK_TOPDIR)/src/$(OPENJDK_TARGET_OS)/lib > > $(LIBDIR)/tzmappings: $(TZMAPPINGS_SRC)/tzmappings > $(call install-file) > > COPY_FILES += $(LIBDIR)/tzmappings > > endif > > I've tried that approach before, but OPENJDK_TARGET_OS_API_DIR is 'solaris' > as I observed on my AIX box, > solaris/lib is not the directory where tzmappings was stored for AIX. > So I'm keeping this change, please fix me if I was wrong about the config. > > > Yes, but my point was to actually use OPENJDK_TARGET_OS for the > construction of TZMAPPINGS_SRC as shown in the code snippet above. > OPENJDK_TARGET_OS is "windows" on Windows platforms and "aix" on AIX > and that should be just enough here. > > > That's right, let me try that and also build/test on Windows platform. > > > 3. regarding the changes proposed by Masayoshi: I agree that we should > put the AIX timezone mapping code in its own function, but I don't > think that getPlatformTimeZoneID() would be the right place. As far as > I understand, getPlatformTimeZoneID() is used to get a platform time > zone id if the environment variable "TZ" is not set. I don't know a > way how this could be done on AIX (@Jonathan: maybe you know?). If > there would be a way to get the time zone from some system files or > so, then we should put this code into the AIX version of > getPlatformTimeZoneID(). > > I guess we may try to use /etc/envrionment file, which is basic setting for > all processes, > seehttp://publib.boulder.ibm.com/infocenter/aix/v7r1/index.jsp?topic=%2Fcom.ibm.aix.files%2Fdoc%2Faixfiles%2Fenvironment.htm > The implementation of getPlatformTimeZoneID() has been updated. > > > That's good! > > > But the current AIX code contributed by Jonathan, actually uses the > time zone id from the "TZ" environment variable and just maps it to a > Java compatible time zone id. So I'd suggest to refactor this code > into a function like for example "static const char* > mapPlatformToJavaTimzone(const char* tz)" and call that from > findJavaTZ_md(). I think that would make the code easier to > understand. > > Agree, and have split the code into a separate static method to do the > mapping work. > > > Good. But there's still one error in findJavaTZ_md(): > > 706 #ifdef _AIX > 707 javatz = mapPlatformToJavaTimezone(java_home_dir, tz); > 708 #endif > > This should read: > > 706 #ifdef _AIX > 707 javatz = mapPlatformToJavaTimezone(java_home_dir, javatz); > 708 #endif > > because in line 703 you free 'tz' via its alias 'freetz' if 'tz' came > from getPlatformTimeZoneID(). > > > Right, but with the second approach, there may be a minor memory leak here, > as javatz was not freed before being overwritten on AIX. will post another > patch on this soon. > > > With the above fixed I'll push this one we get one more review from a > reviewer (I'm currently not an official reviewer). > > Regards, > Volker > > > > @Masayoshi: what do you think, would you agree with such a solution. > > 4. I agree with Masayoshi that you should use the existing > getGMTOffsetID() > > Updated in new patch to eliminate duplication. > > > 5. Please notice that your change breaks all Unix builds except AIX > because of: > > 759 } > 760 tzerr: > 761 if (javatz == NULL) { > 762 time_t offset; > ... > 782 } > 783 #endif > > I think that should read: > > 759 > 760 tzerr: > 761 if (javatz == NULL) { > 762 time_t offset; > ... > 782 } > 783 #endif > 784 } > > Refactoring the code in an extra function will make that error more > evident anyway. > > But please always build at least on one different platform (i.e. > Linux) to prevent such errors in the future. > > My fault, sorry for the failure, should take no chance after any manual > alteration. > > > Regards, > Volker > > > On Wed, Mar 26, 2014 at 10:27 AM, Masayoshi > Okutsu<masayoshi.oku...@oracle.com> <masayoshi.oku...@oracle.com> wrote: > > Hi Jonathan, > > The AIX specific code looks OK to me. But I'd suggest the code be moved > to > getPlatformTimeZoneID() for AIX, which just returns NULL currently. Also > there's a function for producing a fallback ID in "GMT±hh:mm", > getGMTOffsetID() which can be called in tzerr. > > Thanks, > Masayoshi > > > On 3/26/2014 3:47 PM, Jonathan Lu wrote: > > Hi ppc-aix-port-dev, core-libs-dev, > > Here's a patch for JDK-8034220, > http://cr.openjdk.java.net/~luchsh/JDK-8034220/ > > It is trying to add the a more complete timezone mapping mechanism for > AIX > platform. > the changes are primarily based on IBM's commercial JDK code, which > includes: > > - A new timezone mapping file added under directory jdk/src/aix/lib/ > - Code to parse above config file, changed file is > src/solaris/native/java/util/TimeZone_md.c > - And also makefile change in make/CopyFiles.gmk to copy the config > file > > Could you pls help to review and give comments ? > > Cheers > - Jonathan > > Many thanks > Jonathan > > Regards > Jonathan >