Add wrappers for sysconf(3), pathconf(2), and fpathconf(2) #630

asomers · 2017-06-25T00:22:51Z

No description provided.

Susurrus · 2017-06-25T16:00:17Z

I'd prefer to get the missing constants added to libc before merging this since there's a small and clear list of missing constants. Would you be able to do that?

asomers · 2017-06-25T16:14:17Z

Already working on it.

Susurrus · 2017-06-25T16:19:20Z

Awesome, 👍

Susurrus · 2017-06-25T16:19:35Z

And otherwise this PR seems fine to me, though I'd ask that you squash these commits into one.

asomers · 2017-06-25T19:05:27Z

Upstream PR at rust-lang/libc#628

asomers · 2017-06-27T04:14:49Z

All the Travis builds failed with an error like "error: "rustup" "target" "list" failed with exit code: Some(1)". Lame. I'll try forcing a rebuild in the morning.

Susurrus

In general looks pretty good for me, just some docs and a minor code refactoring.

Susurrus · 2017-07-04T18:48:08Z

src/unistd.rs

+///     unsupported (for option variables)
+/// - `Err(x)`: an error occurred
+///
+/// http://pubs.opengroup.org/onlinepubs/9699919799/functions/pathconf.html


It'd be good to give this a proper title for when docs are generated.

Susurrus · 2017-07-04T18:48:45Z

src/unistd.rs

+/// Returns the value of a path-dependent configurable system variable.  Most
+/// supported variables also have associated compile-time constants, but POSIX
+/// allows their values to change at runtime.  There are generally two types of
+/// pathconf variables: options and limits.  See pathconf(3) for more details.


Backticks around pathconf(3).

Susurrus · 2017-07-04T18:49:37Z

src/unistd.rs

+///     unsupported (for option variables)
+/// - Err(x): an error occurred
+///
+/// http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html


Same as above.

Susurrus · 2017-07-04T18:50:26Z

src/unistd.rs

+        Errno::clear();
+        libc::sysconf(var as c_int)
+    };
+    if raw == -1 {


Can you use Errno::result() here to clean up this code a bit?

No, because sysconf will return -1 with errno set to 0 to indicate that the requested variable is not supported. Our Errno::result class can't handle that.

Susurrus · 2017-07-04T18:51:11Z

test/test_unistd.rs

+
+// Test sysconf with an unsupported varible
+// Note: If future versions of FreeBSD add support for this option then the test
+// must be modified.


"should" not "must"

Susurrus · 2017-07-04T18:52:04Z

src/unistd.rs

@@ -819,6 +821,431 @@ pub fn mkstemp<P: ?Sized + NixPath>(template: &P) -> Result<(RawFd, PathBuf)> {
    Ok((fd, PathBuf::from(pathname)))
 }

+/// Variable names for `pathconf`
+// Note: POSIX 1003.1-2008 requires all of these symbols to be


Why not have this note be part of the doc comment? If you put a space between it and the first line, it might be a nice detail to have in the docs.

Susurrus · 2017-07-04T18:52:17Z

src/unistd.rs

+}
+
+/// Variable names for `sysconf`
+// Note: All of these symbols are standardized by POSIX 1003.1-2008, but haven't


Same as above.

Susurrus · 2017-07-09T15:59:49Z

src/unistd.rs

+}
+
+/// Like `pathconf`, but for file descriptors instead of paths
+/// # Parameters


Needs a blank line above.

Susurrus · 2017-07-09T16:00:21Z

src/unistd.rs

+/// Like `pathconf`, but for file descriptors instead of paths
+/// # Parameters
+///
+/// - `fd`: Lookup the value of `var` for this file descriptor


This comment is more of a comment about the entire function then this specific argument.

Susurrus · 2017-07-09T16:01:16Z

src/unistd.rs

