Feature: Implement fields capable_ext and want_ext #135
Conversation
* add fuse_get_feature_flag * throw UnsupportedOperationException by default
…versions < 3.17 * change FuseConnInfoImpl to class * add subclass FuseConnInfoImpl317 adding actual implementation of fields and functions * adjust tests
WalkthroughAdds libfuse 3.17-aware extended capability and feature-flag support: new API defaults, versioned FuseConnInfo implementations for amd64/aarch64, runtime branching in init(), optional FFI bindings for feature-flag functions, unit tests, a small Windows import cleanup, and javadoc tag configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant FuseImpl
participant fuse_h
participant ConnInfo as FuseConnInfoImpl / FuseConnInfoImpl317
participant FuseFunctions
App->>FuseImpl: init()
FuseImpl->>fuse_h: fuse_version()
alt fuse_version >= 317
note right of FuseImpl #DFF2E1: use 64-bit ext fields & feature APIs
FuseImpl->>ConnInfo: instantiate FuseConnInfoImpl317(segment)
FuseImpl->>FuseFunctions: fuse_set_feature_flag(connInfo, READDIRPLUS)
alt symbol present
FuseFunctions-->>FuseImpl: boolean result
else symbol missing
FuseFunctions-->>FuseImpl: throws UnsupportedOperationException
end
else fuse_version < 317
note right of FuseImpl #FFF4D6: use legacy 32-bit want field
FuseImpl->>ConnInfo: instantiate FuseConnInfoImpl(segment)
FuseImpl->>ConnInfo: setWant(connInfo.want() | READDIRPLUS)
end
FuseImpl-->>App: continue init with chosen ConnInfo
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317.java (1)
13-18: Remove commented-out offset-based access.Same as amd64 variant; keep only the jextract accessors.
🧹 Nitpick comments (20)
jfuse-api/src/main/java/org/cryptomator/jfuse/api/FuseConnInfo.java (4)
326-326: Javadoc: reference the Java accessor, not the C field.Use “capableExt()” instead of “capable_ext” for clarity and consistency.
- * @apiNote Since libFUSE 3.17, this field is deprecated left over for ABI compatibility, use capable_ext + * @apiNote Since libFUSE 3.17, this field is deprecated and left over for ABI compatibility. Use capableExt().
337-339: Javadoc: prefer method names and helpers over raw field names.- * @apiNote Since libFUSE 3.17, this field is deprecated left over for ABI compatibility. - * Use want_ext with the helper functions fuse_set_feature_flag() / fuse_unset_feature_flag() + * @apiNote Since libFUSE 3.17, this field is deprecated and left over for ABI compatibility. + * Prefer wantExt() in combination with the helper functions fuse_set_feature_flag()/fuse_unset_feature_flag().
346-348: Javadoc: mirror the wording used above for want().- * @apiNote Since libFUSE 3.17, this field is deprecated left over for ABI compatibility. - * Use want_ext with the helper functions fuse_set_feature_flag() / fuse_unset_feature_flag() + * @apiNote Since libFUSE 3.17, this field is deprecated and left over for ABI compatibility. + * Prefer setWantExt() together with fuse_set_feature_flag()/fuse_unset_feature_flag().
586-588: Wrong exception message in getFeatureFlag().Message mentions unset instead of get. Fix to avoid confusion.
- throw new UnsupportedOperationException("Loaded library does not implement fuse_unset_feature_flag"); + throw new UnsupportedOperationException("Loaded library does not implement fuse_get_feature_flag");jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (1)
15-18: Nit: “cannot” instead of “can not”.- * These method references can not be jextract'ed, partly due to jextract not being able to understand {@code #define}, + * These method references cannot be jextract'ed, partly due to jextract not being able to understand {@code #define},jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseImpl.java (1)
112-117: Consider checking the return value of setFeatureFlag().If the flag isn’t supported, you currently ignore the boolean. Log or handle the false case (e.g., warn), without falling back to deprecated want on >=3.17.
- connInfo.setFeatureFlag(FuseConnInfo.FUSE_CAP_READDIRPLUS); + if (!connInfo.setFeatureFlag(FuseConnInfo.FUSE_CAP_READDIRPLUS)) { + // Optional: log at DEBUG/WARN that READDIRPLUS isn’t supported by the loaded libfuse/kernel. + }jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseImpl.java (1)
112-117: Harden 3.17 path against missing symbol; add safe fallback.If
fuse_set_feature_flagis unavailable despitefuse_version() >= 317(misreported/older lib on PATH),setFeatureFlagmay throwUnsupportedOperationException. Wrap and fall back to legacywantto keep READDIRPLUS enabled best‑effort.Apply this diff:
- if (fuse_h.fuse_version() >= 317) { - connInfo = new FuseConnInfoImpl317(conn); - connInfo.setFeatureFlag(FuseConnInfo.FUSE_CAP_READDIRPLUS); - } else { + if (fuse_h.fuse_version() >= 317) { + connInfo = new FuseConnInfoImpl317(conn); + try { + connInfo.setFeatureFlag(FuseConnInfo.FUSE_CAP_READDIRPLUS); + } catch (UnsupportedOperationException e) { + // fallback for inconsistent libfuse builds + connInfo.setWant(connInfo.want() | FuseConnInfo.FUSE_CAP_READDIRPLUS); + } + } else { connInfo.setWant(connInfo.want() | FuseConnInfo.FUSE_CAP_READDIRPLUS); }jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl.java (1)
10-14: Make MemorySegment field immutable.
segmentnever changes after construction and is accessed by subclasses. Mark itfinalfor safer semantics.Apply this diff:
- protected MemorySegment segment; + protected final MemorySegment segment;jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java (2)
173-188: Fix typo in variable name (Received).Minor readability nit.
Apply this diff:
- var consumerRecievedConnInfo = new AtomicReference<FuseConnInfo>(); + var consumerReceivedConnInfo = new AtomicReference<FuseConnInfo>(); Mockito.doAnswer(invocation -> { - consumerRecievedConnInfo.set(invocation.getArgument(0)); + consumerReceivedConnInfo.set(invocation.getArgument(0)); return null; }).when(fuseOps).init(Mockito.any(), Mockito.any()); @@ - Assertions.assertInstanceOf(FuseConnInfoImpl.class, consumerRecievedConnInfo.get()); - Assertions.assertEquals(FuseConnInfo.FUSE_CAP_READDIRPLUS, consumerRecievedConnInfo.get().want() & FuseConnInfo.FUSE_CAP_READDIRPLUS); + Assertions.assertInstanceOf(FuseConnInfoImpl.class, consumerReceivedConnInfo.get()); + Assertions.assertEquals(FuseConnInfo.FUSE_CAP_READDIRPLUS, consumerReceivedConnInfo.get().want() & FuseConnInfo.FUSE_CAP_READDIRPLUS);
181-185: Optional: assert that feature-flag helper wasn’t called for <3.17.Makes the intent explicit in addition to the thrown‑if‑called stub.
Add after
fuseImpl.init(...):fuseFunctionsClass.verify(() -> FuseFunctions.fuse_set_feature_flag(Mockito.any(), Mockito.anyLong()), Mockito.never());jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java (3)
173-188: Fix typo in variable name (Received).Mirror the aarch64 test for consistency.
Apply this diff:
- var consumerRecievedConnInfo = new AtomicReference<FuseConnInfo>(); + var consumerReceivedConnInfo = new AtomicReference<FuseConnInfo>(); Mockito.doAnswer(invocation -> { - consumerRecievedConnInfo.set(invocation.getArgument(0)); + consumerReceivedConnInfo.set(invocation.getArgument(0)); return null; }).when(fuseOps).init(Mockito.any(), Mockito.any()); @@ - Assertions.assertInstanceOf(FuseConnInfoImpl.class, consumerRecievedConnInfo.get()); - Assertions.assertEquals(FuseConnInfo.FUSE_CAP_READDIRPLUS, consumerRecievedConnInfo.get().want() & FuseConnInfo.FUSE_CAP_READDIRPLUS); + Assertions.assertInstanceOf(FuseConnInfoImpl.class, consumerReceivedConnInfo.get()); + Assertions.assertEquals(FuseConnInfo.FUSE_CAP_READDIRPLUS, consumerReceivedConnInfo.get().want() & FuseConnInfo.FUSE_CAP_READDIRPLUS);
181-185: Optional: assert helper not called on <3.17.Same as aarch64.
Add after
fuseImpl.init(...):fuseFunctionsClass.verify(() -> FuseFunctions.fuse_set_feature_flag(Mockito.any(), Mockito.anyLong()), Mockito.never());
206-214: Nit: keep naming consistent with aarch64 test.Rename local
consumerRecievedConnInfo->consumerReceivedConnInfohere as well.Apply this diff:
- var consumerRecievedConnInfo = new AtomicReference<FuseConnInfo>(); + var consumerReceivedConnInfo = new AtomicReference<FuseConnInfo>(); Mockito.doAnswer(invocation -> { - consumerRecievedConnInfo.set(invocation.getArgument(0)); + consumerReceivedConnInfo.set(invocation.getArgument(0)); return null; }).when(fuseOps).init(Mockito.any(), Mockito.any()); @@ - Assertions.assertInstanceOf(FuseConnInfoImpl317.class, consumerRecievedConnInfo.get()); + Assertions.assertInstanceOf(FuseConnInfoImpl317.class, consumerReceivedConnInfo.get());jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl.java (1)
10-14: Make MemorySegment field immutable.Same rationale as amd64: mark
segmentfinal.Apply this diff:
- protected MemorySegment segment; + protected final MemorySegment segment;jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317.java (2)
13-18: Remove commented-out offset-based access.Dead/commented code adds noise; the direct accessor is correct for jextract outputs.
@Override public long capableExt() { - //old school style - //return segment.get(fuse_conn_info.capable_ext$layout().withByteAlignment(1), fuse_conn_info.capable_ext$offset()); return fuse_conn_info.capable_ext(segment); }
30-43: Behavior when feature-flag symbols are missing.These delegate to optional native symbols that may throw UnsupportedOperationException. That’s fine; just ensure call sites handle it for <3.17.
Consider documenting this in the interface Javadoc and adding a tiny utility like
FuseFunctions.featureFlagsAvailable()to allow pre-checks.jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317Test.java (2)
34-38: Fix display name to match method (capableExt).Minor naming nit to avoid confusion in reports.
return Stream.of( - Arguments.arguments((SetInMemorySegment) fuse_conn_info::want_ext, Named.of("wantExt()", (GetInConnInfo) FuseConnInfo::wantExt)), - Arguments.arguments((SetInMemorySegment) fuse_conn_info::capable_ext, Named.of("capable()", (GetInConnInfo) FuseConnInfo::capableExt)) + Arguments.arguments((SetInMemorySegment) fuse_conn_info::want_ext, Named.of("wantExt()", (GetInConnInfo) FuseConnInfo::wantExt)), + Arguments.arguments((SetInMemorySegment) fuse_conn_info::capable_ext, Named.of("capableExt()", (GetInConnInfo) FuseConnInfo::capableExt)) );
61-64: Add independence check between fields (optional).Verify
setWantExtdoesn’t altercapable_ext.public static Stream<Arguments> testSetters() { return Stream.of( Arguments.arguments(Named.of("setWantExt()", (SetInConnInfo) FuseConnInfo::setWantExt), (GetInMemorySegment) fuse_conn_info::want_ext) ); } + +@DisplayName("setWantExt() must not change capable_ext") +@org.junit.jupiter.api.Test +void testSetWantExtDoesNotChangeCapable() { + try (var arena = Arena.ofConfined()) { + var segment = fuse_conn_info.allocate(arena); + fuse_conn_info.capable_ext(segment, 123L); + var connInfo = new FuseConnInfoImpl317(segment); + connInfo.setWantExt(42L); + Assertions.assertEquals(123L, fuse_conn_info.capable_ext(segment)); + } +}jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317Test.java (2)
34-38: Fix display name to match method (capableExt).Aligns with API naming.
return Stream.of( - Arguments.arguments((SetInMemorySegment) fuse_conn_info::want_ext, Named.of("wantExt()", (GetInConnInfo) FuseConnInfo::wantExt)), - Arguments.arguments((SetInMemorySegment) fuse_conn_info::capable_ext, Named.of("capable()", (GetInConnInfo) FuseConnInfo::capableExt)) + Arguments.arguments((SetInMemorySegment) fuse_conn_info::want_ext, Named.of("wantExt()", (GetInConnInfo) FuseConnInfo::wantExt)), + Arguments.arguments((SetInMemorySegment) fuse_conn_info::capable_ext, Named.of("capableExt()", (GetInConnInfo) FuseConnInfo::capableExt)) );
61-64: Mirror the independence check (optional).Keep test parity across architectures.
public static Stream<Arguments> testSetters() { return Stream.of( Arguments.arguments(Named.of("setWantExt()", (SetInConnInfo) FuseConnInfo::setWantExt), (GetInMemorySegment) fuse_conn_info::want_ext) ); } + +@DisplayName("setWantExt() must not change capable_ext") +@org.junit.jupiter.api.Test +void testSetWantExtDoesNotChangeCapable() { + try (var arena = Arena.ofConfined()) { + var segment = fuse_conn_info.allocate(arena); + fuse_conn_info.capable_ext(segment, 123L); + var connInfo = new FuseConnInfoImpl317(segment); + connInfo.setWantExt(42L); + Assertions.assertEquals(123L, fuse_conn_info.capable_ext(segment)); + } +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
jfuse-api/src/main/java/org/cryptomator/jfuse/api/FuseConnInfo.java(3 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl.java(1 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317.java(1 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java(2 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseImpl.java(1 hunks)jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317Test.java(1 hunks)jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java(2 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl.java(1 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317.java(1 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.java(3 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseImpl.java(1 hunks)jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317Test.java(1 hunks)jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java(2 hunks)jfuse-win/src/main/java/org/cryptomator/jfuse/win/FuseImpl.java(0 hunks)pom.xml(1 hunks)
💤 Files with no reviewable changes (1)
- jfuse-win/src/main/java/org/cryptomator/jfuse/win/FuseImpl.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-27T13:14:40.963Z
Learnt from: overheadhunter
PR: cryptomator/jfuse#61
File: jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseImpl.java:105-0
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The `init` method in `FuseImpl.java` returns `MemorySegment.NULL` because the implementation does not use the `private_data` field of the `fuse_context` struct, making it safe and intentional to return `NULL`.
Applied to files:
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseImpl.javajfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.javajfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseImpl.javajfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317.javajfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317.javajfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.javajfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl.javajfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.javajfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.javajfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl.java
🧬 Code graph analysis (7)
jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java (1)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.java (1)
FuseFunctions(19-86)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317.java (1)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (1)
FuseFunctions(19-86)
jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java (2)
jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java (5)
Nested(34-68)Nested(114-160)Nested(162-216)Nested(218-261)Nested(263-329)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (1)
FuseFunctions(19-86)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl.java (1)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl.java (1)
FuseConnInfoImpl(8-106)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (1)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.java (2)
FuseFunctions(19-86)Holder(47-49)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.java (1)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (2)
FuseFunctions(19-86)Holder(47-49)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl.java (1)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl.java (1)
FuseConnInfoImpl(8-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test jfuse-mac
- GitHub Check: Test jfuse-win
🔇 Additional comments (5)
pom.xml (1)
122-140: Javadoc tag config LGTM — verify via effective POMSandbox lacks Maven (mvn: command not found), so I couldn't confirm pluginManagement merged into the executed plugin; run locally and paste the matching snippet:
mvn -q help:effective-pom | rg -nP '(?s)<artifactId>maven-javadoc-plugin</artifactId>.*?<configuration>.*?<tags>'jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java (2)
12-12: LGTM: new import is used in parseArgs tests.
206-214: Verify FUSE_UNSET_FEATURE_FLAG FunctionDescriptor in FuseFunctionsrg returned "No such file or directory" so FuseFunctions.java couldn't be located. If present (jfuse-linux-aarch64/src/main/java//FuseFunctions.java and jfuse-linux-amd64/src/main/java//FuseFunctions.java), ensure FUSE_UNSET_FEATURE_FLAG is declared as:
private static final FunctionDescriptor FUSE_UNSET_FEATURE_FLAG = FunctionDescriptor.ofVoid(ADDRESS, JAVA_LONG);Invocation remains: method.invokeExact(fuse_conn_info, flag);
jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java (1)
27-27: LGTM: AtomicReference import is required for capturing connInfo.jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317.java (1)
7-11: Resolved — base class already exposessegment.Both amd64 and aarch64 FuseConnInfoImpl declare
protected MemorySegment segmentand provide apublic MemorySegment segment()accessor (files: jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl.java, jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl.java).
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317.java
Show resolved
Hide resolved
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java
Show resolved
Hide resolved
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (2)
23-26: FunctionDescriptor signatures now match libfuse 3.17.set/get return int; unset is void. This aligns with the native API and avoids invokeExact crashes.
Consider adding short comments for get/unset similar to the set() link for quick cross‑referencing in the future.
77-84: get wrapper is correct; consider DRYing across arches.Implementation matches amd64 1:1. Optional: extract these three wrappers into shared code to reduce drift between arch modules.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java(2 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-27T13:14:40.963Z
Learnt from: overheadhunter
PR: cryptomator/jfuse#61
File: jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseImpl.java:105-0
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The `init` method in `FuseImpl.java` returns `MemorySegment.NULL` because the implementation does not use the `private_data` field of the `fuse_context` struct, making it safe and intentional to return `NULL`.
Applied to files:
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java
🧬 Code graph analysis (1)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (1)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFunctions.java (2)
FuseFunctions(19-86)Holder(47-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test jfuse-win
- GitHub Check: Test jfuse-mac
🔇 Additional comments (7)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (7)
8-8: Import for Optional is appropriate.Needed for the Optional fields introduced below.
12-12: Correct value layout for uint64_t flags.JAVA_LONG is the right choice on aarch64 (LP64).
17-17: Doc tweak is clear.Accurately calls out SymbolLookup limitation wrt dlvsym.
28-31: Weak linking via Optional is the right strategy.Keeps 3.16− safe without forcing symbol resolution at load time.
39-44: Downcall handle lookup logic LGTM.Optional wrappers cleanly map missing symbols to UnsupportedOperationException.
59-66: Boolean mapping for fuse_set_feature_flag is correct.Returns non‑zero as true; consistent with amd64 implementation.
68-75: Add test for fuse_unset_feature_flag (exercise unset path)Descriptor and wrapper parity across amd64 and aarch64 is verified; no tests exercise the unset path. Add a test that sets a feature flag, calls fuse_unset_feature_flag, and asserts fuse_get_feature_flag(...) == false (requires libfuse 3.17+).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java (1)
162-215: Init tests cover both <3.17 and ≥3.17 paths; add two small assertionsLooks solid and matches amd64 parity. Add explicit “no call” verification for <3.17 and verify init() invocation in both tests.
Apply this minimal diff:
@@ public void testInit316() { - fuseImpl.init(connInfo, fuseConfig); + fuseImpl.init(connInfo, fuseConfig); + // ensure we never try to use feature flags on <3.17 + fuseFunctionsClass.verify(() -> FuseFunctions.fuse_set_feature_flag(Mockito.any(), Mockito.anyLong()), Mockito.never()); + Mockito.verify(fuseOps).init(Mockito.any(), Mockito.any()); @@ public void testInit317() { - fuseImpl.init(connInfo, fuseConfig); + fuseImpl.init(connInfo, fuseConfig); fuseFunctionsClass.verify(() -> FuseFunctions.fuse_set_feature_flag(connInfo, FuseConnInfo.FUSE_CAP_READDIRPLUS)); + Mockito.verify(fuseOps).init(Mockito.any(), Mockito.any());Optional: Add a negative-case test for ≥3.17 when fuse_set_feature_flag returns false to document expected behavior (fallback vs. no-op). I can draft it if you want.
jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317Test.java (3)
20-33: *Add a 64-bit value round-trip to exercise _ext fields beyond 32 bitsCurrent checks use 42; include a value > 2^32 to assert true 64-bit handling.
Add this test:
@DisplayName("64-bit round-trip on *_ext fields") @Test void testRoundTrip64BitValues() { try (var arena = Arena.ofConfined()) { var segment = fuse_conn_info.allocate(arena); var connInfo = new FuseConnInfoImpl317(segment); long value = 1L << 40; // getters via native-set -> Java-get fuse_conn_info.want_ext(segment, value); Assertions.assertEquals(value, connInfo.wantExt()); fuse_conn_info.capable_ext(segment, value); Assertions.assertEquals(value, connInfo.capableExt()); // setters via Java-set -> native-get connInfo.setWantExt(value); Assertions.assertEquals(value, fuse_conn_info.want_ext(segment)); } }
41-46: Avoid autoboxing in test helper interfaces (nit)Using Long causes boxing. Custom primitive SAMs would remove it, though this is micro in tests.
If you care to change later:
- interface SetInMemorySegment { void accept(MemorySegment s, long v); }
- interface GetInConnInfo { long apply(FuseConnInfo c); }
- interface SetInConnInfo { void accept(FuseConnInfo c, long v); }
- interface GetInMemorySegment { long apply(MemorySegment s); }
47-59: Setter path is fine; mirror 64-bit case (optional)Consider a parallel 64-bit setter-focused test to complement the new round-trip.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
jfuse-api/src/main/java/org/cryptomator/jfuse/api/FuseConnInfo.java(3 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl.java(1 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317.java(1 hunks)jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317Test.java(1 hunks)jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java(2 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl.java(1 hunks)jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317Test.java(1 hunks)jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317Test.java
- jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317.java
- jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java
- jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl.java
- jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl.java
- jfuse-api/src/main/java/org/cryptomator/jfuse/api/FuseConnInfo.java
🧰 Additional context used
🧬 Code graph analysis (2)
jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317Test.java (1)
jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseConnInfoImpl317Test.java (1)
FuseConnInfoImpl317Test(18-72)
jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java (2)
jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java (5)
Nested(34-68)Nested(114-160)Nested(162-216)Nested(218-261)Nested(263-329)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFunctions.java (1)
FuseFunctions(19-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test jfuse-win
- GitHub Check: Test jfuse-linux-aarch64
- GitHub Check: Test jfuse-mac
🔇 Additional comments (4)
jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java (2)
12-12: Import aligns with updated parseArgs testsCorrectly adds fuse_cmdline_opts for help/debug/singlethread flags in tests.
27-27: AtomicReference for capturing FuseConnInfoGood switch; safer than relying on side effects.
jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseConnInfoImpl317Test.java (2)
34-39: MethodSource wiring is correct and mirrors amd64Good parity and readable parameter names.
61-65: Setters MethodSource is conciseCovers setWantExt -> want_ext linkage cleanly.
This PR closes #122.
These fields were introduced with libfuse 3.17.
To ensure downward compatibility and safe usage (aka no jvm crashes), the
FuseConnInfoImplrecord was converted into a class and subclassed withFuseConnInfoImpl317, which is the only class with a real implementation for the fields and methods. Duringfuse_initwe check the fuse version in our decorator and depending on the resultREADDIR_PLUSflag with the correct methods.Summary by CodeRabbit