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

Fixes raw_open and microtime on windows #772

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

SuchAFuriousDeath
Copy link
Contributor

No description provided.

@SuchAFuriousDeath SuchAFuriousDeath requested a review from a team March 26, 2024 21:26
@github-actions github-actions bot added the S-ready Status: Ready for review label Mar 26, 2024
@SuchAFuriousDeath SuchAFuriousDeath changed the title Fixes raw_open on windows Fixes raw_open and microtime on windows Mar 26, 2024
Copy link
Member

@VocalFan VocalFan left a comment

Choose a reason for hiding this comment

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

LGTM! (Also nice fix for the microtime :P)

@VocalFan VocalFan merged commit 2ca7ebe into obhq:main Mar 27, 2024
5 checks passed
@ultimaweapon
Copy link
Member

@VocalFan have you understand what the code does before approving? As I said before that the rule of thumb is do not approve if you don't understand what the code does.

@VocalFan
Copy link
Member

Boy, I may be dumb in the deeper things... But we BOTH dived into this code before.

@ultimaweapon
Copy link
Member

If you understand then can you explain #773?

@VocalFan
Copy link
Member

If you understand then can you explain #773?

Yep... File_OPEN only reads and opens the file if available, and errors if not available... Though normally, the /dev and /mnt folders already exist, you would... Want a fallback where if it DOES error, then create it with File_CREATE. Thing is it seems we have to do it in this order as File_CREATE just throws an error if the file does already exist.

@VocalFan
Copy link
Member

If you understand then can you explain #773?

Hey, your implementation was broken too, so you can't be mad when we go from a non-working fix to a half-working fix that... If the folders already exist... We reach

    pub fn open(&self, path: impl AsRef<VPath>, td: Option<&VThread>) -> Result<VFile, OpenError> {
        let vnode = self
            .lookup(path, true, td)
            .map_err(OpenError::LookupFailed)?;

        todo!();
    }

@ultimaweapon
Copy link
Member

If you understand then can you explain #773?

Yep... File_OPEN only reads and opens the file if available, and errors if not available... Though normally, the /dev and /mnt folders already exist, you would... Want a fallback where if it DOES error, then create it with File_CREATE. Thing is it seems we have to do it in this order as File_CREATE just throws an error if the file does already exist.

https://github.com/obhq/obliteration/blob/main/src/kernel/src/fs/mod.rs#L111-L127

And raw_mkdir need to behave in the same way for both Windows and *nix, which is already correct before this PR. Next time please don't approve any PRs that has any changes on how the code work. You still can approve the other PRs like display string updates, grammar fix, etc.

@SuchAFuriousDeath
Copy link
Contributor Author

This is actually something I was unsure about and wanted to figure out during the review process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ready Status: Ready for review
Development

Successfully merging this pull request may close these issues.

3 participants