RE: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Pengfei Li]

Hi Bernard,

Yes, your fix is good but would be nicer if the comment in line 733 is modified 
as well since it might be misleading.
>  733 /* On AIX, readdir() returns EBADF ...

I only have Linux machines to test. But I suggest that your patch being merged 
soon as the deprecated use breaks the build on many latest Linux distributions, 
and it's hard for guys to find other POSIX platforms in a short time.

--
Thanks,
Pengfei


-Original Message-
From: B. Blaser  
Sent: Thursday, June 21, 2018 00:01
To: Pengfei Li 
Cc: kim.barr...@oracle.com; core-libs-...@openjdk.java.net; nd ; 
build-dev@openjdk.java.net
Subject: Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int 
readdir_r(DIR*, dirent*, dirent**)' is deprecated

Hi Pengfei,

On 20 June 2018 at 12:08, Pengfei Li  wrote:
> Hi
>
> I have tried the patch ( 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/052991.html ) 
> on Ubuntu 18.04 machines (x86_64 & aarch64) with glibc=2.26.1 and build is OK.
>
> There's a small issue within the following code in 
> src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
> Unlike readdir_r(), readdir() does not return int value. The value of res is 
> always 0 before #ifdef.
>
>  727 /* EINTR not listed as a possible error */
>  728 errno = 0;
>  729 ptr = readdir64(dirp);
>  730 res = errno;
>  731
>  732 #ifdef _AIX
>  733 /* On AIX, readdir() returns EBADF (i.e. '9') and sets 'result' to 
> NULL for the */
>  734 /* directory stream end. Otherwise, 'errno' will contain the error 
> code. */
>  735 if (res != 0) {
>  736 res = (ptr == NULL && res == EBADF) ? 0 : errno;
>  737 }
>  738 #endif
>
> May I know that if this core-libs change going to be merged recently, or more 
> platforms needs to be explored?
>
> --
> Thanks,
> Pengfei

Thanks for evaluating this patch, 'readdir()' doesn't return an 'int'
value but it sets 'errno' instead which is then assigned to 'res'.
So, I guess the fix is OK, or did I miss anything?

I've also tested it on Linux/x86_64 but ideally, the patch should be tested on 
all supported POSIX platforms before being integrated.
The JBS issue [1] doesn't mention any fix version, so I'm not sure if it'll be 
targeted to 11 or 12.

But if someone is available to test it on other POSIX platforms and review it, 
this would be nice?

Thanks,
Bernard

[1] https://bugs.openjdk.java.net/browse/JDK-8202794

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[B. Blaser]

Hi Pengfei,

On 20 June 2018 at 12:08, Pengfei Li  wrote:
> Hi
>
> I have tried the patch ( 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/052991.html ) 
> on Ubuntu 18.04 machines (x86_64 & aarch64) with glibc=2.26.1 and build is OK.
>
> There's a small issue within the following code in 
> src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
> Unlike readdir_r(), readdir() does not return int value. The value of res is 
> always 0 before #ifdef.
>
>  727 /* EINTR not listed as a possible error */
>  728 errno = 0;
>  729 ptr = readdir64(dirp);
>  730 res = errno;
>  731
>  732 #ifdef _AIX
>  733 /* On AIX, readdir() returns EBADF (i.e. '9') and sets 'result' to 
> NULL for the */
>  734 /* directory stream end. Otherwise, 'errno' will contain the error 
> code. */
>  735 if (res != 0) {
>  736 res = (ptr == NULL && res == EBADF) ? 0 : errno;
>  737 }
>  738 #endif
>
> May I know that if this core-libs change going to be merged recently, or more 
> platforms needs to be explored?
>
> --
> Thanks,
> Pengfei

Thanks for evaluating this patch, 'readdir()' doesn't return an 'int'
value but it sets 'errno' instead which is then assigned to 'res'.
So, I guess the fix is OK, or did I miss anything?

I've also tested it on Linux/x86_64 but ideally, the patch should be
tested on all supported POSIX platforms before being integrated.
The JBS issue [1] doesn't mention any fix version, so I'm not sure if
it'll be targeted to 11 or 12.

But if someone is available to test it on other POSIX platforms and
review it, this would be nice?

Thanks,
Bernard

[1] https://bugs.openjdk.java.net/browse/JDK-8202794

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On May 7, 2018, at 11:20 AM, B. Blaser  wrote:
> 
> On 7 May 2018 at 14:19, B. Blaser  wrote:
>> On 6 May 2018 at 18:35, B. Blaser  wrote:
>>> On 5 May 2018 at 22:26, Kim Barrett  wrote:
> On May 5, 2018, at 8:03 AM, B. Blaser  wrote:
> 
> On 4 May 2018 at 17:42, Alan Bateman  wrote:
>> On 04/05/2018 16:31, B. Blaser wrote:
>>> 
>>> :
>>> 
>>> I'll create a new JBS issue (unless you want to) since the fix is
>>> mostly located in core-libs and only intended to make the build
>>> complete.
>>> But I'll still need someone to review and push the fix, would you be
>>> interested in doing this?
>>> 
>> I think someone needs to explore the behavior on other platforms too as 
>> the
>> #ifndef __linux__ patches are ugly.
> 
> Yes sure but I cannot test it elsewhere myself... and we can easily
> remove them once POSIX officially deprecates 'readdir_r' or if someone
> validates the fix on other systems.
 
 I'm not sure anyone has the ability to test on all supported.  That
 doesn't (and really can't) prevent making changes across platforms to
 platform-specific code.  It just means one needs to get help with
 testing.  Make the change across platforms, test on those platforms
 one has access to, and ask here for help testing on others.
>>> 
>>> OK, I'll provide a cross-platform fix to be evaluated on other systems too.
>> 
>> Here it is, tier1 is successful on Linux with glibc=2.26.
>> Any feedback on other systems would be very helpful.
> 
> Some more cleanup…

I’ve created https://bugs.openjdk.java.net/browse/JDK-8202794 to cover this.
Suggest switching the subject line.

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[B. Blaser]

On 7 May 2018 at 14:19, B. Blaser  wrote:
> On 6 May 2018 at 18:35, B. Blaser  wrote:
>> On 5 May 2018 at 22:26, Kim Barrett  wrote:
 On May 5, 2018, at 8:03 AM, B. Blaser  wrote:

 On 4 May 2018 at 17:42, Alan Bateman  wrote:
> On 04/05/2018 16:31, B. Blaser wrote:
>>
>> :
>>
>> I'll create a new JBS issue (unless you want to) since the fix is
>> mostly located in core-libs and only intended to make the build
>> complete.
>> But I'll still need someone to review and push the fix, would you be
>> interested in doing this?
>>
> I think someone needs to explore the behavior on other platforms too as 
> the
> #ifndef __linux__ patches are ugly.

 Yes sure but I cannot test it elsewhere myself... and we can easily
 remove them once POSIX officially deprecates 'readdir_r' or if someone
 validates the fix on other systems.
>>>
>>> I'm not sure anyone has the ability to test on all supported.  That
>>> doesn't (and really can't) prevent making changes across platforms to
>>> platform-specific code.  It just means one needs to get help with
>>> testing.  Make the change across platforms, test on those platforms
>>> one has access to, and ask here for help testing on others.
>>
>> OK, I'll provide a cross-platform fix to be evaluated on other systems too.
>
> Here it is, tier1 is successful on Linux with glibc=2.26.
> Any feedback on other systems would be very helpful.

Some more cleanup...

Does this look better?

Thanks,
Bernard


