Hi Volker, Thanks a lot for your comments, I've made another patch,
http://cr.openjdk.java.net/~luchsh/JDK-8034220.v4/ On Fri, Apr 4, 2014 at 9:22 PM, Volker Simonis <volker.simo...@gmail.com>wrote: > Hi Jonathan, > > sorry, but I found a few more issues: > > - please use strncpy instead of strcpy in TimeZone_md.c:798 otherwise > somebody could easily crash the VM by specifying a TZ with more than > 100 characters. > Right, I've fix it by using strncpy, and also updated another usage of strcmp(). > > - you can delete the lines 871-872 because the variables are actually > not used and you can also remove the handling for "timezone == 0" > because that is actually done in getGMTOffsetID() anyway. > Nice catch, have deleted those in latest patch. > > - could you please avoid the usage of strtok. It is not intended for > usage in a multithreaded environment (see for example "man strtok" on > AIX). strtok_r is probably overkill, but you could use for example > strchr. > I've changed it to strtok_r in this patch, although strtok was only used once here, it may still impact other threads. > did you check the build on Windows? > Yes, both patches got built on Windows. > > Thank you and best regards, > Volker > > > On Fri, Apr 4, 2014 at 10:18 AM, Jonathan Lu <luc...@linux.vnet.ibm.com> > wrote: > > 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> > >> 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> > >> 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, > >> see > >> > >> > http://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> 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 > > > > > Regards Jonathan
