> On Apr 25, 2018, at 10:51 AM, Andrew Hughes <gnu.and...@redhat.com> wrote: > > On 24 April 2018 at 20:17, Kim Barrett <kim.barr...@oracle.com> wrote: >>> On Apr 23, 2018, at 3:51 AM, Michal Vala <mv...@redhat.com> 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
