Hi Alan, Thanks for looking at the patch again. I think I have addressed all your concerns (comments inline). Webrev and retest results at the end.
On 16/08/2019 11:39, Alan Bateman wrote: > I think the main changes since I looked at it previously have been in > the tests. That's mostly it. I did also fix a problem that was breaking build of x86_32. I also ensured that MAP_SYNC maps fail as expected on that arch. > On non-Linux platforms, FileChannel.map should throw UOE when invoked > with a mode map of READ_ONLY_SYNC or READ_WRITE_SYNC so I think we need > a test for that. > > MapFail seems fragile. If you add @modules java.base/jdk.internal.misc > to the test description then it could use Unsafe::isWritebackEnabled and > I think would simplify the test and checks. It would mean it could run > on all platforms. Also "MapFail" is probably too general a name for a > test in MBB because its specific to the extended map modes, not a > general test of map failing. I renamed the test to MapSyncFail and modified it to run without restriction to a specific os or cpu. I also generalized it to run twice with a boolean arg which selects mode READ_ONLY_SYNC on the first run and READ_WRITE_SYNC on the second one. The logic of the test is now to expect 1) IOException if Unsafe.isWriteBackEnabled -> true 2) UnsupportedOperationException if Unsafe.isWriteBackEnabled -> false If the wrong exception or neither exception is thrown the test fails. Case 1 currently only applies for x86_64. Case 2 applies for all other architectures. > In passing, MappedByteBuffer load/isLoaded check the fd value before > isSync, can force() do the same? Also the @return on the private isSync > method is very wordy and I don't think needs to duplicate the method > description. Sure, I have modified force() to do that check first. Of course, that means that force(int, int) is going to repeat the same test -- it has to because it may be called direct without going via force(). However, that's not really a problem since the compiler should elide the repeated check. I also shortened the text following the @return annotation as requested. Updated webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.12 Testing: Test PmemTest: passes as expected on x86_64 (only arch for which DAX file system is available) fails to pass as expected on aarch64 and x86_32 (however, this case is covered by the next test) Test MapSyncFail: passes with expected exceptions on Linux for x86_64 (IOException), aarch64 and x86_32 (UnsupportedOperationException). not tested on other arch/OS combinations (I have no access to the necessary kit). Red Hat MW tests: All still passing successfully submit test: still in progress Is it ok to push if the submit test comes back clean? regards, Andrew Dinn ----------- Senior Principal Software Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