diff -r e81481fea884 src/java.base/unix/native/libjava/TimeZone_md.c
--- a/src/java.base/unix/native/libjava/TimeZone_md.cMon May 07
08:56:35 2018 +0200
+++ b/src/java.base/unix/native/libjava/TimeZone_md.cMon May 07
15:14:26 2018 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -122,32 +122,18 @@
 DIR *dirp = NULL;
 struct stat statbuf;
 struct dirent64 *dp = NULL;
-struct dirent64 *entry = NULL;
 char *pathname = NULL;
 int fd = -1;
 char *dbuf = NULL;
 char *tz = NULL;
 int res;
-long name_max = 0;

 dirp = opendir(dir);
 if (dirp == NULL) {
 return NULL;
 }

-name_max = pathconf(dir, _PC_NAME_MAX);
-// If pathconf did not work, fall back to a mimimum buffer size.
-if (name_max < 1024) {
-name_max = 1024;
-}
-
-entry = (struct dirent64 *)malloc(offsetof(struct dirent64,
d_name) + name_max + 1);
-if (entry == NULL) {
-(void) closedir(dirp);
-return NULL;
-}
-
-while (readdir64_r(dirp, entry, ) == 0 && dp != NULL) {
+while ((dp = readdir64(dirp)) != NULL) {
 /*
  * Skip '.' and '..' (and possibly other .* files)
  */
@@ -214,9 +200,6 @@
 pathname = NULL;
 }

-if (entry != NULL) {
-free((void *) entry);
-}
 if (dirp != NULL) {
 (void) closedir(dirp);
 }
diff -r e81481fea884 src/java.base/unix/native/libjava/UnixFileSystem_md.c
--- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cMon May
07 08:56:35 2018 +0200
+++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cMon May
07 15:14:26 2018 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -312,7 +312,6 @@
 {
 DIR *dir = NULL;
 struct dirent64 *ptr;
-struct dirent64 *result;
 int len, maxlen;
 jobjectArray rv, old;
 jclass str_class;
@@ -325,13 +324,6 @@
 } END_PLATFORM_STRING(env, path);
 if (dir == NULL) return NULL;

-ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1));
-if (ptr == NULL) {
-JNU_ThrowOutOfMemoryError(env, "heap allocation failed");
-closedir(dir);
-return NULL;
-}
-
 /* Allocate an initial String array */
 len = 0;
 maxlen = 16;
@@ -339,7 +331,7 @@
 if (rv == NULL) goto error;

 /* Scan the directory */
-while ((readdir64_r(dir, ptr, ) == 0)  && (result != NULL)) {
+while ((ptr = readdir64(dir)) != NULL) {
 jstring name;
 if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, ".."))
 continue;
@@ -360,7 +352,6 @@
 (*env)->DeleteLocalRef(env, name);
 }
 closedir(dir);
-free(ptr);

 /* Copy the final results into an appropriately-sized array */
 old = rv;
@@ -375,7 +366,6 @@

  error:
 closedir(dir);
-free(ptr);
   

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[B. Blaser]

On 6 May 2018 at 18:35, B. Blaser  wrote:
> On 5 May 2018 at 22:26, Kim Barrett  wrote:
>>> On May 5, 2018, at 8:03 AM, B. Blaser  wrote:
>>>
>>> On 4 May 2018 at 17:42, Alan Bateman  wrote:
 On 04/05/2018 16:31, B. Blaser wrote:
>
> :
>
> I'll create a new JBS issue (unless you want to) since the fix is
> mostly located in core-libs and only intended to make the build
> complete.
> But I'll still need someone to review and push the fix, would you be
> interested in doing this?
>
 I think someone needs to explore the behavior on other platforms too as the
 #ifndef __linux__ patches are ugly.
>>>
>>> Yes sure but I cannot test it elsewhere myself... and we can easily
>>> remove them once POSIX officially deprecates 'readdir_r' or if someone
>>> validates the fix on other systems.
>>
>> I'm not sure anyone has the ability to test on all supported.  That
>> doesn't (and really can't) prevent making changes across platforms to
>> platform-specific code.  It just means one needs to get help with
>> testing.  Make the change across platforms, test on those platforms
>> one has access to, and ask here for help testing on others.
>
> OK, I'll provide a cross-platform fix to be evaluated on other systems too.

Here it is, tier1 is successful on Linux with glibc=2.26.
Any feedback on other systems would be very helpful.
Does this look better?

Thanks,
Bernard


diff -r e81481fea884 src/java.base/unix/native/libjava/TimeZone_md.c
--- a/src/java.base/unix/native/libjava/TimeZone_md.cMon May 07
08:56:35 2018 +0200
+++ b/src/java.base/unix/native/libjava/TimeZone_md.cMon May 07
12:38:42 2018 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -122,32 +122,18 @@
 DIR *dirp = NULL;
 struct stat statbuf;
 struct dirent64 *dp = NULL;
-struct dirent64 *entry = NULL;
 char *pathname = NULL;
 int fd = -1;
 char *dbuf = NULL;
 char *tz = NULL;
 int res;
-long name_max = 0;

 dirp = opendir(dir);
 if (dirp == NULL) {
 return NULL;
 }

-name_max = pathconf(dir, _PC_NAME_MAX);
-// If pathconf did not work, fall back to a mimimum buffer size.
-if (name_max < 1024) {
-name_max = 1024;
-}
-
-entry = (struct dirent64 *)malloc(offsetof(struct dirent64,
d_name) + name_max + 1);
-if (entry == NULL) {
-(void) closedir(dirp);
-return NULL;
-}
-
-while (readdir64_r(dirp, entry, ) == 0 && dp != NULL) {
+while ((dp = readdir64(dirp)) != NULL) {
 /*
  * Skip '.' and '..' (and possibly other .* files)
  */
@@ -214,9 +200,6 @@
 pathname = NULL;
 }

-if (entry != NULL) {
-free((void *) entry);
-}
 if (dirp != NULL) {
 (void) closedir(dirp);
 }
diff -r e81481fea884 src/java.base/unix/native/libjava/UnixFileSystem_md.c
--- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cMon May
07 08:56:35 2018 +0200
+++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cMon May
07 12:38:42 2018 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -312,7 +312,6 @@
 {
 DIR *dir = NULL;
 struct dirent64 *ptr;
-struct dirent64 *result;
 int len, maxlen;
 jobjectArray rv, old;
 jclass str_class;
@@ -325,13 +324,6 @@
 } END_PLATFORM_STRING(env, path);
 if (dir == NULL) return NULL;

-ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1));
-if (ptr == NULL) {
-JNU_ThrowOutOfMemoryError(env, "heap allocation failed");
-closedir(dir);
-return NULL;
-}
-
 /* Allocate an initial String array */
 len = 0;
 maxlen = 16;
@@ -339,7 +331,7 @@
 if (rv == NULL) goto error;

 /* Scan the directory */
-while ((readdir64_r(dir, ptr, ) == 0)  && (result != NULL)) {
+while ((ptr = readdir64(dir)) != NULL) {
 jstring name;
 if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, ".."))
 continue;
@@ -360,7 +352,6 @@
 (*env)->DeleteLocalRef(env, name);
 }
 closedir(dir);
-free(ptr);

 /* Copy the final results into an appropriately-sized array */
 old = rv;
@@ -375,7 +366,6 @@

  error:
 closedir(dir);
-free(ptr);
 return NULL;
 }

diff -r e81481fea884 src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
--- 

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[B. Blaser]

On 5 May 2018 at 22:26, Kim Barrett  wrote:
>> On May 5, 2018, at 8:03 AM, B. Blaser  wrote:
>>
>> On 4 May 2018 at 17:42, Alan Bateman  wrote:
>>> On 04/05/2018 16:31, B. Blaser wrote:

 :

 I'll create a new JBS issue (unless you want to) since the fix is
 mostly located in core-libs and only intended to make the build
 complete.
 But I'll still need someone to review and push the fix, would you be
 interested in doing this?

