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
<luc...@linux.vnet.ibm.com <mailto:luc...@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.simo...@gmail.com <mailto:volker.simo...@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
<luc...@linux.vnet.ibm.com <mailto:luc...@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
<luc...@linux.vnet.ibm.com <mailto:luc...@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
<luc...@linux.vnet.ibm.com <mailto:luc...@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.simo...@gmail.com <mailto:volker.simo...@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.oku...@oracle.com
<mailto:masayoshi.oku...@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
