Improve timezone mapping for AIX platform (original) (raw)

Jonathan Lu luchsh at linux.vnet.ibm.com
Wed Apr 2 16:34:02 UTC 2014

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.

  1. 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 := (JDKTOPDIR)/src/(JDK_TOPDIR)/src/(JDKT​OPDIR)/src/(OPENJDK_TARGET_OS)/lib

(LIBDIR)/tzmappings:(LIBDIR)/tzmappings: (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.

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

  1. I agree with Masayoshi that you should use the existing getGMTOffsetID()

Updated in new patch to eliminate duplication.

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

Could you pls help to review and give comments ?

Cheers

Jonathan

Regards Jonathan

More information about the core-libs-dev mailing list