<i18n dev> Improve timezone mapping for AIX platform
Masayoshi Okutsu
masayoshi.okutsu at oracle.com
Mon Apr 14 06:03:17 UTC 2014
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 <mailto: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 <mailto: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 <mailto: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/
> <http://cr.openjdk.java.net/%7Eluchsh/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 <mailto: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 <mailto: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/
> <http://cr.openjdk.java.net/%7Eluchsh/JDK-8034220.v2/>
> >>
> >>
> >> On Thu, Mar 27, 2014 at 1:48 AM, Volker Simonis
> <volker.simonis at gmail.com <mailto: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
> <mailto: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/
> <http://cr.openjdk.java.net/%7Eluchsh/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