[WiP] Load versioned symbols from libfuse.so #117
Conversation
# Conflicts: # jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/LinuxFuseBuilder.java # jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/extr/RuntimeHelper.java # jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/extr/constants$0.java # jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/extr/constants$1.java # jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/extr/ll/RuntimeHelper.java # jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/extr/ll/constants$0.java
# Conflicts: # jfuse-linux-amd64/pom.xml
This reverts commit 5edb152.
This reverts commit b3f1c85. # Conflicts: # jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseSymbolLookup.java # jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseSymbolLookup.java
WalkthroughThis pull request updates the CI/CD pipeline configuration and revises the FUSE API integration for both aarch64 and amd64 modules. The dynamic naming and configurable library paths are introduced in the workflow. The FUSE helper classes have been refactored to replace the deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as LinuxFuseBuilder
participant Lookup as FuseSymbolLookup
participant System as System.load
participant FFI as FuseFFIHelper
Builder->>+Lookup: getInstance().open(libraryPath)
Lookup-->>-Builder: Library handle resolved
Builder->>+System: System.load(libraryPath)
System-->>-Builder: Library loaded
Builder->>+FFI: fuse_new(args, op, op_size, private_data)
FFI-->>-Builder: Fuse instance returned
Assessment against linked issues
Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
overheadhunter
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFFIHelper.java (1)
30-30: Hardcoded version string is acceptable but consider centralizing constants.
If additional versioned symbols must be supported in the future, centralizing version strings may improve maintainability.jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFFIHelper.java (1)
30-30: Consider using a constant for the versioned symbol name.
Centralizing version strings can ease future updates if additional symbols or versions need to be managed.jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseSymbolLookup.java (3)
15-16: Consider final or sealed class usage.
Declaring this class final (or sealed) can prevent unwanted subclassing. Since this class relies on a single-instance holder pattern, making it final can guard its intended usage.
33-40: Ensure thread-safety for method handles initialization.
The lazy retrieval here is fine, but be mindful that the native handles remain consistent across threads. You may want to document expected usage (e.g., callinggetInstance()once at startup) to avoid concurrency pitfalls.
64-93: Review individual errors for dlvsym and dlsym fallback.
When both symbol lookups fail, you log a single error message. Consider clarifying via logs whether both lookups failed rather than only “dlvsym failed.” This can help with diagnosing issues in debugging scenarios.jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/LinuxFuseBuilder.java (1)
40-40: Validate library path before opening.
If the provided path is invalid or unavailable,FuseSymbolLookup.getInstance().open(libraryPath)could throw unexpectedly. Consider verifying the path before invocation or implementing a friendlier error message to guide troubleshooting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/extr/fuse3/statvfs.javais excluded by!**/extr/**
📒 Files selected for processing (13)
.github/workflows/build.yml(2 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFFIHelper.java(2 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseImpl.java(1 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseSymbolLookup.java(1 hunks)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/LinuxFuseBuilder.java(1 hunks)jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java(2 hunks)jfuse-linux-amd64/pom.xml(6 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFFIHelper.java(2 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseImpl.java(1 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseSymbolLookup.java(1 hunks)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/LinuxFuseBuilder.java(1 hunks)jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java(1 hunks)jfuse-tests/src/test/java/org/cryptomator/jfuse/tests/MirrorIT.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/LinuxFuseBuilder.java (1)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseSymbolLookup.java (1)
FuseSymbolLookup(15-95)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/LinuxFuseBuilder.java (2)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseSymbolLookup.java (1)
FuseSymbolLookup(15-95)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseSymbolLookup.java (1)
FuseSymbolLookup(15-95)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFFIHelper.java (3)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFFIHelper.java (2)
FuseFFIHelper(13-47)fuse_new(20-32)jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseSymbolLookup.java (1)
FuseSymbolLookup(15-95)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseSymbolLookup.java (1)
FuseSymbolLookup(15-95)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFFIHelper.java (2)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFFIHelper.java (2)
FuseFFIHelper(13-47)fuse_new(20-32)jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseSymbolLookup.java (1)
FuseSymbolLookup(15-95)
jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseSymbolLookup.java (1)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseSymbolLookup.java (2)
FuseSymbolLookup(15-95)Holder(46-48)
🪛 actionlint (1.7.4)
.github/workflows/build.yml
20-20: label "ubuntu-24.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (36)
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseFFIHelper.java (5)
11-11: Doc comment appropriately clarifies usage of versioned symbols.
This added comment helps readers understand the specific nature offuse_new@FUSE_3.0references. No issues found.
16-16: Confirm library loading before symbol lookup.
Ensure thatFuseSymbolLookup.getInstance().open(...)is called before invokingfindOrThrow(symbol). Otherwise, unresolved symbol errors may occur if the library isn't loaded yet.
20-20: Private nested class effectively organizes thefuse_newsymbol binding.
No concerns with scoping or naming; it clearly matches the versioned symbol approach.
39-39: Public method name matches the underlying native function.
This consistent naming helps users quickly map Java calls to native FUSE APIs.
40-40: Local method handle usage enhances clarity.
Storing the handle inmh$is a neat trick for readability. Implementation is straightforward.jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseFFIHelper.java (5)
11-11: Doc comment accurately documents the new symbol reference.
This detail clarifies why standard jextract may not handle certain versioned symbols.
16-16: Validate library is opened before symbol resolution.
Similar to the aarch64 variant, be sure the library is loaded to prevent runtime errors.
20-20: Inner class scoping is clear and minimally exposed.
Using a private static class is a good pattern for isolating versioned symbol definitions.
39-39: Method name properly mirrors native API.
This direct naming convention simplifies cross-referencing the FUSE documentation.
40-40: Local reference to the MethodHandle clarifies flow.
Retaining the handle in a local variable to invoke is a clean approach. No issues noted.jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseSymbolLookup.java (8)
1-14: Overall structure: new class for dynamic symbol resolution.
This file centralizes all logic related to loading and looking up versioned symbols. A strong foundation for future expansions.
15-18: Visibility of constants is appropriately restricted.
DefiningRTLD_NOW = 0x02at class scope is straightforward, and these are limited to package usage. No issues.
19-26: FunctionDescriptors clearly define native method signatures.
Good practice linking authoritative documentation in the comments, simplifying maintenance.
27-32: Native method handles are properly declared as final.
This ensures references stay constant and can't be reassigned, preventing accidental changes.
33-40: Constructor loads required method handles.
Code comprehensively handles missing symbols withorElseThrow(), ensuring robust error reporting if unresolvable.
42-48: Singleton pattern with lazy holder.
This is a common and reliable thread-safe pattern without explicit synchronization.
50-57:openmethod gracefully handles errors.
Wrapping in a confinedArenais a good approach. ThrowingAssertionErroris somewhat unconventional, but consistent with the code's pattern.
59-95:findmethod: thorough usage of dlvsym/dlsym
• Splitting the symbol at'@'is a neat approach for handling versioned references.
• Logging the error message tostderris helpful. You may consider a structured logging approach if the code grows more complex.jfuse-tests/src/test/java/org/cryptomator/jfuse/tests/MirrorIT.java (1)
72-72: Confirm reason for disabling the-sflag.
Commenting out this flag could change how the filesystem is mounted (particularly affecting foreground or single-threaded mount modes). Ensure you still achieve the intended behavior and consider explicitly documenting why this flag was disabled.jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/LinuxFuseBuilder.java (1)
40-40: Enhances library symbol handling with versioned symbol lookupThis line adds a new step to open the library first with FuseSymbolLookup before loading it with System.load. This is essential for the new implementation of versioned symbol lookup using dlvsym.
jfuse-linux-amd64/pom.xml (6)
103-104: Architecture definition macros added properlyAdding the
__x86_64__=1and__LP64__=1macros ensures the jextract tool generates code with correct architecture-specific memory layout and type definitions for x86_64.
149-150: Consistent architecture macros for fuse_lowlevel.hThe same architecture macros are correctly applied to the fuse_lowlevel.h extraction configuration, maintaining consistency across all FUSE components.
166-169: Added architecture macros to errno extractionThe architecture-specific macros ensure proper extraction of errno constants for the x86_64 platform.
201-204: Consistent architecture macros for stat.hArchitecture macros ensure proper extraction of stat.h structures and constants for x86_64.
219-223: Reorganized fcntl.h extraction configurationThe headerClassName element has been moved below targetPackage, and architecture macros have been added for consistency with other extraction configurations.
248-249: Completed architecture macros for stdio.hArchitecture macros have been consistently applied to the stdio.h extraction configuration, which is important for proper extraction of GNU-specific rename flags.
jfuse-linux-amd64/src/test/java/org/cryptomator/jfuse/linux/amd64/FuseImplTest.java (1)
45-45: Updated mock to use new versioned fuse_new methodThe test has been updated to mock
FuseFFIHelper.fuse_newinstead of the deprecatedfuse_new_31, aligning with the implementation changes in FuseImpl and FuseFFIHelper classes.jfuse-linux-amd64/src/main/java/org/cryptomator/jfuse/linux/amd64/FuseImpl.java (1)
40-40: Updated to use versioned fuse_new methodChanged from using the deprecated
fuse_new_31to the new versionedfuse_newmethod, ensuring better compatibility with different versions of the FUSE library through the new symbol lookup mechanism.jfuse-linux-aarch64/src/test/java/org/cryptomator/jfuse/linux/aarch64/FuseImplTest.java (2)
12-12: Import modified to reference fuse_cmdline_opts from fuse3_lowlevel package.This change aligns with the broader refactoring to handle versioned symbols from the libfuse.so library more effectively.
45-45: Updated mock to use versionedfuse_newmethod instead offuse_new_31.This change is consistent with the PR objective to implement versioned symbol lookup instead of hardcoding specific version numbers in method names. The test now correctly mocks the updated method signature.
jfuse-linux-aarch64/src/main/java/org/cryptomator/jfuse/linux/aarch64/FuseImpl.java (1)
40-40: Replacedfuse_new_31withfuse_newto use versioned symbol lookup.This modification aligns with the PR objective to implement versioned symbol lookup via
dlvsymrather than hardcoding specific version numbers in method names. The change improves the flexibility and future-proofing of the codebase..github/workflows/build.yml (5)
14-14: Set fail-fast to false for the build matrix.This ensures that all architecture builds will run even if one of them fails, which is particularly useful when testing changes across different architectures.
19-22: Added configurable fuseLibPath for each architecture.Introducing the
fuseLibPathvariable for each architecture improves the configurability of the build process and makes the architecture-specific paths explicit.🧰 Tools
🪛 actionlint (1.7.4)
20-20: label "ubuntu-24.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
24-24: Updated job name to include the architecture.This change makes the CI job names more descriptive by including the architecture being tested, making it easier to identify build results for specific architectures.
37-37: Updated Maven build command to use the fuseLibPath matrix variable.This modification leverages the architecture-specific library paths defined in the matrix, making the build process more flexible and maintainable.
20-20:✅ Verification successful
Warning: Unknown GitHub runner label.
The static analysis tool indicates that "ubuntu-24.04-arm" is an unknown label for GitHub runners. Please verify that this runner label is valid and available in your GitHub environment.
🏁 Script executed:
#!/bin/bash # Check if this is a custom self-hosted runner or if there's a mistake in the label gh api repos/{repo_owner}/{repo_name}/actions/runners | jq '.runners[] | .name'Length of output: 166
Action Required: Confirm Validity of Preview Runner Label
The runner label appears to be correct, based on current GitHub Actions updates. As noted, GitHub Actions ARM runners using the label “ubuntu-24.04-arm” are in public preview as of April 2025. The static analysis tool warning is a known false positive for preview features.
- .github/workflows/build.yml (line 20): The used runner label is valid in the context of public preview.
- Action: Please confirm that your GitHub environment supports the preview ARM runner label (or refer to the latest GitHub Actions documentation) and, if confirmed, safely ignore the static analysis warning.
🧰 Tools
🪛 actionlint (1.7.4)
20-20: label "ubuntu-24.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
| public void open(String libPath) { | ||
| try (var session = Arena.ofConfined()) { | ||
| MemorySegment handle = (MemorySegment) dlopen.invokeExact(session.allocateFrom(libPath), RTLD_NOW); | ||
| libHandle.set(handle); | ||
| } catch (Throwable e) { | ||
| throw new AssertionError(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding dlclose functionality or clarifying lifecycle.
Opening the library without closing may lead to resource leaks over long uptimes. If you expect to reload or unload the library at any point, implementing a matching dlclose call or clarifying that this process is intentionally omitted would be beneficial.
|
For documentation: This approach has the problem, that the library path must be set, because symbols loaded with |
This adds a SymbolLookup implementation that uses
dlvsym.Fixes #116
Summary by CodeRabbit
Chores
Refactor
New Features
Tests