Hi Masayoshi, Volker, Thanks a lot for reviewing ! Is it OK for me to push the change into JDK9 directly ? or need another reviewer's approval ?
Many thanks Jonathan On Mon, Apr 14, 2014 at 2:03 PM, Masayoshi Okutsu < masayoshi.oku...@oracle.com> wrote: > Looks good to me. > > Thanks, > Masayoshi > > > On 4/11/2014 10:46 PM, Volker Simonis wrote: > > 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/%7Eluchsh/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 >> > > >