>>> I think someone needs to explore the behavior on other platforms too as the
>>> #ifndef __linux__ patches are ugly.
>>
>> Yes sure but I cannot test it elsewhere myself... and we can easily
>> remove them once POSIX officially deprecates 'readdir_r' or if someone
>> validates the fix on other systems.
>
> I'm not sure anyone has the ability to test on all supported.  That
> doesn't (and really can't) prevent making changes across platforms to
> platform-specific code.  It just means one needs to get help with
> testing.  Make the change across platforms, test on those platforms
> one has access to, and ask here for help testing on others.

OK, I'll provide a cross-platform fix to be evaluated on other systems too.

Thanks,
Bernard

> Waiting for a POSIX change is even more like watching paint dry than
> waiting for Java releases used to be.

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On May 5, 2018, at 8:03 AM, B. Blaser  wrote:
> 
> On 4 May 2018 at 17:42, Alan Bateman  wrote:
>> On 04/05/2018 16:31, B. Blaser wrote:
>>> 
>>> :
>>> 
>>> I'll create a new JBS issue (unless you want to) since the fix is
>>> mostly located in core-libs and only intended to make the build
>>> complete.
>>> But I'll still need someone to review and push the fix, would you be
>>> interested in doing this?
>>> 
>> I think someone needs to explore the behavior on other platforms too as the
>> #ifndef __linux__ patches are ugly.
> 
> Yes sure but I cannot test it elsewhere myself... and we can easily
> remove them once POSIX officially deprecates 'readdir_r' or if someone
> validates the fix on other systems.

I'm not sure anyone has the ability to test on all supported.  That
doesn't (and really can't) prevent making changes across platforms to
platform-specific code.  It just means one needs to get help with
testing.  Make the change across platforms, test on those platforms
one has access to, and ask here for help testing on others.

Waiting for a POSIX change is even more like watching paint dry than
waiting for Java releases used to be.

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[B. Blaser]

On 4 May 2018 at 17:42, Alan Bateman  wrote:
> On 04/05/2018 16:31, B. Blaser wrote:
>>
>> :
>>
>> I'll create a new JBS issue (unless you want to) since the fix is
>> mostly located in core-libs and only intended to make the build
>> complete.
>> But I'll still need someone to review and push the fix, would you be
>> interested in doing this?
>>
> I think someone needs to explore the behavior on other platforms too as the
> #ifndef __linux__ patches are ugly.

Yes sure but I cannot test it elsewhere myself... and we can easily
remove them once POSIX officially deprecates 'readdir_r' or if someone
validates the fix on other systems.

> Is there discussion on the original
> thread about this?

As Kim said, JDK-8202353 summarize the situation.

> -Alan

I've just made a small improvement (below) using 'errno' to preserve
the exact behavior when necessary, what do yo think?

Bernard

diff -r 1871c5d07caf src/java.base/unix/native/libjava/TimeZone_md.c
--- a/src/java.base/unix/native/libjava/TimeZone_md.cFri Apr 27
15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libjava/TimeZone_md.cSat May 05
12:54:56 2018 +0200
@@ -122,7 +122,9 @@
 DIR *dirp = NULL;
 struct stat statbuf;
 struct dirent64 *dp = NULL;
+#ifndef __linux__
 struct dirent64 *entry = NULL;
+#endif
 char *pathname = NULL;
 int fd = -1;
 char *dbuf = NULL;
@@ -140,7 +142,7 @@
 if (name_max < 1024) {
 name_max = 1024;
 }
-
+#ifndef __linux__
 entry = (struct dirent64 *)malloc(offsetof(struct dirent64,
d_name) + name_max + 1);
 if (entry == NULL) {
 (void) closedir(dirp);
@@ -148,6 +150,9 @@
 }

 while (readdir64_r(dirp, entry, ) == 0 && dp != NULL) {
+#else
+while ((dp = readdir64(dirp)) != NULL) {
+#endif
 /*
  * Skip '.' and '..' (and possibly other .* files)
  */
@@ -213,10 +218,11 @@
 free((void *) pathname);
 pathname = NULL;
 }
-
+#ifndef __linux__
 if (entry != NULL) {
 free((void *) entry);
 }
+#endif
 if (dirp != NULL) {
 (void) closedir(dirp);
 }
diff -r 1871c5d07caf src/java.base/unix/native/libjava/UnixFileSystem_md.c
--- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cFri Apr
27 15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cSat May
05 12:54:56 2018 +0200
@@ -312,7 +312,9 @@
 {
 DIR *dir = NULL;
 struct dirent64 *ptr;
+#ifndef __linux__
 struct dirent64 *result;
+#endif
 int len, maxlen;
 jobjectArray rv, old;
 jclass str_class;
@@ -324,14 +326,14 @@
 dir = opendir(path);
 } END_PLATFORM_STRING(env, path);
 if (dir == NULL) return NULL;
-
+#ifndef __linux__
 ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1));
 if (ptr == NULL) {
 JNU_ThrowOutOfMemoryError(env, "heap allocation failed");
 closedir(dir);
 return NULL;
 }
-
+#endif
 /* Allocate an initial String array */
 len = 0;
 maxlen = 16;
@@ -339,7 +341,11 @@
 if (rv == NULL) goto error;

 /* Scan the directory */
+#ifndef __linux__
 while ((readdir64_r(dir, ptr, ) == 0)  && (result != NULL)) {
+#else
+while ((ptr = readdir64(dir)) != NULL) {
+#endif
 jstring name;
 if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, ".."))
 continue;
@@ -360,7 +366,9 @@
 (*env)->DeleteLocalRef(env, name);
 }
 closedir(dir);
+#ifndef __linux__
 free(ptr);
+#endif

 /* Copy the final results into an appropriately-sized array */
 old = rv;
@@ -375,7 +383,9 @@

  error:
 closedir(dir);
+#ifndef __linux__
 free(ptr);
+#endif
 return NULL;
 }

diff -r 1871c5d07caf src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
--- a/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
Fri Apr 27 15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
Sat May 05 12:54:56 2018 +0200
@@ -726,12 +726,19 @@
 char name_extra[PATH_MAX + 1 - sizeof result->d_name];
 } entry;
 struct dirent64* ptr = 
+#ifndef __linux__
 int res;
+#endif
 DIR* dirp = jlong_to_ptr(value);

 /* EINTR not listed as a possible error */
 /* TDB: reentrant version probably not required here */
+#ifndef __linux__
 res = readdir64_r(dirp, ptr, );
+#else
+errno = 0;
+ptr = result = readdir64(dirp);
+#endif

 #ifdef _AIX
 /* On AIX, readdir_r() returns EBADF (i.e. '9') and sets 'result'
to NULL for the */
@@ -741,8 +748,13 @@
 }
 #endif

