Hi Jonathan, thank you for fixing all the remaining issues. From my point of view this change looks good now.
@Masayoshi: can I please get a final approval from you for pushing the change? I also want to downport this to 8u-dev but I don't think that's a big deal as this only touches AX code. Thank you and best regards, Volker On Thu, Apr 10, 2014 at 11:44 AM, Jonathan Lu <luc...@linux.vnet.ibm.com>wrote: > Hi Volker, > > Thanks a lot for your comments, I've made another patch, > > http://cr.openjdk.java.net/~luchsh/JDK-8Masayoshi034220.v4/<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 >
