Skip to content
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

Replace OFlag & Mode with nix-independent type #389

Merged
merged 17 commits into from
Jul 27, 2024
Merged

Replace OFlag & Mode with nix-independent type #389

merged 17 commits into from
Jul 27, 2024

Conversation

magicant
Copy link
Owner

@magicant magicant commented Jul 23, 2024

This is part of #353.

The fcntl_setfl and fcntl_getfl functions are removed in favor of the
new functions get_and_set_nonblocking and ofd_access.
We cannot support the fcntl_setfl function because its return value may
need to contain platform-specific flags that cannot be represented in a
portable type.

The Mode type is replaced with a new homebrew type.

  • Remove fcntl_setfl
  • Define nix-independent flag types
  • Define Mode2 named constants
  • Define System::open2
  • Migrate uses of System::open to System::open2
  • Remove System::open
  • Rename System::open2 to System::open
  • Replace use of fcntl_getfl with a new function that returns access mode of open file description
  • Remove fcntl_getfl
  • Use new Mode in System::umask
  • Remove Mode
  • Rename Mode2 to Mode

Summary by CodeRabbit

  • New Features

    • Enhanced file permission management and access modes using new structured types.
    • Introduced a comprehensive file system module for better directory and file operations.
    • Added non-blocking file handling capabilities for improved responsiveness.
    • New methods for translating file access flags between shell-level and system-specific representations.
  • Bug Fixes

    • Improved clarity and robustness in file handling logic, reducing the likelihood of permission errors.
  • Documentation

    • Added detailed comments and explanations for new types and methods to enhance usability.
  • Chores

    • Updated dependencies to include additional libraries for enhanced functionality.

@magicant magicant added this to the 0.1.0 β3 milestone Jul 23, 2024
@magicant magicant self-assigned this Jul 23, 2024
Copy link

coderabbitai bot commented Jul 23, 2024

Walkthrough

The recent updates in the yash project enhance file handling by replacing outdated flag-based approaches with structured, type-safe enumerations for file access permissions. This transition improves code clarity and maintainability while introducing robust error handling. The reorganization of file system operations results in a more efficient and user-friendly shell environment, fostering better interactions with file descriptors.

Changes

Files Change Summary
yash-builtin/src/exec.rs Updated file permission handling using Mode for clarity; replaced bitwise operations with explicit method calls.
yash-builtin/src/source/semantics.rs Switched from OFlag to OfdAccess and OpenFlag in open_file, enhancing type safety and clarity in file access modes.
yash-cli/src/startup/init_file.rs Refactored file opening logic to use OpenFlag and OfdAccess, improving file access clarity and functionality.
yash-cli/src/startup/input.rs Changed input handling to non-blocking mode; updated file opening with new type-safe flags.
yash-env/CHANGELOG.md Documented changes in the system module, introduced new types, methods, and structural changes for improved usability.
yash-env/src/input/fd_reader.rs Refactored open method to accept OfdAccess and OpenFlag, enhancing file opening granularity.
yash-env/src/system.rs Enhanced file system functionality with new methods; updated open to use structured flags for better access management.
yash-env/src/system/file_system.rs Added DEFAULT_DIRECTORY_MODE; refactored permission management to use structured types for clarity.
yash-semantics/src/command/item.rs Updated nullify_stdin to use OfdAccess, improving clarity in file descriptor handling.
yash-semantics/src/redir.rs Refactored file opening methods to utilize new access types (OfdAccess, OpenFlag), enhancing validation and error handling in file operations.
yash-env/src/system/real/open_flag.rs Added methods to OfdAccess and OpenFlag enums for better mapping to real system flags, improving interoperability with underlying file operations.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Shell
    participant System

    User->>Shell: Request file operation
    activate Shell
    Shell->>System: Open file with OfdAccess
    activate System
    System-->>Shell: Return file descriptor
    deactivate System
    Shell-->>User: Confirm operation success
    deactivate Shell
Loading

