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

Add mount2 syscall #1024

Merged
merged 5 commits into from
Mar 19, 2024
Merged

Add mount2 syscall #1024

merged 5 commits into from
Mar 19, 2024

Conversation

morr0ne
Copy link
Contributor

@morr0ne morr0ne commented Feb 28, 2024

Adds a mount2 syscall.

This is actually the same as the current mount syscall. The main difference being the function parameters which are arguably wrong in the original implementation since it did not account for certain values being null.

This pr is incomplete since the parameters have been changed from Arg to &CStr. This is not inline with other apis and should be fixed but I could not figure out how to convert the types properly. I temporarily used a different type because I needed to use the function. Help fixing the parameters would be appreciated.

@morr0ne morr0ne marked this pull request as draft February 28, 2024 21:20
@sunfishcode
Copy link
Member

To fix the Arg types, would it work to add something like this to src/path/arg.rs and then use that in place of .into_with_c_str?

pub fn option_into_with_c_str<T, F, A: Arg>(a: Option<A>, f: F) -> io::Result<T>
where
    A: Sized,
    F: FnOnce(Option<&CStr>) -> io::Result<T>,
{
    if let Some(a) = a {
        a.into_with_c_str(|p| f(Some(p)))
    } else {
        f(None)
    }
}

@morr0ne
Copy link
Contributor Author

morr0ne commented Mar 1, 2024

It should work I believe. I was thinking of something similar but I wanted to wait for other opinions first

@morr0ne morr0ne marked this pull request as ready for review March 1, 2024 08:30
@morr0ne
Copy link
Contributor Author

morr0ne commented Mar 1, 2024

While testing I realized that that data should not be an Arg, the data is interpreted differently based on the filesystem being mounted so it can't be generalized to anything "Path-like". I changed it to a CStr witch is more inline with the syscall api

@sunfishcode sunfishcode merged commit 4c3335c into bytecodealliance:main Mar 19, 2024
43 checks passed
@SUPERCILEX
Copy link
Contributor

Sorry for resurrecting an old PR, but I don't think this is correct. When are these parameters null during a mount? @morr0ne I think you misunderstood the API. If you want source to be null for example, you should be using https://docs.rs/rustix/latest/rustix/mount/fn.mount_remount.html. Though I guess maybe it's fine to have a "raw" version of the API.

@morr0ne
Copy link
Contributor Author

morr0ne commented Aug 15, 2024

@SUPERCILEX

It has been quite a while since I opened this pr so I do not properly remember what my needs were. I believe this is what I needed this for: https://github.com/morr0ne/woven/blob/48e5e68c736fb7578c58fb847cb4e00dd73aba2e/system/src/raminit.rs#L86

@SUPERCILEX
Copy link
Contributor

Wouldn't the empty string work?

@morr0ne
Copy link
Contributor Author

morr0ne commented Aug 15, 2024

Wouldn't the empty string work?

It might. I followed examples of how c programs used the api and the manual pages. Both pointed to using null and it does work

@SUPERCILEX
Copy link
Contributor

Alrighty, I've submitted #1112 to deprecate that API while also allow passing in null for options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants