You can push it to jdk9-dev directly. No other reviewer is required. Regards, Volker
On Tue, Apr 15, 2014 at 9:48 AM, Jonathan Lu <luc...@linux.vnet.ibm.com> wrote: > 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/ >>> >>> >>> 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 >> >> >> >