🐇 In the meadow where bunnies play,
New changes hop in, brightening the day!
With clearer paths and flags so neat,
File handling's now a joyful feat!
So let us celebrate with a cheerful cheer,
For a better shell is finally here! 🌼✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@magicant magicant mentioned this pull request Jul 23, 2024
17 tasks
This commit migrates the System::open method to the new System::open2
method. In upcoming changes, the System::open method will be removed and
the System::open2 method will be renamed to System::open.
Before renaming the new open2 method to open, we need to remove the
existing open method. This commit removes the deprecated open method.
This method returns the open file description access mode. This method
will replace the fcntl_getfl method.
This method is removed because it depends on the OFlag type, which is
re-exported from the nix crate. To make our public API independent of
the nix crate, we remove this method.
@magicant magicant changed the title Replace OFlag with nix-independent type Replace OFlag & Mode with nix-independent type Jul 27, 2024
Before renaming the new Mode type, we need to remove the existing one.
@magicant magicant marked this pull request as ready for review July 27, 2024 04:26
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Outside diff range, codebase verification and nitpick comments (1)
yash-env/src/system/shared.rs (1)

Discrepancy in get_and_set_nonblocking implementation

The get_and_set_nonblocking method in system/real.rs correctly sets and restores the non-blocking state using fcntl system calls. However, the implementation in system/virtual.rs is currently a placeholder that always returns false and does not actually set or restore the non-blocking state.

  • system/real.rs implementation (correct):

    fn get_and_set_nonblocking(&mut self, fd: Fd, nonblocking: bool) -> Result<bool> {
        let old_flags = unsafe { nix::libc::fcntl(fd.0, nix::libc::F_GETFL) }.errno_if_m1()?;
        let new_flags = if nonblocking {
            old_flags | nix::libc::O_NONBLOCK
        } else {
            old_flags & !nix::libc::O_NONBLOCK
        };
        if new_flags != old_flags {
            unsafe { nix::libc::fcntl(fd.0, nix::libc::F_SETFL, new_flags) }.errno_if_m1()?;
        }
        let was_nonblocking = old_flags & nix::libc::O_NONBLOCK != 0;
        Ok(was_nonblocking)
    }
  • system/virtual.rs implementation (placeholder):

    fn get_and_set_nonblocking(&mut self, fd: Fd, _nonblocking: bool) -> Result<bool> {
        self.with_open_file_description_mut(fd, |_ofd| {
            // TODO Implement non-blocking I/O
            Ok(false)
        })
    }

Please ensure that the get_and_set_nonblocking method in system/virtual.rs is implemented to provide consistent behavior across different system implementations.

Analysis chain

Line range hint 139-160:
Verify the correctness of get_and_set_nonblocking integration.

Ensure that the new method correctly sets and restores the non-blocking state.


To verify the correctness of the get_and_set_nonblocking integration, we need to review the implementation of this method in system/real.rs and system/virtual.rs, and ensure that its usage in shared.rs correctly sets and restores the non-blocking state.

Once we have the implementations, we can analyze them to ensure they correctly set and restore the non-blocking state.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of `get_and_set_nonblocking` integration.

# Test: Search for the function usage. Expect: Correct integration of the new method.
rg --type rust -A 5 $'get_and_set_nonblocking'

Length of output: 4577


Script:

#!/bin/bash
# Description: Verify the implementation of `get_and_set_nonblocking` in `system/real.rs` and `system/virtual.rs`.

# Extract the implementation of `get_and_set_nonblocking` in `system/real.rs`.
ast-grep --lang rust --pattern 'fn get_and_set_nonblocking(&mut self, fd: Fd, nonblocking: bool) -> Result<bool> { $$$ }' yash-env/src/system/real.rs

# Extract the implementation of `get_and_set_nonblocking` in `system/virtual.rs`.
ast-grep --lang rust --pattern 'fn get_and_set_nonblocking(&mut self, fd: Fd, _nonblocking: bool) -> Result<bool> { $$$ }' yash-env/src/system/virtual.rs

Length of output: 1750

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a78c24e and 9f6f5c3.

Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing (22)
  • yash-builtin/src/exec.rs (5 hunks)
  • yash-builtin/src/source/semantics.rs (2 hunks)
  • yash-cli/src/startup/init_file.rs (2 hunks)
  • yash-cli/src/startup/input.rs (3 hunks)
  • yash-env/CHANGELOG.md (1 hunks)
  • yash-env/Cargo.toml (1 hunks)
  • yash-env/src/input/fd_reader.rs (2 hunks)
  • yash-env/src/lib.rs (1 hunks)
  • yash-env/src/system.rs (9 hunks)
  • yash-env/src/system/file_system.rs (1 hunks)
  • yash-env/src/system/id.rs (2 hunks)
  • yash-env/src/system/open_flag.rs (1 hunks)
  • yash-env/src/system/real.rs (6 hunks)
  • yash-env/src/system/real/open_flag.rs (1 hunks)
  • yash-env/src/system/shared.rs (9 hunks)
  • yash-env/src/system/virtual.rs (33 hunks)
  • yash-env/src/system/virtual/file_system.rs (8 hunks)
  • yash-semantics/Cargo.toml (1 hunks)
  • yash-semantics/src/command/item.rs (2 hunks)
  • yash-semantics/src/command/simple_command/external.rs (7 hunks)
  • yash-semantics/src/expansion/glob.rs (2 hunks)
  • yash-semantics/src/redir.rs (10 hunks)
Files skipped from review due to trivial changes (1)
  • yash-env/src/system/id.rs
Additional comments not posted (84)
yash-semantics/Cargo.toml (1)

18-18: Addition of enumset dependency is appropriate.

The enumset crate at version 1.1.2 is correctly added to handle sets of enum values efficiently. Ensure that this dependency is utilized properly in the codebase.

yash-env/Cargo.toml (1)

19-19: Addition of bitflags dependency is appropriate.

The bitflags crate at version 2.6.0 is correctly added to handle sets of flags in a type-safe manner. Ensure that this dependency is utilized properly in the codebase.

yash-env/src/system/open_flag.rs (3)

1-20: License information and imports are appropriate.

The file includes the correct GNU General Public License information and imports the enumset crate, which is necessary for the functionality provided in this file.


21-35: OfdAccess enum is well-defined.

The OfdAccess enum correctly defines various access modes for open file descriptions, covering all necessary modes.


37-62: OpenFlag enum is well-defined and uses EnumSetType appropriately.

The OpenFlag enum correctly defines various options for opening file descriptors and uses the EnumSetType derive macro for efficient handling of enum sets.

yash-cli/src/startup/input.rs (3)

37-37: Import OfdAccess and OpenFlag.

The new imports OfdAccess and OpenFlag are necessary for the updated file opening mechanism.


93-93: Ensure non-blocking state is correctly set.

The change to non-blocking reads enhances responsiveness. Ensure that the non-blocking state is correctly set for all relevant file descriptors.


114-119: Update file opening mechanism.

The new file opening mechanism using OfdAccess::ReadOnly and OpenFlag::Cloexec improves semantic clarity. Ensure that the new mechanism is compatible with all intended use cases.

yash-env/src/system/file_system.rs (8)

25-28: LGTM! Constants are correctly defined.

The constants RAW_AT_FDCWD are correctly defined for Unix and non-Unix platforms.


36-44: LGTM! DirEntry struct is correctly defined.

The DirEntry struct is straightforward and correctly defined.


46-54: LGTM! Dir trait is correctly defined.

The Dir trait is straightforward and correctly defined.


56-72: Define RawMode type alias.

The RawMode type alias is correctly defined for Unix and non-Unix platforms.


74-82: Define Mode struct.

The Mode struct is correctly defined, implementing the new type pattern for better type safety.


83-128: Define Mode bitflags.

The Mode bitflags are correctly defined, providing a comprehensive set of file permission bits.


130-134: Implement Debug for Mode.

The Debug implementation for Mode is correctly defined, providing a clear and concise representation.


136-141: Implement Default for Mode.

The Default implementation for Mode is correctly defined, setting the default mode to 0o644.

yash-env/CHANGELOG.md (3)

12-26: LGTM!

The additions accurately reflect the new types, methods, and dependencies introduced in the codebase.


27-34: LGTM!

The changes accurately reflect the modifications made to the existing types and methods in the codebase.


35-42: LGTM!

The deprecated and removed items accurately reflect the changes made in the codebase.

yash-builtin/src/source/semantics.rs (1)

120-125: LGTM!

The changes to the open_file function improve the clarity and type safety by replacing OFlag with OfdAccess and OpenFlag.