+#ifndef __linux__
 if (res != 0) {
 throwUnixException(env, res);
+#else
+if (errno != 0) {
+throwUnixException(env, errno);
+#endif
 return NULL;
 } else {
 if (result == NULL) {
diff -r 1871c5d07caf
src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c
--- a/src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c
   Fri Apr 27 15:55:29 2018 -0700

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On May 4, 2018, at 11:42 AM, Alan Bateman  wrote:
> 
> On 04/05/2018 16:31, B. Blaser wrote:
>> :
>> 
>> I'll create a new JBS issue (unless you want to) since the fix is
>> mostly located in core-libs and only intended to make the build
>> complete.
>> But I'll still need someone to review and push the fix, would you be
>> interested in doing this?
>> 
> I think someone needs to explore the behavior on other platforms too as the 
> #ifndef __linux__ patches are ugly. Is there discussion on the original 
> thread about this?
> 
> -Alan

Yes, see https://bugs.openjdk.java.net/browse/JDK-8202353

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Alan Bateman]

On 04/05/2018 16:31, B. Blaser wrote:

:

I'll create a new JBS issue (unless you want to) since the fix is
mostly located in core-libs and only intended to make the build
complete.
But I'll still need someone to review and push the fix, would you be
interested in doing this?

I think someone needs to explore the behavior on other platforms too as 
the #ifndef __linux__ patches are ugly. Is there discussion on the 
original thread about this?


-Alan

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[B. Blaser]

On 3 May 2018 at 21:43, Kim Barrett  wrote:
>> On May 3, 2018, at 1:52 PM, B. Blaser  wrote:
>>
>> Hi,
>>
>> On 2 May 2018 at 22:40, Kim Barrett  wrote:
 On May 2, 2018, at 5:10 AM, Michal Vala  wrote:



 On 05/01/2018 07:59 PM, Kim Barrett wrote:
>> On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:
>> Someone to sponsor this please?
> Do you have a sponsor yet?  If not, I’ll do it.

 No, I don't. I'd really appreciate if you sponsor it.
>>>
>>> Will do.  I should be able to take care of it in the next day or so.
>>>
>>
>> I've noted some similar issues at several other places which makes the
>> JDK build fail on Linux with glibc = 2.26, see comments here:
>> https://bugs.openjdk.java.net/browse/JDK-8202353
>>
>> Here is a patch for that, does this look reasonable (tier1 seems OK)?
>
> I think we should move in the direction of eliminating the use of readdir_r 
> entirely,
> either under JDK-8202353, or leave that as only the HotSpot part and have 
> another
> RFE for the uses in java.base/unix/native.

I'll create a new JBS issue (unless you want to) since the fix is
mostly located in core-libs and only intended to make the build
complete.
But I'll still need someone to review and push the fix, would you be
interested in doing this?

Thanks,
Bernard

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On May 3, 2018, at 1:52 PM, B. Blaser  wrote:
> 
> Hi,
> 
> On 2 May 2018 at 22:40, Kim Barrett  wrote:
>>> On May 2, 2018, at 5:10 AM, Michal Vala  wrote:
>>> 
>>> 
>>> 
>>> On 05/01/2018 07:59 PM, Kim Barrett wrote:
> On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:
> Someone to sponsor this please?
 Do you have a sponsor yet?  If not, I’ll do it.
>>> 
>>> No, I don't. I'd really appreciate if you sponsor it.
>> 
>> Will do.  I should be able to take care of it in the next day or so.
>> 
> 
> I've noted some similar issues at several other places which makes the
> JDK build fail on Linux with glibc = 2.26, see comments here:
> https://bugs.openjdk.java.net/browse/JDK-8202353
> 
> Here is a patch for that, does this look reasonable (tier1 seems OK)?

I think we should move in the direction of eliminating the use of readdir_r 
entirely,
either under JDK-8202353, or leave that as only the HotSpot part and have 
another
RFE for the uses in java.base/unix/native.

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[B. Blaser]

Hi,

On 2 May 2018 at 22:40, Kim Barrett  wrote:
>> On May 2, 2018, at 5:10 AM, Michal Vala  wrote:
>>
>>
>>
>> On 05/01/2018 07:59 PM, Kim Barrett wrote:
 On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:
 Someone to sponsor this please?
>>> Do you have a sponsor yet?  If not, I’ll do it.
>>
>> No, I don't. I'd really appreciate if you sponsor it.
>
> Will do.  I should be able to take care of it in the next day or so.
>

I've noted some similar issues at several other places which makes the
JDK build fail on Linux with glibc = 2.26, see comments here:
https://bugs.openjdk.java.net/browse/JDK-8202353

Here is a patch for that, does this look reasonable (tier1 seems OK)?

Thanks,
Bernard


diff -r 1871c5d07caf src/java.base/unix/native/libjava/TimeZone_md.c
--- a/src/java.base/unix/native/libjava/TimeZone_md.cFri Apr 27
15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libjava/TimeZone_md.cThu May 03
17:59:31 2018 +0200
@@ -122,7 +122,9 @@
 DIR *dirp = NULL;
 struct stat statbuf;
 struct dirent64 *dp = NULL;
+#ifndef __linux__
 struct dirent64 *entry = NULL;
+#endif
 char *pathname = NULL;
 int fd = -1;
 char *dbuf = NULL;
@@ -140,7 +142,7 @@
 if (name_max < 1024) {
 name_max = 1024;
 }
-
+#ifndef __linux__
 entry = (struct dirent64 *)malloc(offsetof(struct dirent64,
d_name) + name_max + 1);
 if (entry == NULL) {
 (void) closedir(dirp);
@@ -148,6 +150,9 @@
 }

 while (readdir64_r(dirp, entry, ) == 0 && dp != NULL) {
+#else
+while ((dp = readdir64(dirp)) != NULL) {
+#endif
 /*
  * Skip '.' and '..' (and possibly other .* files)
  */
@@ -213,10 +218,11 @@
 free((void *) pathname);
 pathname = NULL;
 }
-
+#ifndef __linux__
 if (entry != NULL) {
 free((void *) entry);
 }
+#endif
 if (dirp != NULL) {
 (void) closedir(dirp);
 }
diff -r 1871c5d07caf src/java.base/unix/native/libjava/UnixFileSystem_md.c
--- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cFri Apr
27 15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cThu May
03 17:59:31 2018 +0200
@@ -312,7 +312,9 @@
 {
 DIR *dir = NULL;
 struct dirent64 *ptr;
+#ifndef __linux__
 struct dirent64 *result;
+#endif
 int len, maxlen;
 jobjectArray rv, old;
 jclass str_class;
@@ -324,14 +326,14 @@
 dir = opendir(path);
 } END_PLATFORM_STRING(env, path);
 if (dir == NULL) return NULL;
-
+#ifndef __linux__
 ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1));
 if (ptr == NULL) {
 JNU_ThrowOutOfMemoryError(env, "heap allocation failed");
 closedir(dir);
 return NULL;
 }
-
+#endif
 /* Allocate an initial String array */
 len = 0;
 maxlen = 16;
@@ -339,7 +341,11 @@
 if (rv == NULL) goto error;

 /* Scan the directory */
+#ifndef __linux__
 while ((readdir64_r(dir, ptr, ) == 0)  && (result != NULL)) {
+#else
+while ((ptr = readdir64(dir)) != NULL) {
+#endif
 jstring name;
 if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, ".."))
 continue;
@@ -360,7 +366,9 @@
 (*env)->DeleteLocalRef(env, name);
 }
 closedir(dir);
+#ifndef __linux__
 free(ptr);
+#endif

 /* Copy the final results into an appropriately-sized array */
 old = rv;
@@ -375,7 +383,9 @@

  error:
 closedir(dir);
+#ifndef __linux__
 free(ptr);
+#endif
 return NULL;
 }

diff -r 1871c5d07caf src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
--- a/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
Fri Apr 27 15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
Thu May 03 17:59:31 2018 +0200
@@ -726,12 +726,18 @@
 char name_extra[PATH_MAX + 1 - sizeof result->d_name];
 } entry;
 struct dirent64* ptr = 
+#ifndef __linux__
 int res;
+#endif
 DIR* dirp = jlong_to_ptr(value);

 /* EINTR not listed as a possible error */
 /* TDB: reentrant version probably not required here */
+#ifndef __linux__
 res = readdir64_r(dirp, ptr, );
+#else
+ptr = result = readdir64(dirp);
+#endif

 #ifdef _AIX
 /* On AIX, readdir_r() returns EBADF (i.e. '9') and sets 'result'
to NULL for the */
@@ -741,10 +747,12 @@
 }
 #endif

+#ifndef __linux__
 if (res != 0) {
 throwUnixException(env, res);
 return NULL;
 } else {
+#endif
 if (result == NULL) {
 return NULL;
 } else {
@@ -755,7 +763,9 @@
 }
 return bytes;
 }
+#ifndef __linux__
 }
+#endif
 }

 JNIEXPORT void JNICALL
diff -r 1871c5d07caf
src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c
--- a/src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c
   Fri Apr 27 15:55:29 2018 -0700
+++ 

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On May 2, 2018, at 5:10 AM, Michal Vala  wrote:
> 
> 
> 
> On 05/01/2018 07:59 PM, Kim Barrett wrote:
>>> On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:
>>> Someone to sponsor this please?
>> Do you have a sponsor yet?  If not, I’ll do it.
> 
> No, I don't. I'd really appreciate if you sponsor it.

Will do.  I should be able to take care of it in the next day or so.

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Michal Vala]

On 05/01/2018 07:59 PM, Kim Barrett wrote:

On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:

For now, proposed patch looks like this:

--- old/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 
09:16:34.498343185 +0200
+++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 
09:16:34.214342670 +0200
@@ -98,26 +98,8 @@

inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
{
-// readdir_r has been deprecated since glibc 2.24.
-// See https://sourceware.org/bugzilla/show_bug.cgi?id=19056 for more details.
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
-
-  dirent* p;
-  int status;
   assert(dirp != NULL, "just checking");
-
-  // NOTE: Linux readdir_r (on RH 6.2 and 7.2 at least) is NOT like the POSIX
-  // version. Here is the doc for this function:
-  // http://www.gnu.org/manual/glibc-2.2.3/html_node/libc_262.html
-
-  if((status = ::readdir_r(dirp, dbuf, )) != 0) {
-errno = status;
-return NULL;
-  } else
-return p;
-
-#pragma GCC diagnostic pop
+  return ::readdir(dirp);
}

inline int os::closedir(DIR *dirp) {

This looks good.


Thanks!
Someone to sponsor this please?


Do you have a sponsor yet?  If not, I’ll do it.



No, I don't. I'd really appreciate if you sponsor it.

--
Michal Vala
OpenJDK QE
Red Hat Czech

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Andrew Hughes]

On 26 April 2018 at 23:55, Kim Barrett  wrote:

snip...

>
> I disagree, and still think the perfMemory_linux.cpp change should be
> removed.
>
> (1) The change to perfMemory_linux.cpp is entirely unnecessary to
> address the problem this bug is about.
>
> (2) It violates the (implied) protocol for os::readdir.  If
> Linux-specific code wants to invoke Linux-specific behavior, it should
> do so by calling a Linux-specific API, not abuse an intended to be
> portable API.
>
> (3) It minorly interferes with some desirable future work.  If there
> were some significant benefit to doing so, I wouldn't give this much
> weight, but I don't see a significant benefit.
>
> (4) The only benefit is saving some rare short-term memory
> allocations.  I don't think that's worth the above costs.
>
> Note that the Windows version of os::readdir also ignores the second
> argument, but all callers provide it anyway.
>
> I've opened a new CR for general os::readdir cleanup.
> https://bugs.openjdk.java.net/browse/JDK-8202353
>

Ok, I see your points and don't feel particularly strongly either way.
The original version of this patch I wrote didn't include those changes either.
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:
>>> For now, proposed patch looks like this:
>>> 
>>> --- old/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 
>>> 09:16:34.498343185 +0200
>>> +++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 
>>> 09:16:34.214342670 +0200
>>> @@ -98,26 +98,8 @@
>>> 
>>> inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
>>> {
>>> -// readdir_r has been deprecated since glibc 2.24.
>>> -// See https://sourceware.org/bugzilla/show_bug.cgi?id=19056 for more 
>>> details.
>>> -#pragma GCC diagnostic push
>>> -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>>> -
>>> -  dirent* p;
>>> -  int status;
>>>   assert(dirp != NULL, "just checking");
>>> -
>>> -  // NOTE: Linux readdir_r (on RH 6.2 and 7.2 at least) is NOT like the 
>>> POSIX
>>> -  // version. Here is the doc for this function:
>>> -  // http://www.gnu.org/manual/glibc-2.2.3/html_node/libc_262.html
>>> -
>>> -  if((status = ::readdir_r(dirp, dbuf, )) != 0) {
>>> -errno = status;
>>> -return NULL;
>>> -  } else
>>> -return p;
>>> -
>>> -#pragma GCC diagnostic pop
>>> +  return ::readdir(dirp);
>>> }
>>> 
>>> inline int os::closedir(DIR *dirp) {
>> This looks good.
> 
> Thanks!
> Someone to sponsor this please?

Do you have a sponsor yet?  If not, I’ll do it.

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Michal Vala]

On 04/27/2018 06:50 PM, Kim Barrett wrote:

On Apr 27, 2018, at 4:56 AM, Michal Vala  wrote:



On 04/27/2018 12:55 AM, Kim Barrett wrote:

On Apr 25, 2018, at 10:51 AM, Andrew Hughes  wrote:

On 24 April 2018 at 20:17, Kim Barrett  wrote:

On Apr 23, 2018, at 3:51 AM, Michal Vala  wrote:

Hi,

following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm posting 
updated patch replacing deprecated readdir_r with readdir. Bug ID is JDK-8179887 [2].

webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/

I'm asking for the review and eventually sponsorship.




The patch looks good to me.


The change to os::readdir in os_linux.inline.hpp looks fine.

I was going to suggest that rather than changing perfMemory_linux.cpp to use 
os::readdir in an
unusual and platform-specific way, it should instead just call ::readdir 
directly.  However, neither
of those is right, and that part of the change should not be made; see
https://bugs.openjdk.java.net/browse/JDK-8134540
Much nearly duplicated code for PerfMemory support



I think, in the circumstances, Michal's solution is the best option at
this point.
8134540 looks like a more long-term change currently scheduled for 12 (is
anyone currently working on it?), whereas this fix could go into 11 and remove
a couple of unneeded memory allocations.


Looking a bit deeper, there might be some additional follow-on that could be 
done, eliminating
the second argument to os::readdir.
windows: unused
bsd: freebsd also deprecated readdir_r, I think for similar reasons.
solaris: doc is clear about thread safety issue being about sharing the DIR*
aix: I haven’t looked at it, but would guess it’s similar.

In other words, don’t operate on the DIR* from multiple threads simultaneously, 
and just use
::readdir on non-Windows.  That would all be for another RFE though.




This also seems like a more in-depth separate change and not one that
can be performed
by any of us alone, as we don't test all these platforms. As it
stands, Michal's change affects
Linux and Linux alone, and the addition of the use of NULL will make
it clearer that moving
to an os:readdir is feasible on that platform.

I disagree, and still think the perfMemory_linux.cpp change should be
removed.
(1) The change to perfMemory_linux.cpp is entirely unnecessary to
address the problem this bug is about.
(2) It violates the (implied) protocol for os::readdir.  If
Linux-specific code wants to invoke Linux-specific behavior, it should
do so by calling a Linux-specific API, not abuse an intended to be
portable API.
(3) It minorly interferes with some desirable future work.  If there
were some significant benefit to doing so, I wouldn't give this much
weight, but I don't see a significant benefit.
(4) The only benefit is saving some rare short-term memory
allocations.  I don't think that's worth the above costs.
Note that the Windows version of os::readdir also ignores the second
argument, but all callers provide it anyway.
I've opened a new CR for general os::readdir cleanup.
https://bugs.openjdk.java.net/browse/JDK-8202353


