Skip to content

add support for mknodat #751

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

add support for mknodat #751

wants to merge 1 commit into from

Conversation

Mic92
Copy link
Contributor

@Mic92 Mic92 commented Sep 1, 2017

No description provided.

@Mic92 Mic92 changed the title add support for mknodat [WIP] add support for mknodat Sep 1, 2017
src/sys/stat.rs Outdated
@@ -50,6 +50,19 @@ pub fn mknod<P: ?Sized + NixPath>(path: &P, kind: SFlag, perm: Mode, dev: dev_t)
Errno::result(res).map(drop)
}

/// Create a special or ordinary file
/// ([see mknodat(2)](http://man7.org/linux/man-pages/man2/mknodat.2.html)).
Copy link
Member

Choose a reason for hiding this comment

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

Better to reference POSIX docs than Linux docs
http://pubs.opengroup.org/onlinepubs/9699919799/functions/mknod.html

Copy link
Contributor Author

@Mic92 Mic92 Sep 1, 2017

Choose a reason for hiding this comment

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

The POSIX one is not super helpful for end-users. I would rather include no manpage link, then link to the posix one (example: http://man7.org/linux/man-pages/man2/mknodat.2.html#NOTES)

Copy link
Member

Choose a reason for hiding this comment

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

nix is a portable crate, not a Linux crate. The Linux manpage is only useful for Linux users. But the POSIX man page can be relied upon by all nix users.

Copy link
Contributor Author

@Mic92 Mic92 Sep 1, 2017

Choose a reason for hiding this comment

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

But POSIX deviate from reality in subtle ways (hence the use of shall all over the place). Nix is not fully portable anyway, since it only provides a thin layer above systems libc unlike projects like gnulib. So you have to actually look at every manpage of every operating system you care about to be sure, it won't break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be actually be the more honest documentation: https://www.gnu.org/software/gnulib/manual/gnulib.html#mknodat

Copy link
Member

Choose a reason for hiding this comment

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

That's irrelevant, because nix doesn't bind to gnulib; it binds to the system's C library. The POSIX documentation is the GCD of all supported platforms. While some platforms may deviate, the POSIX docs are still the single most relevant source.

@Mic92 Mic92 changed the title [WIP] add support for mknodat add support for mknodat Sep 1, 2017
@asomers
Copy link
Member

asomers commented Sep 3, 2017

Thanks for changing the man page link. Next, could you try to add a test for mknod and mknodat? Perhaps creating a fifo and then stating it?

@Susurrus
Copy link
Contributor

Susurrus commented Sep 4, 2017

We would like to write tests/examples for all additions to nix if possible. While having tests that don't require root would be ideal, I think having an example in a doc test that isn't run and a test that requires root would be sufficient coverage.

@Mic92
Copy link
Contributor Author

Mic92 commented Sep 4, 2017

I already added the test for FIFO.

@Susurrus
Copy link
Contributor

Susurrus commented Sep 5, 2017

I just read up on mknodat and it follows the common theme for many POSIX APIs where there are 2 function arguments that are really 3 separate states and not all combinations of these variables are valid. For example, if pathname is absolute, dirfd is ignored. I think we should use an enum type here to encapsulate these 3 types instead of continuing to allow some partially-valid types. Instead of dirfd and path, I'd like just path which is an enum with AbsPath(&P), RelCurDir(&P), and RelTo(&P, &RawFd) or something like that. I think there was a previous issue where I suggested this approach as well, but I can't find it now. The only issue here is where/if we should verify the path is absolute or relative in the code to generate those enum types. But since this is a convention that pops up a lot, I'd like us to have a good API for working with it.

@asomers Any thoughts on this?

@Mic92
Copy link
Contributor Author

Mic92 commented Sep 5, 2017

Hi, sorry but I have no time on polishing the API further.
If you think you want to this feature, just do:

git push [email protected]:Mic92/nix mknodat

(force-push is also possible just in case).

@Mic92
Copy link
Contributor Author

Mic92 commented Oct 3, 2017

I close this pull request in case I block somebody else who wants to implement this feature.
Feel free to pick whatever you think is necessary. I also implement some other features like setrlimit/getrlimit or transparent large file support on this branch: https://github.com/Mic92/nix/commits/cntrfs

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