RFR: 8293067: (fs) Implement WatchService using system library (macOS) [v3]

[Maxim Kartashev]

On Tue, 20 Sep 2022 20:55:48 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Removed the change unnecessary without recursive watch
>
> src/java.base/macosx/classes/sun/nio/fs/MacOSXWatchService.java line 129:
> 
>> 127:      * that need to be re-scanned.
>> 128:      */
>> 129:     private void callback(final long eventStreamRef,
> 
> Could this be named something more descriptive than `callback`, perhaps even `handleEvents` even though there is a method of the same name defined in `MacOSXWatchKey`?

Sure.

> src/java.base/macosx/classes/sun/nio/fs/MacOSXWatchService.java line 396:
> 
>> 394:                 if (!relativeRootPath.equals(path)) {
>> 395:                     // Ignore events from subdirectories for now.
>> 396:                     continue;
> 
> Should a line
> 
> eventFlagsPtr += SIZEOF_FS_EVENT_STREAM_EVENT_FLAGS;
> 
> be added here? (The definition of `SIZEOF_FS_EVENT_STREAM_EVENT_FLAGS` would have to be moved up.)

Yes, most certainly. Thanks for catching this!

> src/java.base/macosx/native/libnio/fs/MacOSXWatchService.c line 89:
> 
>> 87: {
>> 88:     JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2);
>> 89:     if (!env) { // Shouldn't happen as run loop starts from Java code
> 
> Is this check, hence the `jvm` extern, really necessary?

The check isn't strictly necessary, so I'm goign to drop it (there's a JNI version check in `nio_util.c`), but the `jvm` extern seems necessary because that's how `JNIEnv` is obtained and the latter is necessary to communicate the events back to the Java side.

-------------

PR: https://git.openjdk.org/jdk/pull/10140