<i18n dev> Improve timezone mapping for AIX platform
Volker Simonis
volker.simonis at gmail.com
Tue Apr 15 08:50:34 UTC 2014
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 <luchsh at 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.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/
>>>
>>>
>>> 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