Patch to expand tz checking scope in TimeZone_md.c

[David Holmes]

On 4/11/2011 2:53 PM, David Holmes wrote:
> On 4/11/2011 2:13 PM, Masayoshi Okutsu wrote:
>> Probably the difference isn't documented. I tried Solaris 10 and Ubuntu
>> 10.03. The difference still exists.
>>
>> Solaris 10:
>> $ unset TZ
>> $ date
>> Fri Nov 4 13:04:45 JST 2011
>> $ TZ="" date
>> Fri Nov 4 13:04:53 JST 2011
>>
>> Ubuntu 10.04:
>> $ unset TZ
>> $ date
>> Fri Nov 4 13:05:50 JST 2011
>> $ TZ="" date
>> Fri Nov 4 04:05:55 UTC 2011
>>
>> When the TZ value is an empty string, Ubuntu uses UTC while Solaris
>> still looks up the system default.
>
> I have to take back my comment regarding this not seeming to be platform
> specific code - it is highly platform specific! It seems that on Linux
> we are happy to report a TZ of "" but not so on Solaris. I presume this
> is an attempt to keep Java's use of TZ consistent with how other apps
> handle it on that platform. (environ(5) gives a little insight on
> Solaris as to how TZ is used.)
>
> So the key thing here is to not disturb the existing behaviour on either
> linux or Solaris - which suggests the original patch. That said I'm not
> convinced - given this is so platform specific - that simply treating
> non-linux the same as Solaris is a reasonable thing to do. I think it
> would be useful to see what the BSD/OSX port(s) had to do with this code
> - if anything.

To answer my own queries BSD/OSX does

       511 #if defined(__linux__) || defined(_ALLBSD_SOURCE)
       512     if (tz == NULL) {
       513 #else
       514 #ifdef __solaris__
       515     if (tz == NULL || *tz == '\0') {
       516 #endif
       517 #endif

so the suggested patch would at least not interfere.

Anyway this needs input from other core-libs folk. I didn't intend to 
get quite so heavily involved. ;-)

David
-----



> David
> -----
>
>
>> Thanks,
>> Masayoshi
>>
>> On 11/3/2011 4:16 PM, Jonathan Lu wrote:
>>> Hi Masayoshi,
>>>
>>> I did find some references about date-time related functions / TZ
>>> variables on Linux but got only a few about Solaris, so could not see
>>> any differences between those two platforms about the changes
>>> described in my patch. Have you got any links or references about
>>> these differences? I'm interested in it and may update the patch again
>>> after reading them.
>>>
>>> Thanks a lot!
>>> - Jonathan
>>>
>>> On 11/02/2011 10:27 PM, Masayoshi Okutsu wrote:
>>>> Hi Jonathan,
>>>>
>>>> IIRC, the difference came from some behavioral difference between the
>>>> Linux and Solaris libc date-time functions and/or the date command,
>>>> and TimeZone_md.c tries to follow the difference. But the code was
>>>> written looooong ago. The difference may no longer exist.
>>>>
>>>> Thanks,
>>>> Masayoshi
>>>>
>>>> On 11/2/2011 8:39 PM, Jonathan Lu wrote:
>>>>> On 11/02/2011 07:00 PM, David Holmes wrote:
>>>>>> On 2/11/2011 7:01 PM, Jonathan Lu wrote:
>>>>>>> On 11/02/2011 04:56 PM, Jonathan Lu wrote:
>>>>>>>> Hi core-libs-dev,
>>>>>>>>
>>>>>>>> In jdk/src/solaris/native/java/util/TimeZone_md.c, starting from
>>>>>>>> line
>>>>>>>> 626, I found that the scope of "#ifdef __solaris__" might be too
>>>>>>>> narrow, since it also works for some kind of OS which I'm currently
>>>>>>>> working on, such as AIX.
>>>>>>>> So I suggest to just remove the '#ifdef __solaris__' and leave the
>>>>>>>> "#else" to accommodate more conditions, see attachment
>>>>>>>> 'patch.diff'. I
>>>>>>>> think this may enhance the cross-platform ability, any ideas about
>>>>>>>> this modification?
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> - Jonathan Lu
>>>>>>> I'm not sure why the attachment got filtered, here paste it to the
>>>>>>> mail
>>>>>>> content directly.
>>>>>>>
>>>>>>> diff -r 4788745572ef src/solaris/native/java/util/TimeZone_md.c
>>>>>>> --- a/src/solaris/native/java/util/TimeZone_md.c Mon Oct 17 19:06:53
>>>>>>> 2011 -0700
>>>>>>> +++ b/src/solaris/native/java/util/TimeZone_md.c Thu Oct 20 13:43:47
>>>>>>> 2011 +0800
>>>>>>> @@ -626,10 +626,8 @@
>>>>>>> #ifdef __linux__
>>>>>>> if (tz == NULL) {
>>>>>>> #else
>>>>>>> -#ifdef __solaris__
>>>>>>> if (tz == NULL || *tz == '\0') {
>>>>>>> #endif
>>>>>>> -#endif
>>>>>>> tz = getPlatformTimeZoneID();
>>>>>>> freetz = tz;
>>>>>>> }
>>>>>>
>>>>>> I'm unclear why any of that code needs to be platform specific - is
>>>>>> an empty TZ string somehow valid on linux? I would have thought the
>>>>>> following would be platform neutral:
>>>>>>
>>>>>> if (tz == NULL || *tz == '\0') {
>>>>>> tz = getPlatformTimeZoneID();
>>>>>> freetz = tz;
>>>>>> }
>>>>>>
>>>>> Hi David,
>>>>>
>>>>> getenv("TZ") returns NULL when TZ environment variable is not set at
>>>>> all and returns '\0' when TZ was exported as empty string. After
>>>>> more checking for both cases, I agree with you, nothing useful can
>>>>> be retrieved from that environment variable.
>>>>> So I changed the patch to this,
>>>>>
>>>>> diff -r 7ab0d613cd1a src/solaris/native/java/util/TimeZone_md.c
>>>>> --- a/src/solaris/native/java/util/TimeZone_md.c Thu Oct 20 10:32:47
>>>>> 2011 -0700
>>>>> +++ b/src/solaris/native/java/util/TimeZone_md.c Wed Nov 02 19:34:51
>>>>> 2011 +0800
>>>>> @@ -623,13 +623,7 @@
>>>>>
>>>>> tz = getenv("TZ");
>>>>>
>>>>> -#ifdef __linux__
>>>>> - if (tz == NULL) {
>>>>> -#else
>>>>> -#ifdef __solaris__
>>>>> if (tz == NULL || *tz == '\0') {
>>>>> -#endif
>>>>> -#endif
>>>>> tz = getPlatformTimeZoneID();
>>>>> freetz = tz;
>>>>> }
>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> Regards
>>>>>>> - Jonathan Lu
>>>>> Regards
>>>>>
>>>>> - Jonathan
>>>>>
>>>