+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
+#[repr(i32)]
+pub enum SysconfVar {
+AIO_LISTIO_MAX = libc::_SC_AIO_LISTIO_MAX,


These should all be indented.

Susurrus · 2017-07-09T16:04:12Z

src/unistd.rs

+_POSIX2_UPE = libc::_SC_2_UPE,
+_POSIX2_VERSION = libc::_SC_2_VERSION,
+PAGE_SIZE = libc::_SC_PAGE_SIZE,
+// POSIX requires an alias PAGESIZE == PAGE_SIZE, but Rust doesn't like


It'd be nice if this were able to be documented in the docs for this enum value. Maybe add a doc comment for PAGE_SIZE that says this as well since that way the user could search nix docs and find this instead of having to search the source code.

Susurrus · 2017-07-09T16:04:24Z

src/unistd.rs

+///     unsupported (for option variables)
+/// - Err(x): an error occurred
+///
+/// # Referencces


Misspelled.

Susurrus · 2017-07-09T16:04:38Z

src/unistd.rs

+///     unsupported (for option variables)
+/// - `Err(x)`: an error occurred
+///
+/// # Referencces


Susurrus · 2017-07-09T16:04:45Z

src/unistd.rs

+///     unsupported (for option variables)
+/// - `Err(x)`: an error occurred
+///
+/// # Referencces


Susurrus · 2017-07-09T16:05:12Z

And please squash this into 1 commit in the next round.

Susurrus · 2017-07-10T20:04:54Z

src/unistd.rs

+///
+/// # References
+///
+/// http://pubs.opengroup.org/onlinepubs/9699919799/functions/pathconf.html


Please make these bulleted, as they just show up on a single line when docs are generated. Also if you could make them links, use the title of the manpage as the title for them or providing a summary would be great.

Susurrus · 2017-07-10T20:05:50Z

src/unistd.rs

+/// Returns the value of a configurable system variable.  Most supported
+/// variables also have associated compile-time constants, but POSIX
+/// allows their values to change at runtime.  There are generally two types of
+/// sysconf variables: options and limits.  See sysconf(3) for more details.


Can you turn "sysconf(3)" into a link to the man pages. You could then remove the "References" section at the end.

Susurrus · 2017-07-10T20:06:39Z

src/unistd.rs

+///
+/// # References
+///
+/// http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html


This doesn't turn into a path when you have it like this. Can you linkify it?

Susurrus · 2017-07-10T20:07:19Z

Just some doc comment improvements. I found these by running cargo doc --no-deps --open and looking at the generated docs.

Susurrus · 2017-07-11T13:21:45Z

src/unistd.rs

+/// Returns the value of a path-dependent configurable system variable.  Most
+/// supported variables also have associated compile-time constants, but POSIX
+/// allows their values to change at runtime.  There are generally two types of
+/// `pathconf` variables: options and limits.  See pathconf(2) for more details.


Can you turn pathconf(2) into a link and also put it between backticks?

Susurrus · 2017-07-12T23:02:43Z

src/unistd.rs

+    /// The implementation supports the Software Development Utilities option.
+    _POSIX2_SW_DEV = libc::_SC_2_SW_DEV,
+    /// The implementation supports the User Portability Utilities option.
+    _POSIX2_UPE = libc::_SC_2_UPE,


This shouldn't be defined here, but in libc. Can you upstream these into libc instead of defining them here? I'd like to do this for all of the constants that are missing in libc and then the libc_enum! macro can be used for defining all of these.

In this case, I don't think I can use libc_enum! because the abstract variable names differ from the sysconf symbol names. Unless, that is, I switch naming conventions and simply use the SC forms for nix's public API.

Susurrus · 2017-07-12T23:03:08Z

src/unistd.rs

+    ///
+    /// POSIX also defines an alias named `PAGESIZE`, but Rust does not allow two
+    /// enum constants to have the same value, so nix omits `PAGESIZE`.
+    PAGE_SIZE = libc::_SC_PAGE_SIZE,


Why isn't this named the same thing?

Because "PAGE_SIZE" is the abstract variable, and _SC_PAGE_SIZE is the sysconf variable name.

Susurrus · 2017-07-12T23:03:30Z

src/unistd.rs

+    #[cfg(any(target_os="dragonfly", target_os="freebsd", target_os = "ios",
+              target_os="linux", target_os = "macos", target_os="openbsd"))]
+    /// The implementation supports timeouts.
+    _POSIX_TIMEOUTS = libc::_SC_TIMEOUTS,