yash-env/src/input/fd_reader.rs (1)

223-229: LGTM!

The changes to the open method improve the granularity of file opening options by replacing OFlag with OfdAccess and OpenFlag.

yash-builtin/src/exec.rs (5)

145-145: Import statement for Mode is appropriate.

The import statement is necessary for the new method call to set permissions.


168-168: Improved permission setting method call.

The new method call content.permissions.set(Mode::USER_EXEC, true) enhances readability and maintainability.


203-203: Improved permission setting method call.

The new method call content.permissions.set(Mode::USER_EXEC, true) enhances readability and maintainability.


238-238: Improved permission setting method call.

The new method call content.permissions.set(Mode::USER_EXEC, true) enhances readability and maintainability.


268-268: Improved permission setting method call.

The new method call content.permissions.set(Mode::USER_EXEC, true) enhances readability and maintainability.

yash-cli/src/startup/init_file.rs (2)

41-41: Updated import statement is appropriate.

The import statement has been updated to replace OFlag with OfdAccess and OpenFlag, which is necessary for the new types used in the open_fd function.


149-154: Updated open_fd function enhances clarity.

The function now uses OfdAccess::ReadOnly and OpenFlag::Cloexec.into(), which enhances the clarity and intent of the code by explicitly defining the access type and flags.

yash-semantics/src/command/item.rs (2)

33-33: Import statement for OfdAccess is appropriate.

The import statement is necessary for the new type used in the nullify_stdin function.


119-121: Updated nullify_stdin function enhances clarity.

The function now uses OfdAccess::ReadOnly, which enhances the clarity and intent of the code by explicitly defining the access type.

yash-env/src/system/virtual/file_system.rs (6)

33-33: LGTM! Centralized default directory mode enhances maintainability.

The introduction of DEFAULT_DIRECTORY_MODE improves code clarity and maintainability by centralizing the definition of default permissions.


50-50: LGTM! Consistent application of default directory permissions.

Using DEFAULT_DIRECTORY_MODE in the FileSystem default implementation ensures consistent application of default permissions across the codebase.


112-112: LGTM! Adherence to default directory permissions.

The save method now uses DEFAULT_DIRECTORY_MODE, ensuring that new directories adhere to the defined default permissions.


155-155: LGTM! Improved readability of permission check.

The permission check in the get method now uses node_ref.permissions.contains(Mode::USER_EXEC), improving code readability.


197-223: LGTM! Enhanced documentation and support for named pipes.

The new documentation comments clarify the purpose of each FileBody variant, and the Fifo variant adds support for named pipes.


254-257: LGTM! Promoted consistency in permission handling.

The deprecation of the Mode struct in favor of a type alias encourages the use of yash_env::system::Mode, promoting consistency and maintainability.

yash-semantics/src/command/simple_command/external.rs (6)

237-237: LGTM! Necessary import for Mode type.

The import statement for Mode has been added to the test module, which is necessary for the test cases to compile and run correctly.


253-253: LGTM! Improved readability in setting permissions.

The test case now uses content.permissions.set(Mode::USER_EXEC, true), improving readability and maintainability.


297-297: LGTM! Improved readability in setting permissions.

The test case now uses content.permissions.set(Mode::USER_EXEC, true), improving readability and maintainability.


323-323: LGTM! Improved readability in setting permissions.

The test case now uses content.permissions.set(Mode::USER_EXEC, true), improving readability and maintainability.


352-352: LGTM! Improved readability in setting permissions.

The test case now uses content.permissions.set(Mode::USER_EXEC, true), improving readability and maintainability.


425-425: LGTM! Improved readability in setting permissions.

The test case now uses content.permissions.set(Mode::USER_EXEC, true), improving readability and maintainability.

yash-semantics/src/expansion/glob.rs (1)

297-297: LGTM! Necessary import for Mode type.

The import statement for Mode has been added to the test module, which is necessary for the test cases to compile and run correctly.

yash-env/src/system/real.rs (4)

274-276: LGTM!

The ofd_access function improves specificity and focuses on access control.


279-290: LGTM!

The get_and_set_nonblocking function provides explicit control over non-blocking behavior.


351-352: LGTM!

The umask function changes ensure that the returned value accurately reflects the new mask set by the call.


