feat: add rootfsPropagation support and improve error handling#105
Conversation
- Add rootfsPropagation field to linux config (shared/slave/private/unbindable) - Fix mount propagation flag handling (use |= instead of &=) - Replace std::cerr with LINYAPS_BOX_ERR() for consistent logging - Use _exit() instead of exit() in child processes - Improve error messages with better context Allows control over mount propagation for the root filesystem according to OCI spec. Signed-off-by: ComixHe <ComixHe1895@outlook.com>
|
Here's the code health analysis summary for commits Analysis Summary
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| } catch (...) { | ||
| std::cerr << "Error: unknown" << std::endl; | ||
| LINYAPS_BOX_ERR() << "unknown error"; | ||
| return -1; |
There was a problem hiding this comment.
The error handling for unknown exceptions logs a very generic message, which might not be helpful for diagnosing issues. Consider logging more context-specific information if possible, or at least include a suggestion to check the logs for more details. This could help in troubleshooting and maintaining the application more effectively.
Suggested Improvement:
LINYAPS_BOX_ERR() << "Unknown error occurred. Please check the logs for more details.";| continue; | ||
| } | ||
| if (auto it = propagation_flags_map.find(opt); it != propagation_flags_map.end()) { | ||
| propagation_flags &= it->second; | ||
| propagation_flags |= it->second; | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
The repeated pattern of checking for an option in a map and then setting a flag is a candidate for refactoring to reduce code duplication and improve maintainability. Consider creating a helper function that takes the option, the map, and a reference to the flag variable. This function would encapsulate the logic found in lines 79-85, making the code cleaner and easier to maintain.
| linux.readonly_paths = std::move(readonly_paths); | ||
| } | ||
|
|
||
| if (auto rootfs_propagation = ptr / "rootfsPropagation"; obj.contains(rootfs_propagation)) { | ||
| auto val = obj[rootfs_propagation].get<std::string>(); | ||
| if (val == "shared") { | ||
| linux.rootfs_propagation = MS_SHARED; | ||
| } else if (val == "slave") { | ||
| linux.rootfs_propagation = MS_SLAVE; | ||
| } else if (val == "private") { | ||
| linux.rootfs_propagation = MS_PRIVATE; | ||
| } else if (val == "unbindable") { | ||
| linux.rootfs_propagation = MS_UNBINDABLE; | ||
| } else { | ||
| throw std::runtime_error("unsupported rootfs propagation: " + val); | ||
| } | ||
| } | ||
|
|
||
| return linux; | ||
| } | ||
|
|
There was a problem hiding this comment.
The handling of rootfsPropagation using multiple if statements (lines 266-276) could be simplified and made more maintainable by using a map to associate string values with constants. This would reduce the complexity of the code and make it easier to add support for new propagation types in the future.
| std::optional<std::vector<id_mapping_t>> gid_mappings; | ||
| std::optional<std::vector<std::filesystem::path>> masked_paths; | ||
| std::optional<std::vector<std::filesystem::path>> readonly_paths; |
There was a problem hiding this comment.
The use of std::optional for std::vector types in lines 123-125 is unnecessary. A std::vector can be empty by default, which effectively represents 'no elements', making std::optional redundant. This not only simplifies the code but also avoids the overhead of checking whether the optional has a value.
Recommended Change:
Remove std::optional from gid_mappings, masked_paths, and readonly_paths, and handle them as regular vectors that can be empty if no elements are present.
| std::optional<std::vector<id_mapping_t>> gid_mappings; | ||
| std::optional<std::vector<std::filesystem::path>> masked_paths; | ||
| std::optional<std::vector<std::filesystem::path>> readonly_paths; | ||
| unsigned int rootfs_propagation{ 0 }; |
There was a problem hiding this comment.
The rootfs_propagation field is initialized to zero without clear documentation or named constants, which can lead to confusion about the meaning of different values. Using named constants or an enum would improve the readability and maintainability of the code by making the mount propagation settings more explicit.
Recommended Change:
Define an enum or named constants that represent valid mount propagation settings and use these in place of the plain integer for rootfs_propagation.
1 participant
Allows control over mount propagation for the root filesystem according to OCI spec.