These should also be defined in libc and not here.

libc contains all the definitions that it needs. But the situation is fairly confusing. Let me explain:

POSIX says that some "variables" can have values that are either fixed at compile time or changeable at runtime. The variable is an abstract thing; not a C symbol. But there are two C symbols associated with it: one to look it up at build time and one to look it up at runtime via sysconf. In this case, the variable's abstract name is "_POSIX_TIMEOUTS", it's build time symbols is _POSIX_TIMEOUTS, and its runtime symbol is _SC_TIMEOUTS (the "SC" stands for "sysconf"). See http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html . The table's left column has the abstract variable names, and the right column has the sysconf names. I used the abstract names for nix, reasoning that there's no need to namespace them with "SC" since Rust has real namespaces.

Interesting, thanks for explaining it. I would like to see this explanation included in the docs for both of the enums so that it's clear to the users what's going on. I don't like these decisions to be completely lost in GitHub PRs.

Once you've done that, go ahead and r+ susurrus for me, as I think this looks good.

Susurrus · 2017-07-12T23:06:05Z

This time I went through some of the enum variants and a few are missing constants in libc. I'd like us to stop declaring constants here and only do them in libc. I know there was just a release of libc and you wanted to get this into the next version of nix, so we either wait until a release contains those constants or we push this from the current release.

asomers · 2017-07-15T15:36:10Z

Susurrus stated his conditional approval on me adding a few more comments for SysconfVar and PathconfVar, so here it is:

bors r+ susurrus

630: Add wrappers for sysconf(3), pathconf(2), and fpathconf(2) r=asomers

bors · 2017-07-15T16:13:10Z

Build succeeded

asomers mentioned this pull request Jun 28, 2017

Release 0.9.0 #632

Closed

10 tasks

asomers force-pushed the sysconf branch from 90b8733 to 0f0825e Compare July 4, 2017 04:52

Susurrus suggested changes Jul 4, 2017

View reviewed changes

Susurrus suggested changes Jul 9, 2017

View reviewed changes

asomers force-pushed the sysconf branch from 6e3da15 to cd706fb Compare July 9, 2017 17:35

Susurrus suggested changes Jul 10, 2017

View reviewed changes

asomers force-pushed the sysconf branch from cd706fb to 96dcb88 Compare July 11, 2017 03:41

Susurrus suggested changes Jul 11, 2017

View reviewed changes

asomers force-pushed the sysconf branch from 96dcb88 to 8532150 Compare July 12, 2017 01:47

Susurrus suggested changes Jul 12, 2017

View reviewed changes

asomers changed the title ~~Add wrappers for sysconf(3), pathconf(3), and fpathconf(3)~~ Add wrappers for sysconf(3), pathconf(2), and fpathconf(2) Jul 13, 2017

Add sysconf(3), pathconf(2), and fpathconf(2)

b259964

asomers force-pushed the sysconf branch from 8532150 to b259964 Compare July 15, 2017 15:35

bors bot added a commit that referenced this pull request Jul 15, 2017

Merge #630

f84a008

630: Add wrappers for sysconf(3), pathconf(2), and fpathconf(2) r=asomers

Susurrus approved these changes Jul 15, 2017

View reviewed changes

bors bot merged commit b259964 into nix-rust:master Jul 15, 2017

Add wrappers for sysconf(3), pathconf(2), and fpathconf(2) #630

Add wrappers for sysconf(3), pathconf(2), and fpathconf(2) #630

Conversation

asomers commented Jun 25, 2017

Susurrus commented Jun 25, 2017

asomers commented Jun 25, 2017

Susurrus commented Jun 25, 2017

Susurrus commented Jun 25, 2017

asomers commented Jun 25, 2017

asomers commented Jun 27, 2017

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 12, 2017

asomers commented Jul 15, 2017

bors bot commented Jul 15, 2017

Build succeeded