231-250: LGTM! But verify the function usage in the codebase.

The refactored open function enhances flexibility and type safety.

However, ensure that all function calls to open match the new signature.

Verification successful

LGTM! But verify the function usage in the codebase.

The refactored open function enhances flexibility and type safety.

However, ensure that all function calls to open match the new signature.


All instances of the open function match the new signature.

The refactored open function has been correctly applied across the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `open` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type rust -A 5 $'open'

Length of output: 179215


Script:

#!/bin/bash
# Description: Verify all function calls to `open` match the new signature.

# Search for the function usage and check for the new signature.
rg --type rust -A 5 'open\(' | grep -E 'open\([^,]+, OfdAccess, EnumSet<OpenFlag>, Mode\)'

Length of output: 92


Script:

#!/bin/bash
# Description: Verify all function calls to `open` match the new signature.

# Search for all instances of the open function.
rg --type rust 'open\(' -A 5

Length of output: 15976

yash-env/src/system.rs (4)

156-157: LGTM!

The ofd_access function improves specificity and focuses on access control.


159-164: LGTM!

The get_and_set_nonblocking function provides explicit control over non-blocking behavior.


217-217: LGTM!

The umask function changes ensure that the returned value accurately reflects the new mask set by the call.


135-141: LGTM! But verify the function usage in the codebase.

The refactored open function enhances flexibility and type safety.

However, ensure that all function calls to open match the new signature.

yash-env/src/lib.rs (1)

298-299: LGTM!

The get_tty function changes enhance clarity and maintainability by adopting a more explicit representation of access modes.

yash-env/src/system/shared.rs (4)

344-345: Verify the correctness of get_and_set_nonblocking integration.

Ensure that the new function correctly sets and restores the non-blocking state.


Line range hint 178-207:
Verify the correctness of get_and_set_nonblocking integration.

Ensure that the new method correctly sets and restores the non-blocking state.


341-342: Verify the correctness of ofd_access integration.

Ensure that the new function correctly retrieves the access mode of the file descriptor.

Verification successful

Verify the correctness of ofd_access integration.

The ofd_access function is correctly integrated and retrieves the access mode of the file descriptor as expected in both real.rs and virtual.rs.

  • real.rs: Uses fcntl system call to get file status flags.
  • virtual.rs: Uses internal logic to determine access mode based on readability and writability.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of `ofd_access` integration.

# Test: Search for the function usage. Expect: Correct integration of the new function.
rg --type rust -A 5 $'ofd_access'

Length of output: 2747


Script:

#!/bin/bash
# Extract the full implementation of the `ofd_access` function in `real.rs`
ast-grep --lang rust --pattern $'fn ofd_access(&self, fd: Fd) -> Result<OfdAccess> {
  $$$
}' yash-env/src/system/real.rs

# Extract the full implementation of the `ofd_access` function in `virtual.rs`
ast-grep --lang rust --pattern $'fn ofd_access(&self, fd: Fd) -> Result<OfdAccess> {
  $$$
}' yash-env/src/system/virtual.rs

Length of output: 1894


326-333: Verify the correctness of OfdAccess and EnumSet<OpenFlag> integration.

Ensure that the new parameters are correctly integrated and that the logic remains consistent.

yash-semantics/src/redir.rs (4)

Line range hint 357-394:
Verify the correctness of OfdAccess integration.

Ensure that the new parameter is correctly integrated and that the logic remains consistent.

Verification successful

Verify the correctness of OfdAccess integration.

The OfdAccess parameter is correctly integrated into the copy_fd function and its usage within the same file. The logic and error handling related to OfdAccess appear consistent.

  • copy_fd function definition: yash-semantics/src/redir.rs lines 357-394
  • Function calls with OfdAccess::ReadOnly and OfdAccess::WriteOnly: yash-semantics/src/redir.rs lines 372-397
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of `OfdAccess` integration.

# Test: Search for the function usage. Expect: Correct integration of the new parameter.
rg --type rust -A 5 $'copy_fd'

Length of output: 804


281-289: Verify the correctness of OfdAccess and EnumSet<OpenFlag> integration.

Ensure that the new parameters are correctly integrated and that the logic remains consistent.


416-431: Verify the correctness of OfdAccess and EnumSet<OpenFlag> integration.

