RE: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
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
On May 7, 2018, at 11:20 AM, B. Blaser <bsrbnd@gmail.com> wrote:
On 7 May 2018 at 14:19, B. Blaser <bsrbnd@gmail.com> wrote:
On 6 May 2018 at 18:35, B. Blaser <bsrbnd@gmail.com> wrote:
On 5 May 2018 at 22:26, Kim Barrett <kim.barrett@oracle.com> wrote:
On May 5, 2018, at 8:03 AM, B. Blaser <bsrbnd@gmail.com> wrote:
On 4 May 2018 at 17:42, Alan Bateman <Alan.Bateman@oracle.com> 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…
Hi Pengfei, On 20 June 2018 at 12:08, Pengfei Li <Pengfei.Li@arm.com> 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
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 <bsrbnd@gmail.com> Sent: Thursday, June 21, 2018 00:01 To: Pengfei Li <Pengfei.Li@arm.com> Cc: kim.barrett@oracle.com; core-libs-dev@openjdk.java.net; nd <nd@arm.com>; 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 <Pengfei.Li@arm.com> 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
On 20/06/2018 11: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?
I see your other mail looking to to add #pragma GCC ... to avoid using configure --disable-warnings-as-errors. I think it would be better to just replace the usage readdir_r usage in that native method. I've tested the patch below on macOS, Linux and Solaris. The SAP folks maintain the AIX port and would need to test it on that platform as it eliminates the AIX specific code for dealing with end of stream that should be needed when using readdir. -Alan diff -r 9d62da00bf15 -r 5e67e87bd6fa src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c --- a/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Sat May 26 06:59:49 2018 +0200 +++ b/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Mon Jun 25 14:17:03 2018 +0100 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2008, 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 @@ -72,7 +72,7 @@ #define fstat64 fstat #define lstat64 lstat #define dirent64 dirent -#define readdir64_r readdir_r +#define readdir64 readdir #endif #include "jni.h" @@ -720,41 +720,23 @@ JNIEXPORT jbyteArray JNICALL Java_sun_nio_fs_UnixNativeDispatcher_readdir(JNIEnv* env, jclass this, jlong value) { - struct dirent64* result; - struct { - struct dirent64 buf; - char name_extra[PATH_MAX + 1 - sizeof result->d_name]; - } entry; - struct dirent64* ptr = &entry.buf; - int res; DIR* dirp = jlong_to_ptr(value); + struct dirent64* ptr; - /* EINTR not listed as a possible error */ - /* TDB: reentrant version probably not required here */ - res = readdir64_r(dirp, ptr, &result); - -#ifdef _AIX - /* On AIX, readdir_r() returns EBADF (i.e. '9') and sets 'result' to NULL for the */ - /* directory stream end. Otherwise, 'errno' will contain the error code. */ - if (res != 0) { - res = (result == NULL && res == EBADF) ? 0 : errno; - } -#endif - - if (res != 0) { - throwUnixException(env, res); + errno = 0; + ptr = readdir64(dirp); + if (ptr == NULL) { + if (errno != 0) { + throwUnixException(env, errno); + } return NULL; } else { - if (result == NULL) { - return NULL; - } else { - jsize len = strlen(ptr->d_name); - jbyteArray bytes = (*env)->NewByteArray(env, len); - if (bytes != NULL) { - (*env)->SetByteArrayRegion(env, bytes, 0, len, (jbyte*)(ptr->d_name)); - } - return bytes; + jsize len = strlen(ptr->d_name); + jbyteArray bytes = (*env)->NewByteArray(env, len); + if (bytes != NULL) { + (*env)->SetByteArrayRegion(env, bytes, 0, len, (jbyte*)(ptr->d_name)); } + return bytes; } }
Hi Alan, On 28 June 2018 at 13:49, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 20/06/2018 11: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?
I see your other mail looking to to add #pragma GCC ... to avoid using configure --disable-warnings-as-errors.
I think it would be better to just replace the usage readdir_r usage in that native method. I've tested the patch below on macOS, Linux and Solaris. The SAP folks maintain the AIX port and would need to test it on that platform as it eliminates the AIX specific code for dealing with end of stream that should be needed when using readdir.
-Alan
I've checked AIX's 'readdir()' doc [1] and I guess you're right that it never sets 'errno' to 'EBADF' while 'readdir_r()' may return it (see [2]). I missed this on my original patch. So, your update seems fine to me even if I cannot evaluate it on AIX myself. Thanks, Bernard [1] https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/com.ibm.aix.basetr... [2] https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/com.ibm.aix.basetr...
diff -r 9d62da00bf15 -r 5e67e87bd6fa src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c --- a/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Sat May 26 06:59:49 2018 +0200 +++ b/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Mon Jun 25 14:17:03 2018 +0100 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2008, 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 @@ -72,7 +72,7 @@ #define fstat64 fstat #define lstat64 lstat #define dirent64 dirent -#define readdir64_r readdir_r +#define readdir64 readdir #endif
#include "jni.h" @@ -720,41 +720,23 @@
JNIEXPORT jbyteArray JNICALL Java_sun_nio_fs_UnixNativeDispatcher_readdir(JNIEnv* env, jclass this, jlong value) { - struct dirent64* result; - struct { - struct dirent64 buf; - char name_extra[PATH_MAX + 1 - sizeof result->d_name]; - } entry; - struct dirent64* ptr = &entry.buf; - int res; DIR* dirp = jlong_to_ptr(value); + struct dirent64* ptr;
- /* EINTR not listed as a possible error */ - /* TDB: reentrant version probably not required here */ - res = readdir64_r(dirp, ptr, &result); - -#ifdef _AIX - /* On AIX, readdir_r() returns EBADF (i.e. '9') and sets 'result' to NULL for the */ - /* directory stream end. Otherwise, 'errno' will contain the error code. */ - if (res != 0) { - res = (result == NULL && res == EBADF) ? 0 : errno; - } -#endif - - if (res != 0) { - throwUnixException(env, res); + errno = 0; + ptr = readdir64(dirp); + if (ptr == NULL) { + if (errno != 0) { + throwUnixException(env, errno); + } return NULL; } else { - if (result == NULL) { - return NULL; - } else { - jsize len = strlen(ptr->d_name); - jbyteArray bytes = (*env)->NewByteArray(env, len); - if (bytes != NULL) { - (*env)->SetByteArrayRegion(env, bytes, 0, len, (jbyte*)(ptr->d_name)); - } - return bytes; + jsize len = strlen(ptr->d_name); + jbyteArray bytes = (*env)->NewByteArray(env, len); + if (bytes != NULL) { + (*env)->SetByteArrayRegion(env, bytes, 0, len, (jbyte*)(ptr->d_name)); } + return bytes; } }
participants (3)
-
Alan Bateman
-
B. Blaser
-
Pengfei Li