Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v4]

[Wu Yan]

On Fri, 15 Oct 2021 07:03:18 GMT, Hamlin Li  wrote:

>> Wu Yan has updated the pull request with a new target base due to a merge or 
>> a rebase. The incremental webrev excludes the unrelated changes brought in 
>> by the merge/rebase. The pull request contains four additional commits since 
>> the last revision:
>> 
>>  - Merge branch 'master' into timezone
>>  - change functions to be static
>>  - replace realpath
>>  - 8273111: Default timezone should return zone ID if /etc/localtime is 
>> valid but not canonicalization on linux
>
> src/java.base/unix/native/libjava/TimeZone_md.c line 82:
> 
>> 80: 
>> 81: /*
>> 82:  * remove repeated path separators ('/') in the giving 'path'.
> 
> given?

Thanks, fix it.

> src/java.base/unix/native/libjava/TimeZone_md.c line 89:
> 
>> 87: char *left = path;
>> 88: char *right = path;
>> 89: char *end = path + strlen(path);
> 
> "char* end"? better to align with existing code style

OK, fixed it.

> src/java.base/unix/native/libjava/TimeZone_md.c line 95:
> 
>> 93: for (; right < end; right++) {
>> 94: if (*right == '/' && *(right + 1) == '/') break;
>> 95: }
> 
> Is this for loop necessary? Seems it's ok to merge with the nested loop below.

Thanks for your suggestion, this for loop is indeed unnecessary. Removed it.

-

PR: https://git.openjdk.java.net/jdk/pull/5327

Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v4]

[Hamlin Li]

On Thu, 14 Oct 2021 02:08:45 GMT, Wu Yan  wrote:

>> Hi,
>> Please help me review the change to enhance getting  time zone ID from 
>> /etc/localtime on linux.
>> 
>> We use `realpath` instead of `readlink` to obtain the link name of 
>> /etc/localtime, because `readlink` can only read the value of a symbolic of 
>> link, not the canonicalized absolute pathname.
>> 
>> For example, the value of /etc/localtime is 
>> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by 
>> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of 
>> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which 
>> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you 
>> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“.
>> 
>> Thanks,
>> wuyan
>
> Wu Yan has updated the pull request with a new target base due to a merge or 
> a rebase. The incremental webrev excludes the unrelated changes brought in by 
> the merge/rebase. The pull request contains four additional commits since the 
> last revision:
> 
>  - Merge branch 'master' into timezone
>  - change functions to be static
>  - replace realpath
>  - 8273111: Default timezone should return zone ID if /etc/localtime is valid 
> but not canonicalization on linux

src/java.base/unix/native/libjava/TimeZone_md.c line 82:

> 80: 
> 81: /*
> 82:  * remove repeated path separators ('/') in the giving 'path'.

given?

src/java.base/unix/native/libjava/TimeZone_md.c line 89:

> 87: char *left = path;
> 88: char *right = path;
> 89: char *end = path + strlen(path);

"char* end"? better to align with existing code style

src/java.base/unix/native/libjava/TimeZone_md.c line 95:

> 93: for (; right < end; right++) {
> 94: if (*right == '/' && *(right + 1) == '/') break;
> 95: }

Is this for loop necessary? Seems it's ok to merge with the nested loop below.

-

PR: https://git.openjdk.java.net/jdk/pull/5327

Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v4]

[Wu Yan]

> Hi,
> Please help me review the change to enhance getting  time zone ID from 
> /etc/localtime on linux.
> 
> We use `realpath` instead of `readlink` to obtain the link name of 
> /etc/localtime, because `readlink` can only read the value of a symbolic of 
> link, not the canonicalized absolute pathname.
> 
> For example, the value of /etc/localtime is 
> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by 
> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of 
> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which 
> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you 
> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“.
> 
> Thanks,
> wuyan

Wu Yan has updated the pull request with a new target base due to a merge or a 
rebase. The incremental webrev excludes the unrelated changes brought in by the 
merge/rebase. The pull request contains four additional commits since the last 
revision:

 - Merge branch 'master' into timezone
 - change functions to be static
 - replace realpath
 - 8273111: Default timezone should return zone ID if /etc/localtime is valid 
but not canonicalization on linux

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5327/files
  - new: https://git.openjdk.java.net/jdk/pull/5327/files/38177cd0..93cff3d1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5327=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5327=02-03

  Stats: 1449026 lines in 13982 files changed: 735454 ins; 659127 del; 54445 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5327.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327

PR: https://git.openjdk.java.net/jdk/pull/5327