Ensure that the new parameters are correctly integrated and that the logic remains consistent.

Verification successful

Verify the integration of OfdAccess and EnumSet<OpenFlag> in open_normal function.

Ensure that the new parameters are correctly integrated and that the logic remains consistent.


The integration of OfdAccess and EnumSet<OpenFlag> in open_normal and related functions is correct and consistent.

  • open_file correctly uses OfdAccess and EnumSet<OpenFlag>.
  • open_file_noclobber and copy_fd also utilize OfdAccess appropriately.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of `OfdAccess` and `EnumSet<OpenFlag>` integration.

# Test: Search for the function usage. Expect: Correct integration of the new parameters.
rg --type rust -A 5 $'open_normal'

Length of output: 821


Script:

#!/bin/bash
# Description: Verify the implementation details of `open_normal` and the functions it calls.

# Search for the definition and usage of `open_file`, `open_file_noclobber`, and `copy_fd` functions.
rg --type rust -A 5 'fn open_file'
rg --type rust -A 5 'fn open_file_noclobber'
rg --type rust -A 5 'fn copy_fd'

Length of output: 3153


Line range hint 300-323:
Verify the correctness of EnumSet<OpenFlag> integration.

Ensure that the new parameter is correctly integrated and that the logic remains consistent.

Verification successful

Verify the correctness of EnumSet<OpenFlag> integration.

The integration of EnumSet<OpenFlag> in the function open_file_noclobber appears correct. The function is only called in one place, and the logic at this call site is consistent with the intended use of EnumSet<OpenFlag>.

  • yash-semantics/src/redir.rs: Line 313-326
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of `EnumSet<OpenFlag>` integration.

# Test: Search for the function usage. Expect: Correct integration of the new parameter.
rg --type rust -A 5 $'open_file_noclobber'

Length of output: 989


Script:

#!/bin/bash
# Description: Search for all instances of `open_file_noclobber` being called in the codebase.

# Test: Search for function calls. Expect: Consistent logic and correct integration of `EnumSet<OpenFlag>`.
rg --type rust 'open_file_noclobber'

Length of output: 271

yash-env/src/system/virtual.rs (24)

28-30: Documentation Update Approved.

The documentation now accurately reflects the support for named pipes and symbolic links in the virtual file system.


66-67: New Types Imported Approved.

The new types OfdAccess and OpenFlag have been imported, which will likely improve type safety and expressiveness.


80-80: EnumSet Import Approved.

The enumset::EnumSet type has been imported, which is useful for handling sets of flags.


182-182: Use of Mode::ALL_9 Approved.

The /tmp directory is set with full permissions using Mode::ALL_9, aligning with the new type-safe approach.


324-324: Use of Mode Type in stat Function Approved.

The stat function now uses the Mode type to set file permissions, improving clarity and reducing errors.


371-371: Use of Mode Type in is_executable_file Function Approved.

The is_executable_file function now uses the Mode type to check for executable permissions, aligning with the new type-safe approach.


440-446: Updated open Function Signature Approved.

The open function signature now uses OfdAccess, EnumSet<OpenFlag>, and Mode, refining file access mode specifications for better expressiveness and type safety.


451-454: Open Function Flag Checks Approved.

The open function now correctly checks for the Exclusive and Directory flags using the new OpenFlag type, adhering to the type-safe approach.


466-469: Use of Mode Type for Setting File Permissions Approved.

The open function now sets file permissions using the Mode type when creating a new file, aligning with the type-safe approach.


477-481: Use of OfdAccess Type in Open Function Approved.

The open function now uses the OfdAccess type to determine file access modes, improving clarity and type safety.


501-501: Use of OpenFlag Type for Setting is_appending Flag Approved.

The open function now sets the is_appending flag using the OpenFlag type, ensuring correct handling of the append mode.


505-505: Use of OpenFlag Type for Setting flag Field Approved.

The open function now sets the flag field using the OpenFlag type, aligning with the type-safe approach.


538-552: New ofd_access Function Approved.

The ofd_access function replaces the fcntl_getfl function and uses the OfdAccess type, improving the structured determination of access modes for open file descriptors.


620-622: Updated opendir Function Approved.