Ok, if we agree on os_linux.inline.hpp part, I think it should go in. Rest can 
be solved in JDK-8202353, which should imho include also removal of second 
argument of os::readdir, once it's unused.

For now, proposed patch looks like this:

--- old/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 
09:16:34.498343185 +0200
+++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 
09:16:34.214342670 +0200
@@ -98,26 +98,8 @@

inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
{
-// readdir_r has been deprecated since glibc 2.24.
-// See https://sourceware.org/bugzilla/show_bug.cgi?id=19056 for more details.
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
-
-  dirent* p;
-  int status;
   assert(dirp != NULL, "just checking");
-
-  // NOTE: Linux readdir_r (on RH 6.2 and 7.2 at least) is NOT like the POSIX
-  // version. Here is the doc for this function:
-  // http://www.gnu.org/manual/glibc-2.2.3/html_node/libc_262.html
-
-  if((status = ::readdir_r(dirp, dbuf, )) != 0) {
-errno = status;
-return NULL;
-  } else
-return p;
-
-#pragma GCC diagnostic pop
+  return ::readdir(dirp);
}

inline int os::closedir(DIR *dirp) {


This looks good.



Thanks!
Someone to sponsor this please?

--
Michal Vala
OpenJDK QE
Red Hat Czech

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On Apr 27, 2018, at 4:56 AM, Michal Vala  wrote:
> 
> 
> 
> On 04/27/2018 12:55 AM, Kim Barrett wrote:
>>> On Apr 25, 2018, at 10:51 AM, Andrew Hughes  wrote:
>>> 
>>> On 24 April 2018 at 20:17, Kim Barrett  wrote:
> On Apr 23, 2018, at 3:51 AM, Michal Vala  wrote:
> 
> Hi,
> 
> following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm 
> posting updated patch replacing deprecated readdir_r with readdir. Bug ID 
> is JDK-8179887 [2].
> 
> webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/
> 
> I'm asking for the review and eventually sponsorship.
 
>>> 
>>> The patch looks good to me.
>>> 
 The change to os::readdir in os_linux.inline.hpp looks fine.
 
 I was going to suggest that rather than changing perfMemory_linux.cpp to 
 use os::readdir in an
 unusual and platform-specific way, it should instead just call ::readdir 
 directly.  However, neither
 of those is right, and that part of the change should not be made; see
 https://bugs.openjdk.java.net/browse/JDK-8134540
 Much nearly duplicated code for PerfMemory support
 
>>> 
>>> I think, in the circumstances, Michal's solution is the best option at
>>> this point.
>>> 8134540 looks like a more long-term change currently scheduled for 12 (is
>>> anyone currently working on it?), whereas this fix could go into 11 and 
>>> remove
>>> a couple of unneeded memory allocations.
>>> 
 Looking a bit deeper, there might be some additional follow-on that could 
 be done, eliminating
 the second argument to os::readdir.
 windows: unused
 bsd: freebsd also deprecated readdir_r, I think for similar reasons.
 solaris: doc is clear about thread safety issue being about sharing the 
 DIR*
 aix: I haven’t looked at it, but would guess it’s similar.
 
 In other words, don’t operate on the DIR* from multiple threads 
 simultaneously, and just use
 ::readdir on non-Windows.  That would all be for another RFE though.
 
 
>>> 
>>> This also seems like a more in-depth separate change and not one that
>>> can be performed
>>> by any of us alone, as we don't test all these platforms. As it
>>> stands, Michal's change affects
>>> Linux and Linux alone, and the addition of the use of NULL will make
>>> it clearer that moving
>>> to an os:readdir is feasible on that platform.
>> I disagree, and still think the perfMemory_linux.cpp change should be
>> removed.
>> (1) The change to perfMemory_linux.cpp is entirely unnecessary to
>> address the problem this bug is about.
>> (2) It violates the (implied) protocol for os::readdir.  If
>> Linux-specific code wants to invoke Linux-specific behavior, it should
>> do so by calling a Linux-specific API, not abuse an intended to be
>> portable API.
>> (3) It minorly interferes with some desirable future work.  If there
>> were some significant benefit to doing so, I wouldn't give this much
>> weight, but I don't see a significant benefit.
>> (4) The only benefit is saving some rare short-term memory
>> allocations.  I don't think that's worth the above costs.
>> Note that the Windows version of os::readdir also ignores the second
>> argument, but all callers provide it anyway.
>> I've opened a new CR for general os::readdir cleanup.
>> https://bugs.openjdk.java.net/browse/JDK-8202353
> 
> Ok, if we agree on os_linux.inline.hpp part, I think it should go in. Rest 
> can be solved in JDK-8202353, which should imho include also removal of 
> second argument of os::readdir, once it's unused.
> 
> For now, proposed patch looks like this:
> 
> --- old/src/hotspot/os/linux/os_linux.inline.hpp  2018-04-20 
> 09:16:34.498343185 +0200
> +++ new/src/hotspot/os/linux/os_linux.inline.hpp  2018-04-20 
> 09:16:34.214342670 +0200
> @@ -98,26 +98,8 @@
> 
> inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
> {
> -// readdir_r has been deprecated since glibc 2.24.
> -// See https://sourceware.org/bugzilla/show_bug.cgi?id=19056 for more 
> details.
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> -
> -  dirent* p;
> -  int status;
>   assert(dirp != NULL, "just checking");
> -
> -  // NOTE: Linux readdir_r (on RH 6.2 and 7.2 at least) is NOT like the POSIX
> -  // version. Here is the doc for this function:
> -  // http://www.gnu.org/manual/glibc-2.2.3/html_node/libc_262.html
> -
> -  if((status = ::readdir_r(dirp, dbuf, )) != 0) {
> -errno = status;
> -return NULL;
> -  } else
> -return p;
> -
> -#pragma GCC diagnostic pop
> +  return ::readdir(dirp);
> }
> 
> inline int os::closedir(DIR *dirp) {

This looks good.

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Thomas Stüfe]

This patch looks fine.

I also welcome Kim's proposal of reverting back to readdir(). We
should do this for JDK libraries too, there are still open issues
(e.g. https://bugs.openjdk.java.net/browse/JDK-8166372).

Best Regards, Thomas

On Fri, Apr 27, 2018 at 10:56 AM, Michal Vala  wrote:
>
>
> On 04/27/2018 12:55 AM, Kim Barrett wrote:
>>>
>>> On Apr 25, 2018, at 10:51 AM, Andrew Hughes 
>>> wrote:
>>>
>>> On 24 April 2018 at 20:17, Kim Barrett  wrote:
>
> On Apr 23, 2018, at 3:51 AM, Michal Vala  wrote:
>
> Hi,
>
> following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm
> posting updated patch replacing deprecated readdir_r with readdir. Bug ID 
> is
> JDK-8179887 [2].
>
> webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/
>
> I'm asking for the review and eventually sponsorship.


>>>
>>> The patch looks good to me.
>>>
 The change to os::readdir in os_linux.inline.hpp looks fine.

 I was going to suggest that rather than changing perfMemory_linux.cpp to
 use os::readdir in an
 unusual and platform-specific way, it should instead just call ::readdir
 directly.  However, neither
 of those is right, and that part of the change should not be made; see
 https://bugs.openjdk.java.net/browse/JDK-8134540
 Much nearly duplicated code for PerfMemory support

>>>
>>> I think, in the circumstances, Michal's solution is the best option at
>>> this point.
>>> 8134540 looks like a more long-term change currently scheduled for 12 (is
>>> anyone currently working on it?), whereas this fix could go into 11 and
>>> remove
>>> a couple of unneeded memory allocations.
>>>
 Looking a bit deeper, there might be some additional follow-on that
 could be done, eliminating
 the second argument to os::readdir.
 windows: unused
 bsd: freebsd also deprecated readdir_r, I think for similar reasons.
 solaris: doc is clear about thread safety issue being about sharing the
 DIR*
 aix: I haven’t looked at it, but would guess it’s similar.

 In other words, don’t operate on the DIR* from multiple threads
 simultaneously, and just use
 ::readdir on non-Windows.  That would all be for another RFE though.


>>>
>>> This also seems like a more in-depth separate change and not one that
>>> can be performed
>>> by any of us alone, as we don't test all these platforms. As it
>>> stands, Michal's change affects
>>> Linux and Linux alone, and the addition of the use of NULL will make
>>> it clearer that moving
>>> to an os:readdir is feasible on that platform.
>>
>>
>> I disagree, and still think the perfMemory_linux.cpp change should be
>> removed.
>>
>> (1) The change to perfMemory_linux.cpp is entirely unnecessary to
>> address the problem this bug is about.
>>
>> (2) It violates the (implied) protocol for os::readdir.  If
>> Linux-specific code wants to invoke Linux-specific behavior, it should
>> do so by calling a Linux-specific API, not abuse an intended to be
>> portable API.
>>
>> (3) It minorly interferes with some desirable future work.  If there
>> were some significant benefit to doing so, I wouldn't give this much
>> weight, but I don't see a significant benefit.
>>
>> (4) The only benefit is saving some rare short-term memory
>> allocations.  I don't think that's worth the above costs.
>>
>> Note that the Windows version of os::readdir also ignores the second
>> argument, but all callers provide it anyway.
>>
>> I've opened a new CR for general os::readdir cleanup.
>> https://bugs.openjdk.java.net/browse/JDK-8202353
>>
>
> Ok, if we agree on os_linux.inline.hpp part, I think it should go in. Rest
> can be solved in JDK-8202353, which should imho include also removal of
> second argument of os::readdir, once it's unused.
>
> For now, proposed patch looks like this:
>
> --- old/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20
> 09:16:34.498343185 +0200
> +++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20
> 09:16:34.214342670 +0200
> @@ -98,26 +98,8 @@
>
>  inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
>  {
> -// readdir_r has been deprecated since glibc 2.24.
> -// See https://sourceware.org/bugzilla/show_bug.cgi?id=19056 for more
> details.
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> -
> -  dirent* p;
> -  int status;
>assert(dirp != NULL, "just checking");
> -
> -  // NOTE: Linux readdir_r (on RH 6.2 and 7.2 at least) is NOT like the
> POSIX
> -  // version. Here is the doc for this function:
> -  // http://www.gnu.org/manual/glibc-2.2.3/html_node/libc_262.html
> -
> -  if((status = ::readdir_r(dirp, dbuf, )) != 0) {
> -errno = status;
> -return NULL;
> -  } else
> -return p;
> -
> -#pragma GCC diagnostic pop
> +  return ::readdir(dirp);
>  }
>
>  inline int 

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Michal Vala]

On 04/27/2018 12:55 AM, Kim Barrett wrote:

On Apr 25, 2018, at 10:51 AM, Andrew Hughes  wrote:

On 24 April 2018 at 20:17, Kim Barrett  wrote:

On Apr 23, 2018, at 3:51 AM, Michal Vala  wrote:

Hi,

following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm posting 
updated patch replacing deprecated readdir_r with readdir. Bug ID is JDK-8179887 [2].

webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/

I'm asking for the review and eventually sponsorship.




The patch looks good to me.


The change to os::readdir in os_linux.inline.hpp looks fine.

I was going to suggest that rather than changing perfMemory_linux.cpp to use 
os::readdir in an
unusual and platform-specific way, it should instead just call ::readdir 
directly.  However, neither
of those is right, and that part of the change should not be made; see
https://bugs.openjdk.java.net/browse/JDK-8134540
Much nearly duplicated code for PerfMemory support



I think, in the circumstances, Michal's solution is the best option at
this point.
8134540 looks like a more long-term change currently scheduled for 12 (is
anyone currently working on it?), whereas this fix could go into 11 and remove
a couple of unneeded memory allocations.


Looking a bit deeper, there might be some additional follow-on that could be 
done, eliminating
the second argument to os::readdir.
windows: unused
bsd: freebsd also deprecated readdir_r, I think for similar reasons.
solaris: doc is clear about thread safety issue being about sharing the DIR*
aix: I haven’t looked at it, but would guess it’s similar.

In other words, don’t operate on the DIR* from multiple threads simultaneously, 
and just use
::readdir on non-Windows.  That would all be for another RFE though.




This also seems like a more in-depth separate change and not one that
can be performed
by any of us alone, as we don't test all these platforms. As it
stands, Michal's change affects
Linux and Linux alone, and the addition of the use of NULL will make
it clearer that moving
to an os:readdir is feasible on that platform.


I disagree, and still think the perfMemory_linux.cpp change should be
removed.

(1) The change to perfMemory_linux.cpp is entirely unnecessary to
address the problem this bug is about.

(2) It violates the (implied) protocol for os::readdir.  If
Linux-specific code wants to invoke Linux-specific behavior, it should
do so by calling a Linux-specific API, not abuse an intended to be
portable API.

(3) It minorly interferes with some desirable future work.  If there
were some significant benefit to doing so, I wouldn't give this much
weight, but I don't see a significant benefit.

(4) The only benefit is saving some rare short-term memory
allocations.  I don't think that's worth the above costs.

Note that the Windows version of os::readdir also ignores the second
argument, but all callers provide it anyway.

I've opened a new CR for general os::readdir cleanup.
https://bugs.openjdk.java.net/browse/JDK-8202353



Ok, if we agree on os_linux.inline.hpp part, I think it should go in. Rest can 
be solved in JDK-8202353, which should imho include also removal of second 
argument of os::readdir, once it's unused.


For now, proposed patch looks like this:

--- old/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 
09:16:34.498343185 +0200
+++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 
09:16:34.214342670 +0200
@@ -98,26 +98,8 @@

 inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
 {
-// readdir_r has been deprecated since glibc 2.24.
-// See https://sourceware.org/bugzilla/show_bug.cgi?id=19056 for more details.
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
-
-  dirent* p;
-  int status;
   assert(dirp != NULL, "just checking");
-
-  // NOTE: Linux readdir_r (on RH 6.2 and 7.2 at least) is NOT like the POSIX
-  // version. Here is the doc for this function:
-  // http://www.gnu.org/manual/glibc-2.2.3/html_node/libc_262.html
-
-  if((status = ::readdir_r(dirp, dbuf, )) != 0) {
-errno = status;
-return NULL;
-  } else
-return p;
-
-#pragma GCC diagnostic pop
+  return ::readdir(dirp);
 }

 inline int os::closedir(DIR *dirp) {



--
Michal Vala
OpenJDK QE
Red Hat Czech

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On Apr 25, 2018, at 10:51 AM, Andrew Hughes  wrote:
> 
> On 24 April 2018 at 20:17, Kim Barrett  wrote:
>>> On Apr 23, 2018, at 3:51 AM, Michal Vala  wrote:
>>> 
>>> Hi,
>>> 
>>> following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm 
>>> posting updated patch replacing deprecated readdir_r with readdir. Bug ID 
>>> is JDK-8179887 [2].
>>> 
>>> webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/
>>> 
>>> I'm asking for the review and eventually sponsorship.
>> 
> 
> The patch looks good to me.
> 
>> The change to os::readdir in os_linux.inline.hpp looks fine.
>> 
>> I was going to suggest that rather than changing perfMemory_linux.cpp to use 
>> os::readdir in an
>> unusual and platform-specific way, it should instead just call ::readdir 
>> directly.  However, neither
>> of those is right, and that part of the change should not be made; see
>> https://bugs.openjdk.java.net/browse/JDK-8134540
>> Much nearly duplicated code for PerfMemory support
>> 
> 
> I think, in the circumstances, Michal's solution is the best option at
> this point.
> 8134540 looks like a more long-term change currently scheduled for 12 (is
> anyone currently working on it?), whereas this fix could go into 11 and remove
> a couple of unneeded memory allocations.
> 
>> Looking a bit deeper, there might be some additional follow-on that could be 
>> done, eliminating
>> the second argument to os::readdir.
>> windows: unused
>> bsd: freebsd also deprecated readdir_r, I think for similar reasons.
>> solaris: doc is clear about thread safety issue being about sharing the DIR*
>> aix: I haven’t looked at it, but would guess it’s similar.
>> 
>> In other words, don’t operate on the DIR* from multiple threads 
>> simultaneously, and just use
>> ::readdir on non-Windows.  That would all be for another RFE though.
>> 
>> 
> 
> This also seems like a more in-depth separate change and not one that
> can be performed
> by any of us alone, as we don't test all these platforms. As it
> stands, Michal's change affects
> Linux and Linux alone, and the addition of the use of NULL will make
> it clearer that moving
> to an os:readdir is feasible on that platform.

I disagree, and still think the perfMemory_linux.cpp change should be
removed.

(1) The change to perfMemory_linux.cpp is entirely unnecessary to
address the problem this bug is about.

(2) It violates the (implied) protocol for os::readdir.  If
Linux-specific code wants to invoke Linux-specific behavior, it should
do so by calling a Linux-specific API, not abuse an intended to be
portable API.

(3) It minorly interferes with some desirable future work.  If there
were some significant benefit to doing so, I wouldn't give this much
weight, but I don't see a significant benefit.

(4) The only benefit is saving some rare short-term memory
allocations.  I don't think that's worth the above costs.

Note that the Windows version of os::readdir also ignores the second
argument, but all callers provide it anyway.

I've opened a new CR for general os::readdir cleanup.
https://bugs.openjdk.java.net/browse/JDK-8202353

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Andrew Hughes]

On 24 April 2018 at 20:17, Kim Barrett  wrote:
>> On Apr 23, 2018, at 3:51 AM, Michal Vala  wrote:
>>
>> Hi,
>>
>> following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm 
>> posting updated patch replacing deprecated readdir_r with readdir. Bug ID is 
>> JDK-8179887 [2].
>>
>> webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/
>>
>> I'm asking for the review and eventually sponsorship.
>

The patch looks good to me.

> The change to os::readdir in os_linux.inline.hpp looks fine.
>
> I was going to suggest that rather than changing perfMemory_linux.cpp to use 
> os::readdir in an
> unusual and platform-specific way, it should instead just call ::readdir 
> directly.  However, neither
> of those is right, and that part of the change should not be made; see
> https://bugs.openjdk.java.net/browse/JDK-8134540
> Much nearly duplicated code for PerfMemory support
>

I think, in the circumstances, Michal's solution is the best option at
this point.
8134540 looks like a more long-term change currently scheduled for 12 (is
anyone currently working on it?), whereas this fix could go into 11 and remove
a couple of unneeded memory allocations.

> Looking a bit deeper, there might be some additional follow-on that could be 
> done, eliminating
> the second argument to os::readdir.
> windows: unused
> bsd: freebsd also deprecated readdir_r, I think for similar reasons.
> solaris: doc is clear about thread safety issue being about sharing the DIR*
> aix: I haven’t looked at it, but would guess it’s similar.
>
> In other words, don’t operate on the DIR* from multiple threads 
> simultaneously, and just use
> ::readdir on non-Windows.  That would all be for another RFE though.
>
>

This also seems like a more in-depth separate change and not one that
can be performed
by any of us alone, as we don't test all these platforms. As it
stands, Michal's change affects
Linux and Linux alone, and the addition of the use of NULL will make
it clearer that moving
to an os:readdir is feasible on that platform.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Michal Vala]

On 04/24/2018 09:17 PM, Kim Barrett wrote:

On Apr 23, 2018, at 3:51 AM, Michal Vala  wrote:

Hi,

following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm posting 
updated patch replacing deprecated readdir_r with readdir. Bug ID is JDK-8179887 [2].

webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/

I'm asking for the review and eventually sponsorship.


The change to os::readdir in os_linux.inline.hpp looks fine.

I was going to suggest that rather than changing perfMemory_linux.cpp to use 
os::readdir in an
unusual and platform-specific way, it should instead just call ::readdir 
directly.  However, neither
of those is right, and that part of the change should not be made; see
https://bugs.openjdk.java.net/browse/JDK-8134540
Much nearly duplicated code for PerfMemory support

Looking a bit deeper, there might be some additional follow-on that could be 
done, eliminating
the second argument to os::readdir.


That's what was I first doing. However, I have no resources to test all OSes.
I understand that this solution is not clear. However, until we remove the 
second argument and eventually merge PerfMemory code, it's useless to passing it 
on linux. That's why I did that cleanup. It can be done for all OSes under 
another bug id though.



windows: unused
bsd: freebsd also deprecated readdir_r, I think for similar reasons.
solaris: doc is clear about thread safety issue being about sharing the DIR*
aix: I haven’t looked at it, but would guess it’s similar.

