Improve timezone mapping for AIX platform

Fri Apr 4 08:18:32 UTC 2014

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> <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> <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,
> seehttp://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> <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
>