<i18n dev> Improve timezone mapping for AIX platform
Jonathan Lu
luchsh at linux.vnet.ibm.com
Tue Apr 15 07:48:35 UTC 2014
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.okutsu at 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 <luchsh at 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.simonis at 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 <luchsh at 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 <
>>> luchsh at 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 <
>>> luchsh at 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.simonis at 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.okutsu at 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
>>
>
>
>
More information about the core-libs-dev
mailing list