In other words, don’t operate on the DIR* from multiple threads simultaneously, 
and just use
::readdir on non-Windows.  That would all be for another RFE though.




--
Michal Vala
OpenJDK QE
Red Hat Czech

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On Apr 23, 2018, at 3:51 AM, Michal Vala  wrote:
> 
> Hi,
> 
> following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm posting 
> updated patch replacing deprecated readdir_r with readdir. Bug ID is 
> JDK-8179887 [2].
> 
> webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/
> 
> I'm asking for the review and eventually sponsorship.

The change to os::readdir in os_linux.inline.hpp looks fine.

I was going to suggest that rather than changing perfMemory_linux.cpp to use 
os::readdir in an
unusual and platform-specific way, it should instead just call ::readdir 
directly.  However, neither
of those is right, and that part of the change should not be made; see
https://bugs.openjdk.java.net/browse/JDK-8134540
Much nearly duplicated code for PerfMemory support

Looking a bit deeper, there might be some additional follow-on that could be 
done, eliminating
the second argument to os::readdir.
windows: unused
bsd: freebsd also deprecated readdir_r, I think for similar reasons.
solaris: doc is clear about thread safety issue being about sharing the DIR*
aix: I haven’t looked at it, but would guess it’s similar.

In other words, don’t operate on the DIR* from multiple threads simultaneously, 
and just use
::readdir on non-Windows.  That would all be for another RFE though.

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[David Holmes]

Hi Michal,

This was discussed in a couple of different threads. Please also see:

http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021530.html

Thanks,
David

On 23/04/2018 5:51 PM, Michal Vala wrote:

Hi,

following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm 
posting updated patch replacing deprecated readdir_r with readdir. Bug 
ID is JDK-8179887 [2].


webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/

I'm asking for the review and eventually sponsorship.

Thanks!

[1] - 
http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021236.html

[2] - https://bugs.openjdk.java.net/browse/JDK-8179887

RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Michal Vala]

Hi,

following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm posting 
updated patch replacing deprecated readdir_r with readdir. Bug ID is JDK-8179887 
[2].


webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/

I'm asking for the review and eventually sponsorship.

Thanks!

[1] - http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021236.html
[2] - https://bugs.openjdk.java.net/browse/JDK-8179887
--
Michal Vala
OpenJDK QE
Red Hat Czech