The opendir function now uses the OfdAccess and OpenFlag types, aligning with the type-safe approach.


627-628: Updated umask Function Signature Approved.

The umask function signature now uses the Mode type, simplifying the function and aligning with the type-safe approach.


1342-1342: Updated fstatat Test Approved.

The test for fstatat now uses the Mode type, ensuring it correctly reflects the new permission handling logic.


1380-1380: Updated fstatat FIFO Test Approved.

The test for fstatat with a FIFO now uses the Mode type, ensuring it correctly reflects the new permission handling logic.


1411-1411: Updated fstatat Symlink to Regular File Test Approved.

The test for fstatat with a symlink to a regular file now uses the Mode type, ensuring it correctly reflects the new permission handling logic.


1420-1420: Updated fstatat Symlink No Follow Test Approved.

The test for fstatat with a symlink and no follow flag now uses the Mode type, ensuring it correctly reflects the new permission handling logic.


1445-1445: Updated is_executable_file Test Approved.

The test for is_executable_file with an executable file now uses the Mode type, ensuring it correctly reflects the new permission handling logic.


1527-1529: Updated Test for Opening Non-Existing File Without Creation Approved.

The test for opening a non-existing file without creation now uses the OfdAccess and Mode types, ensuring it correctly reflects the new file access and permission handling logic.


1539-1541: Updated Test for Creating and Opening Non-Existing File Approved.

The test for creating and opening a non-existing file now uses the OfdAccess and OpenFlag types, ensuring it correctly reflects the new file access and permission handling logic.


1559-1571: Updated Test for Opening Existing File Approved.

The test for opening an existing file now uses the OfdAccess, OpenFlag, and Mode types, ensuring it correctly reflects the new file access and permission handling logic.


1587-1597: Updated Test for Opening Existing File with Exclusive Flag Approved.

The test for opening an existing file with the Exclusive flag now uses

Comment on lines +22 to +32
impl OfdAccess {
#[must_use]
pub(super) fn to_real_flags(self) -> Option<c_int> {
match self {
Self::ReadOnly => Some(nix::libc::O_RDONLY),
Self::WriteOnly => Some(nix::libc::O_WRONLY),
Self::ReadWrite => Some(nix::libc::O_RDWR),
// TODO Support O_EXEC, O_PATH and O_SEARCH
Self::Exec | Self::Search => None,
}
}
Copy link

Choose a reason for hiding this comment

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

Reminder: Complete support for all flags.

The function to_real_flags does not support O_EXEC, O_PATH, and O_SEARCH.

Would you like me to assist with implementing support for these flags?

Comment on lines +34 to +42
#[must_use]
pub(super) fn from_real_flags(flags: c_int) -> Self {
match flags & nix::libc::O_ACCMODE {
nix::libc::O_RDONLY => Self::ReadOnly,
nix::libc::O_WRONLY => Self::WriteOnly,
nix::libc::O_RDWR => Self::ReadWrite,
_ => Self::Exec, // TODO Support O_PATH and O_SEARCH
}
}
Copy link

Choose a reason for hiding this comment

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

Reminder: Complete support for all flags.

The function from_real_flags does not support O_PATH and O_SEARCH.

Would you like me to assist with implementing support for these flags?

yash-env/src/system/real/open_flag.rs Outdated Show resolved Hide resolved
Comment on lines +557 to +561
fn get_and_set_nonblocking(&mut self, fd: Fd, _nonblocking: bool) -> Result<bool> {
self.with_open_file_description_mut(fd, |_ofd| {
// TODO Implement non-blocking I/O
Ok(false)
})
Copy link

Choose a reason for hiding this comment

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

New get_and_set_nonblocking Function Approved, but Implementation Needed.

The get_and_set_nonblocking function has been introduced but currently returns false. The TODO comment indicates that non-blocking I/O needs to be implemented.

Do you want me to implement the non-blocking I/O functionality or open a GitHub issue to track this task?

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9f6f5c3 and dd139ec.

Files selected for processing (1)
  • yash-env/src/system/real/open_flag.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • yash-env/src/system/real/open_flag.rs

@magicant magicant merged commit aa310ea into master Jul 27, 2024
6 checks passed
@magicant magicant deleted the open-flag branch July 27, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant