Improve timezone mapping for AIX platform

[Jonathan Lu]

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.


>
> 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.


>
> 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.


>
> @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