<i18n dev> Improve timezone mapping for AIX platform
Volker Simonis
volker.simonis at gmail.com
Fri Apr 11 13:46:08 UTC 2014
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/~luchsh/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 i18n-dev
mailing list