Skip to content

Refactor module mount load in rust #8846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Refactor module mount load in rust #8846

wants to merge 1 commit into from

Conversation

yujincheng08
Copy link
Collaborator

@yujincheng08 yujincheng08 commented Mar 10, 2025

Todo:

@yujincheng08 yujincheng08 force-pushed the module.rs branch 5 times, most recently from 36b06e7 to 515d7c5 Compare March 16, 2025 16:52
@yujincheng08 yujincheng08 marked this pull request as ready for review March 16, 2025 17:30
@topjohnwu topjohnwu added the next Planned for the release *after* the next release label Mar 19, 2025
@vvb2060
Copy link
Collaborator

vvb2060 commented Mar 20, 2025

where is #8790 ?

@yujincheng08
Copy link
Collaborator Author

@vvb2060 fixed

@yujincheng08 yujincheng08 requested a review from Copilot April 2, 2025 04:12
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the module mount load in Rust to build a module mount tree and support zygisk and magisk nodes, along with various mount operations.

  • Added new features and module support in native/src/core/lib.rs for deploying modules.
  • Modified error logging behavior in native/src/base/result.rs and improved file/dir operations in native/src/base/files.rs and native/src/base/dir.rs.
  • Introduced resize functionality in native/src/base/cstr.rs for both Utf8CString and FsPathBuf.

Reviewed Changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
native/src/core/lib.rs Added a new C++ extern function deploy_modules and module import.
native/src/base/result.rs Changed log_ok function signature to return Option.
native/src/base/files.rs Updated libc constants and added functions for whiteout check and bind mounts.
native/src/base/dir.rs Updated directory open function with enhanced flags.
native/src/base/cstr.rs Added resize methods for Utf8CString and FsPathBuf.
Files not reviewed (2)
  • native/src/core/module.cpp: Language not supported
  • native/src/core/node.hpp: Language not supported
Comments suppressed due to low confidence (1)

native/src/core/lib.rs:245

  • [nitpick] Consider using a slice type (&[ModuleInfo]) instead of &Vec for improved flexibility and idiomatic Rust usage.
fn deploy_modules(module_list: &Vec<ModuleInfo>, zygisk_lib: &CxxString, magisk_path: &CxxString) -> bool;

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the module mount load implementation in Rust, simplifying the mounting process while adding support for zygisk and magisk nodes. Key changes include the removal of the legacy node.hpp file, updates in module.cpp to replace the previous mount tree deployment with a consolidated deploy_modules call, and several minor enhancements across auxiliary modules (lib.rs, base/mount.rs, base/dir.rs, etc).

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
native/src/core/node.hpp Removed legacy node definitions and related mount tree logic
native/src/core/module.cpp Replaced previous mounting logic with deploy_modules and adjusted magisk path selection logic
native/src/core/lib.rs Enabled a new Rust feature and updated module imports
native/src/base/result.rs Updated signature of log_ok for improved clarity
native/src/base/mount.rs Added bind_mount_ro_to function for read-only bind mounts
native/src/base/files.rs Added helper is_whiteout to FileAttr
native/src/base/dir.rs Updated flag usage in open_as_dir_at for safer directory handling
Comments suppressed due to low confidence (2)

native/src/core/node.hpp:1

  • The entire node.hpp file has been removed; please ensure that all necessary declarations and type definitions previously provided by this file are now available in their new location to maintain functionality.
#pragma once

native/src/core/module.cpp:57

  • Replacing the previous mounting process with deploy_modules shifts key responsibilities; please confirm that deploy_modules fully replicates the removed behavior and document any differences.
deploy_modules(module_list, native_bridge, magisk_path);

Comment on lines 29 to 40
if (get_magisk_tmp() != "/sbin"sv || !str_contains(getenv("PATH") ?: "", "/sbin")) {
// Need to inject our binaries into /system/bin
inject_magisk_bins(system);
magisk_path = "/system/bin";
for (struct stat st{}; auto &item: split(getenv("PATH"), ":")) {
item.erase(0, item.starts_with("/system/") ? 8 : 1);
auto &&system_path = "/system/"s + item;
if (stat(system_path.data(), &st) == 0 && st.st_mode & S_IXOTH) {
magisk_path = std::move(system_path);
break;
}
}
}
Copy link
Preview

Copilot AI May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The new logic for selecting magisk_path relies on parsing getenv('PATH'); consider adding inline comments or assertions to clarify the candidate prioritization and ensure that the fallback default is intentional.

Copilot uses AI. Check for mistakes.

@topjohnwu
Copy link
Owner

I tried to understand the code in this PR, but failed to do so. The implementation here diverged too far from the original C++ implementation, making it very difficult to decipher and modify (I wanted to change a few things but it became unmanageable).

I've created my own PR #8999, which documents clearly how the code work. It actually fixed a few bugs in the C++ implementation. I'd love you to review it.

@topjohnwu
Copy link
Owner

Superseded by #8999

@topjohnwu topjohnwu closed this May 9, 2025
@topjohnwu topjohnwu deleted the module.rs branch May 9, 2025 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next Planned for the release *after* the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants