Add type selection fallback for fs open. #5
Conversation
Right now it is not possible to either force or to pick a filesystem provider that does not have content probing facilities. This patch adds a fallback filesystem provider selection choice screen populated with filesystems that are available but do not probe their contents if no probing filesystem claimed the resource in question.
WalkthroughThe changes introduce enhancements to Ghidra's filesystem handling. A new method is added to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileSystemBrowserPlugin
participant FileSystemFactoryMgr
participant FileSystemService
User->>FileSystemBrowserPlugin: Open file system
FileSystemBrowserPlugin->>FileSystemFactoryMgr: Probe for filesystem provider
alt Provider found
FileSystemBrowserPlugin->>FileSystemService: Mount filesystem
else No provider found
FileSystemBrowserPlugin->>FileSystemFactoryMgr: getFileSystemInfoRecs()
FileSystemBrowserPlugin->>User: Show fallback chooser dialog
alt User selects provider
FileSystemBrowserPlugin->>FileSystemService: Attempt to mount selected filesystem
alt Already mounted
FileSystemBrowserPlugin->>FileSystemService: Return existing reference
else Not mounted
FileSystemBrowserPlugin->>FileSystemService: Add new filesystem
end
else User cancels or mounting fails
FileSystemBrowserPlugin->>User: Show warning message
end
end
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
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Ghidra/Features/Base/src/main/java/ghidra/plugins/fsbrowser/FileSystemBrowserPlugin.java (1)
269-303: Well-implemented fallback filesystem selection mechanism.This method provides a clean implementation for manual filesystem selection:
- Filters providers that implement the
GFileSystemProbeManualinterface- Sorts them by description for better user experience
- Handles user selection and filesystem mounting
- Properly manages resources and checks for existing mounts
Consider adding more detailed error logging if mounting fails, which would help with troubleshooting.
The only suggestion would be to add more specific error logging in the case where mountFileSystem fails but doesn't throw an exception (the case where
fsis null at line 292). This would help with troubleshooting problems during manual filesystem selection:if (fs != null) { FileSystemRef fsRef = fsService().getMountedFilesystem(containerFSRL.makeNested(chosenFilesystem.getType())); if (fsRef != null) { fs.close(); return fsRef; } fsService().addFileSystem(fs); return fs.getRefManager().create(); + } else { + Msg.debug(this, "Failed to mount filesystem of type " + chosenFilesystem.getType() + + " for " + containerFSRL.getName()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Ghidra/Features/Base/src/main/java/ghidra/formats/gfilesystem/FileSystemService.java(1 hunks)Ghidra/Features/Base/src/main/java/ghidra/formats/gfilesystem/factory/FileSystemFactoryMgr.java(1 hunks)Ghidra/Features/Base/src/main/java/ghidra/formats/gfilesystem/factory/GFileSystemProbeManual.java(1 hunks)Ghidra/Features/Base/src/main/java/ghidra/plugins/fsbrowser/FileSystemBrowserPlugin.java(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Ghidra/Features/Base/src/main/java/ghidra/plugins/fsbrowser/FileSystemBrowserPlugin.java (1)
Ghidra/Features/Base/src/main/java/ghidra/formats/gfilesystem/factory/FileSystemFactoryMgr.java (1)
FileSystemFactoryMgr(39-358)
🔇 Additional comments (5)
Ghidra/Features/Base/src/main/java/ghidra/formats/gfilesystem/factory/FileSystemFactoryMgr.java (1)
350-357: Well-implemented accessor for filesystem providers.This is a clean implementation that safely provides access to the registered filesystem providers by returning a defensive copy of the internal map, which prevents external modification of the internal state.
Ghidra/Features/Base/src/main/java/ghidra/formats/gfilesystem/FileSystemService.java (1)
965-976: Thread-safe implementation to prevent duplicate filesystem mounts.This implementation correctly synchronizes on the
fsInstanceManagerand performs a check before adding to prevent duplicate filesystems at the same location. The method complements the existing API and follows the established patterns in the class.Ghidra/Features/Base/src/main/java/ghidra/formats/gfilesystem/factory/GFileSystemProbeManual.java (1)
1-24: Good design for marker interface.This marker interface is well-designed to identify filesystem probes that can be manually selected by users. The empty interface pattern is appropriate for this classification purpose, and the documentation clearly explains its intent.
Ghidra/Features/Base/src/main/java/ghidra/plugins/fsbrowser/FileSystemBrowserPlugin.java (2)
19-46: Required imports added for new functionality.The new imports support the fallback filesystem selection mechanism and properly maintain code organization.
253-258: Improved user experience with fallback mechanism.The modified code now calls the fallback chooser before showing a warning, giving users an opportunity to manually select a filesystem when automatic detection fails.
| * | ||
| * @param fs {@link GFileSystem} to add to the managed filesystems list. | ||
| */ | ||
| public void addFileSystem(GFileSystem fs) { |
There was a problem hiding this comment.
Wouldn't this leak the fs instance being added if it was already mounted?
Probably should close the passed-in fs if already mounted.
Won't be an issue with how you are currently using it because you check for that, but other people calling it could break that assumption.
Right now it is not possible to either force or to pick a filesystem provider that does not have content probing facilities. This patch adds a fallback filesystem provider selection choice screen populated with filesystems that are available but do not probe their contents if no probing filesystem claimed the resource in question.
This fixes NationalSecurityAgency#4448.
Summary by CodeRabbit