Add `getpwnam` and related functions by ctrlcctrlv · Pull Request #864 · nix-rust/nix

ctrlcctrlv · 2018-02-23T08:55:38Z

This PR closes #862.

Implemented functions:

As User::query:
- getpwnam
- getpwuid
As Group::query:
- getgrgid
- getgrnam
As Users iterator:
- getpwent
- setpwent
- endpwent
As Groups iterator:
- getgrent
- setgrent
- endgrent

~~I wanted to implement getgrent, setgrent and endgrent, but they are missing from libc. (See rust-lang/libc#923 )~~

kristate

Thanks for your commit!

kristate · 2018-02-26T05:25:01Z

+    fn from(pw: libc::passwd) -> Passwd {
+        unsafe {
+            Passwd {
+                name: CString::new(CStr::from_ptr(pw.pw_name).to_bytes()).unwrap(),


I wonder if it is smart to use CStrings here (as opposed to Strings).
Does the library use CStrings in any other places?

I took a quick search of the code, and it does seem that we are using String and not CString as an interface. Please switch to String.

Ref:

nix/src/ifaddrs.rs

Line 21 in a00bd10

pub interface_name: String,

nix/src/pty.rs

Line 146 in ebf7505

pub unsafe fn ptsname(fd: &PtyMaster) -> Result<String> {

nix/src/pty.rs

Line 167 in ebf7505

pub fn ptsname_r(fd: &PtyMaster) -> Result<String> {

et cetera

Thanks!

Thank you very much for your review. I did have what I believed to be a good reason to use CStrings. If this function is ran on a non UTF8 system, the fields, especially gecos, can be in a non UTF8 encoding such as SHIFTIJIS. from_utf8_lossy will cause data loss. Should String still be used in light of this?

Does Nix support any operating systems that run in non-UTF8 mode?

Yes, many. FreeBSD, Linux etc can all use /etc/passwd files that are in the user's non-UTF8 locale. I hate to keep mentioning SHIFT-JIS, but because in my day job I'm dealing with a lot of SJIS data and SJIS systems, I am accutely aware of this problem. Japanese sysadmins, even in 2018, in many cases still prefer SJIS because it is byte for byte around 2/3 to 1/2 the size of UTF-8. So that means an uncompressed database of Japanese text data is double the size if it's UTF-8. Therefore, many many Unix systems in Japan use SJIS everywhere, including the passwd and group files.

kristate · 2018-02-26T05:26:18Z

+    unsafe fn members(mem: *mut *mut c_char) -> Vec<CString> {
+        let mut ret = vec![];
+
+        for i in 0.. {


This looks like it has too many moving parts.

I couldn't think of another way to decode a **char

kristate · 2018-02-26T05:27:46Z

+pub fn getpwent() -> Result<Option<Passwd>> {
+    let pw = unsafe { libc::getpwent().as_ref() };
+
+    if errno::errno() != 0 {


Is it smart to check the errno in this way?

It works, but I do have to admit that I did not really understand too well the errno.rs module.

libc::getpwent also uses a static object. It also isn't very Rusty, because it's basically the next method for an implied iterator. I don't think it should be directly exposed by Nix. Instead, I think you should create an iterator class that will internally use libc::getpwent_r to iterate through the password file. Something like this:

struct PwEntries { ... } impl PwEntries { fn new() -> Self { libc::setpwent(); ... } } impl Iterator for PwEntries { type item = Passwd; fn next(&mut self) -> Option<Passwd> { ... } } impl Drop for PwEntries { fn drop(&mut self) { libc::endpwent(); } }

Unfortunately, this still isn't 100% reentrant since those setpwent and endpwent functions still store their file descriptors in static variables. But I don't see a better option.

libc::getpwent_r does not exist, so I suppose this is on hold until I can figure out how to get it added to libc...

kristate · 2018-02-26T05:29:10Z

+#[inline]
+pub fn endpwent() -> Result<()> {
+    unsafe { libc::endpwent() };
+    if errno::errno() != 0 {


kristate · 2018-02-26T05:30:52Z

+#[derive(Debug, Clone, PartialEq)]
+pub struct Passwd {
+    /// username
+    pub name: CString,


Is it important to mark these all as pub?

I think so. . . the whole point of these functions is to get data from this struct.

One thing that other Nix modules do is create a Newtype to wrap the libc struct, and define accessors for that. See for example nix::sys::Kevent or nix::sys::UtsName. I think that pattern would work well here. You'd do something like:

pub struct Passwd(libc::passwd); impl Passwd { pub fn name(&self) -> &str { ... } }

@asomers That seems very Java-like to me and not necessary for these structs. Furthermore, some day I may also add putpwent, which would require a user to build this struct. putpwent is basically how the shell utilitty adduser works. http://man7.org/linux/man-pages/man3/putpwent.3.html

Accessor functions may seem unnecessary to you, but converting C's struct passwd to a different layout seems unnecessary to me. As long as we're dealing with C and Rust datatypes that have incompatible layouts, we're going to need some kind of conversion. I like the accessor method version better, because that way we won't waste time converting fields that the user doesn't care about. We'll probably save RAM too.

asomers · 2018-02-27T15:39:19Z

+/// Gets a user DB entry based on username (see
+/// [getpwnam(3)](http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpwnam.html))
+pub fn getpwnam<S: Into<Vec<u8>>>(name: S) -> Result<Option<Passwd>> {
+    let pw = unsafe { libc::getpwnam(CString::new(name).unwrap().as_ptr()).as_ref() };


The man page says that applications should explicitly set errno to 0 before calling getpwnam.

asomers · 2018-02-27T15:45:29Z

+/// Gets a user DB entry based on username (see
+/// [getpwnam(3)](http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpwnam.html))
+pub fn getpwnam<S: Into<Vec<u8>>>(name: S) -> Result<Option<Passwd>> {
+    let pw = unsafe { libc::getpwnam(CString::new(name).unwrap().as_ptr()).as_ref() };


libc::getpwnam is not thread safe because it uses a static buffer. In fact, it's worse than that, because the static buffer is malloced on demand, and libc will free it and reallocate it if it isn't large enough. That means that multithreaded programs can use-after-free if they call getpwnam without synchronization. You should use libc::getpwnam_r instead.

asomers · 2018-02-27T15:46:25Z

+/// [getpwuid(3)](http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpwuid.html))
+#[inline]
+pub fn getpwuid(uid: Uid) -> Result<Option<Passwd>> {
+    let pw = unsafe { libc::getpwuid(uid.0).as_ref() };


Same concerns here about thread-safety and initializing errno.

asomers · 2018-02-27T16:01:29Z

+pub fn getpwent() -> Result<Option<Passwd>> {
+    let pw = unsafe { libc::getpwent().as_ref() };
+
+    if errno::errno() != 0 {


libc::getpwent also uses a static object. It also isn't very Rusty, because it's basically the next method for an implied iterator. I don't think it should be directly exposed by Nix. Instead, I think you should create an iterator class that will internally use libc::getpwent_r to iterate through the password file. Something like this:

struct PwEntries { ... } impl PwEntries { fn new() -> Self { libc::setpwent(); ... } } impl Iterator for PwEntries { type item = Passwd; fn next(&mut self) -> Option<Passwd> { ... } } impl Drop for PwEntries { fn drop(&mut self) { libc::endpwent(); } }

Unfortunately, this still isn't 100% reentrant since those setpwent and endpwent functions still store their file descriptors in static variables. But I don't see a better option.

asomers · 2018-02-27T16:16:39Z

+/// Gets a group DB entry based on group name (see
+/// [getgrnam(3)](http://pubs.opengroup.org/onlinepubs/9699919799/functions/getgrnam.html))
+#[inline]
+pub fn getgrnam<S: Into<Vec<u8>>>(name: S) -> Result<Option<Group>> {


Ditto all the comments for the getpwent family of functions.

asomers · 2018-02-27T16:22:09Z

+    fn from(pw: libc::passwd) -> Passwd {
+        unsafe {
+            Passwd {
+                name: CString::new(CStr::from_ptr(pw.pw_name).to_bytes()).unwrap(),


Does Nix support any operating systems that run in non-UTF8 mode?

asomers · 2018-02-27T16:22:29Z

+#[derive(Debug, Clone, PartialEq)]
+pub struct Passwd {
+    /// username
+    pub name: CString,


One thing that other Nix modules do is create a Newtype to wrap the libc struct, and define accessors for that. See for example nix::sys::Kevent or nix::sys::UtsName. I think that pattern would work well here. You'd do something like:

pub struct Passwd(libc::passwd); impl Passwd { pub fn name(&self) -> &str { ... } }

@gnzlbg

Add passwd/group APIs needed for nix-rust/nix#864 Hope I did this right. I only added platforms I could personally test. . . cc: @gnzlbg

ctrlcctrlv · 2018-03-03T04:43:20Z

I was able to get a bunch of functions added to libc in rust-lang/libc#934 .

I am now working on getting the APIs I added there into nix again. Hoping no more libc changes are needed.

ctrlcctrlv · 2018-03-03T11:23:48Z

@kristate @asomers @Susurrus I would very much appreciate it if y'all could read what I wrote in the commit message here 3612526

I'm worried that this problem will cause all my work on this PR to go to waste, very stressed since I've poured hours into this already 😢

ctrlcctrlv · 2018-03-03T11:35:39Z

I don't know why the test system is doing this:

   Compiling libc v0.2.38 (https://github.com/rust-lang/libc#936e16bc)
   Compiling libc v0.2.37

Why it immediately compiles old version after new? I got a PR into libc for the express purpose of getting this PR into nix, so using 0.2.38 is an absolute must. My branch will not compile without it.

ctrlcctrlv · 2018-03-03T11:37:02Z

😟

asomers · 2018-03-03T15:34:42Z


 [dependencies]
-libc = { git = "https://github.com/rust-lang/libc" }
+libc = { version = ">=0.2.38", git = "https://github.com/rust-lang/libc", branch = "master" }


This change is unnecessary and can be reverted. Cargo will always try to use the latest version when you use a "git" specification. If the included libc version in your workspace seems behind, then you should run cargo update.

asomers · 2018-03-03T15:43:21Z

+/// Users::new()
+///     .map(
+///         |pw| println!("{}\t{}",
+///                       pw.name.to_string_lossy().into_owned(),


Do you really need the into_owned?

asomers · 2018-03-03T15:44:08Z

+///     .map(
+///         |pw| println!("{}\t{}",
+///                       pw.name.to_string_lossy().into_owned(),
+///                       format!("{}", pw.uid)))


format is basically what the println is doing. I think you can shorten this entire line to pw.uid)

asomers · 2018-03-03T15:46:29Z

+///
+/// ```
+/// use nix::unistd::Users;
+/// eprintln!("Users on this system:\nname\tuid");


When you run cargo test, does all this stuff get printed? If so, you must silence it.

Even with nocapture, nothing is printed.

Doc-tests nix running 1 test test src/unistd.rs - unistd::Users (line 2300) ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 47 filtered out

asomers · 2018-03-03T15:47:53Z

+#[cfg(not(target_os = "android"))]
+pub struct Users(pub usize);
+
+impl Users {


This line seems unlikely to compile on android

Yup you're right, I thought the compiler was smarter than it is, since the struct doesn't exist when compiling for Android.

asomers · 2018-03-03T15:50:56Z

+    type Item = Passwd;
+    fn next(&mut self) -> Option<Passwd> {
+
+        let mut cbuf = vec![0i8; self.0];


0u8 would be more appropriate than 0i8, I think. A 0i8 array can't store any text encodings that use the high bit.

I can't, type mismatch. libc's type c_char = i8.

asomers · 2018-03-03T16:01:35Z

+#[derive(Debug, Clone, PartialEq)]
+pub struct Passwd {
+    /// username
+    pub name: CString,


Accessor functions may seem unnecessary to you, but converting C's struct passwd to a different layout seems unnecessary to me. As long as we're dealing with C and Rust datatypes that have incompatible layouts, we're going to need some kind of conversion. I like the accessor method version better, because that way we won't waste time converting fields that the user doesn't care about. We'll probably save RAM too.

asomers · 2018-03-03T16:06:57Z

+        let i = unsafe { libc::getpwent_r(&mut pwd, cbuf.as_mut_ptr(), self.0, &mut res) };
+
+        match i {
+            0 if !res.is_null() => {


You need a way to distinguish between "entry not found" (errno==0) and "not enough buffer space" (errno==ERANGE)

asomers · 2018-03-03T16:08:41Z

+        let mut pwd: libc::passwd =  unsafe { mem::zeroed() };
+        let mut res = ptr::null_mut();
+
+        let i = unsafe { libc::getpwent_r(&mut pwd, cbuf.as_mut_ptr(), self.0, &mut res) };


Don't forget to initialize errno to zero before calling this function.

To be certain, you mean I have to make a call to Errno::clear, right?

Yeah, that'll do.

asomers · 2018-03-03T16:09:20Z

+        let mut grp: libc::group =  unsafe { mem::zeroed() };
+        let mut res = ptr::null_mut();
+
+        let i = unsafe { libc::getgrent_r(&mut grp, cbuf.as_mut_ptr(), self.0, &mut res) };


You should probably initialize errno=0 here too.

asomers · 2018-03-03T16:12:58Z

+            0 if !res.is_null() => {
+                unsafe { Some(Group::from(*res)) }
+            },
+            _ => None


Need to handle ERANGE here too.

asomers · 2018-03-03T16:18:07Z

We already have a way of serializing tests that use global variables. Look in tests/test.rs at the bottom. There are several mutexes there. You should add another one to protect the global variables used by the getpwent family of functions.

ctrlcctrlv · 2018-03-04T00:11:43Z

Oh okay thanks, I get it, I'll make the tests use that when I get home from church, as well as deal with your other concerns 😃

ctrlcctrlv · 2018-03-05T12:42:08Z

Alright, got a bunch of work done today, but as you can see from the failing builds there is still more to do :-)

Thank you everyone for your patience with me while I sort this out. Just realized I'm going to have to open another libc PR as my original accidentally neglected FreeBSD.

ctrlcctrlv · 2018-03-05T13:58:18Z

FreeBSD build will stop failing when rust-lang/libc#939 is merged.

ctrlcctrlv · 2018-03-06T14:15:53Z

Hey @asomers , really curious what you think of this API for querying instead of just exposing it under the names of the POSIX functions. It's getting late here, if you like it I'll make the Group version of it tomorrow.

ctrlcctrlv · 2018-03-06T15:01:19Z

P.S. Anybody know what's up with https://travis-ci.org/nix-rust/nix/jobs/349841588 ? I can't get test_users_iterator_smallbuf to do that on my Linux VM.

asomers · 2018-03-06T15:33:09Z

The query API could certainly work. One problem is that it's not specific enough. Why have enum values that can't be used to look up a user? Why have values that can't be used to look up a group? Better to have one enum for User queries, and one for Group queries. That way you'll catch usage problems at compile time. Another problem is that you aren't handling "user not found" errors correctly. You shouldn't return an Errno in that case.

ctrlcctrlv · 2018-03-07T14:09:46Z

Happy to report my build is now passing :D

If you have no more concerns it's ready to be merged. I responded to or fixed every concern in the previous reviews to my knowledge (hope I didn't miss anything).

@asomers @Susurrus @kristate

asomers · 2018-03-07T18:25:54Z


 [dependencies]
-libc = { git = "https://github.com/rust-lang/libc" }
+libc = { git = "https://github.com/rust-lang/libc", branch = "master" }


The branch specification isn't necessary. Please revert changes to this file.

asomers · 2018-03-07T18:27:06Z

 #[cfg(any(target_os = "android", target_os = "freebsd",
          target_os = "linux", target_os = "openbsd"))]
 pub use self::setres::*;
+#[cfg(not(any(target_os = "android", 


Why did you have to exclude so many architectures? Shouldn't this be architecture independent?

Not really sure -- my builds were failing due to missing functions on those architectures. Could be a libc issue, but it's also not unheard of for GNU libc to not have every function in every platform.

asomers · 2018-03-07T18:28:26Z

+#[derive(Debug, Clone, PartialEq, Default)]
+pub struct User {
+    /// username
+    pub name: String,


Why make some fields String if others must be CString? The inconsistency is weird.

Because it's guaranteed that the fields I made String will conform to NAME_REGEX. gecos ("real name") has no such guarantee.

Ahh, makes sense. Could you add a comment to that effect?

this seems done as well since d4d5a00.

asomers · 2018-03-07T18:29:10Z

+#[derive(Debug, Clone, PartialEq)]
+pub struct Group {
+    /// group name
+    pub name: String,


Should the concerns about shift-jis force this structure to use CString too?

Addressed above.

asomers · 2018-03-07T18:30:08Z

+}
+
+#[derive(Debug)]
+/// Query a group. The buffer size variants should not be needed 99% of the time; the default


Doc comments, here and elsewhere, should begin with a short description. Then there should be a blank line followed by the detailed description.

asomers · 2018-03-07T18:39:26Z

+    /// 
+    /// ```
+    ///
+    /// This iterator should not be used in different threads without synchronization; while doing so


Please put this in a # Safety section.

This seems done

asomers · 2018-03-07T18:41:56Z

+
+    /// This iterator can be used to get all of the groups on the system. For example:
+    ///
+    /// ```


Please put this in an # Examples section.

This seems done

asomers · 2018-03-07T18:42:07Z

+    /// 
+    /// ```
+    ///
+    /// This iterator should not be used in different threads without synchronization; while doing so


Use a # Safety section here.

This seems done

asomers · 2018-03-07T18:44:26Z

+#[test]
+fn test_getpwnam() {
+    let res = User::query( UserQuery::Name("root".to_string()) ).unwrap();
+    assert!(res.unwrap().uid == Uid::from_raw(0));


This isn't a very good test. That field would've been zero even if getpwnam did nothing, because you initialize the struct to zero. Can you find a more interesting field to check?

I transformed this into a doctest. About to push it.

asomers · 2018-03-07T18:45:19Z

+              target_arch = "s390x")))] 
+fn test_users_iterator() {
+    #[allow(unused_variables)]
+    let m = ::USER_GRP_ITER_MTX.lock().expect("Mutex got poisoned by another test");


Change the name to _m and you won't need the #[allow(unused_variables)]

I copied this pattern from elsewhere in the file. I think that the reason that other tests do it this way is because maybe the compiler will optimize it out, but I did not test.

I'm pretty sure the reason that other tests do it this way is because they were added back when the minimum compiler version was 1.14.0 or something, and it didn't understand that leading underscores denoted unused variables.

I think that would be best left to another PR, since I'd be editing many other unrelated tests, which I'd be happy to work on after this one.

Let's add the underscore and get rid of the #[allow( as part of this PR. You don't need to clean up existing examples, but since this is new code it should do this the right way from the start.

ctrlcctrlv · 2018-03-11T12:00:38Z

@asomers Any further concerns?

asomers · 2018-03-11T15:48:34Z

+/// should be more than adequate. Only use the buffer size variants if you receive an ERANGE error
+/// without them, or you know that you will be likely to.
+///
+/// # Example


s/Example/Examples for consistency with the rest of the project

@asomers What consistency? mkfifo, getcwd etc use "Example"...this makes sense because it's a lone example, in their case as well as mine.

I'm not saying we're perfect, but currently Nix has 30 "Examples" compared to 9 "Example". Best stick to the predominant style.

@asomers OK

asomers · 2018-03-23T16:53:33Z

There are still two things that concern me:

You have some really big unsafe blocks. Can you shrink the scope of those blocks to the absolute minimum?
You have some from functions that take their arguments by value, even though it looks like a reference would work fine. Unless it's absolutely necessary, it's better to take arguments by reference. That gives the caller more flexibility.

ctrlcctrlv · 2018-03-27T13:08:56Z

@asomers f47800c

Hoping this will be the final commit. 🤞

asomers · 2018-03-27T14:01:08Z

Why did you use raw pointers instead of references? Dereferencing raw pointers is unsafe.

ctrlcctrlv · 2018-03-28T09:34:46Z

@asomers Because I only ever convert libc::passwd and libc::group from raw pointers, and that is the only way to get their data. A reference to one of them would just be a reference to a raw pointer...I think. You have to dereference them, I just changed where that happens.

asomers · 2018-03-28T14:26:13Z

Yeah, but you moved the raw pointer dereference into part of the public API! And in a safe function, no less! That means that any user can pass us a *const libc::passwd (which they can create in safe code by casting), and we'll dereference it and potentially crash. That's not ok. We need to control when and where that pointer is dereferenced. It should only happen when we have control of how the pointer was created, and we know that it's valid.

ctrlcctrlv · 2018-03-29T04:20:37Z

@asomers OK, I understand. Thank you very much for the explanation. I'm sorry if I'm annoying you. This is my first major commit to a Rust library. I'll think about how to handle this. Maybe using the From trait at all is unwise, I think probably this should not be public.

Susurrus

A lot of minor style changes.

Susurrus · 2018-04-03T04:27:33Z

+    }
+}
+
+#[derive(Debug)]


These should always come after the doccomments.

Also is there a reason you didn't derive the other commonly-derived traits as well?

moved below the doc-comments, and added the full set of Debug, Copy, Clone, Eq, PartialEq, Hash, Default, for UserQuery, GroupQuery, Users, Groups (where applicable, not all can be Copy, enums can't be Default)

Susurrus · 2018-04-03T04:29:05Z

+        let mut pwd: libc::passwd =  unsafe { mem::zeroed() };
+        let mut res = ptr::null_mut();
+
+        let i = { 


Can you use a more descriptive variable name here? i is almost always a counter variable as well, so this is confusing.

changed i to error for all four occurences

Susurrus · 2018-04-03T04:29:54Z

+        };
+
+        match i {
+            0 => {


If you're just doing an if/else like this I'd recommend doing that instead of a match, which is a little more heavyweight.

changed it to an if/else.

Susurrus · 2018-04-03T04:30:23Z

+              target_arch = "s390x")))] 
+mod usergroupiter {
+    use libc;
+    use libc::{c_char};


You can just do use libc::{self, c_char};

Susurrus · 2018-04-03T04:33:35Z

+    /// `setpwent` and `endpwent`, it is very likely that iterators running in different threads will
+    /// yield different numbers of items.
+    #[derive(Debug)]
+    pub struct Users(pub usize);


Is there any reason the user of this API needs to set the internal value? If not, you should make it private and add a getter to get the value instead. This is more common with Rust types and used elsewhere in nix.

Susurrus · 2018-04-03T04:40:24Z

+/// Representation of a group, based on `libc::group`
+#[derive(Debug, Clone, PartialEq)]
+pub struct Group {
+    /// group name


The first letters in all these doccomments should be capitalized.

Susurrus · 2018-04-03T04:40:46Z

+                gid: Gid::from_raw((*pw).pw_gid),
+                #[cfg(not(any(target_os = "linux", target_os = "android")))]
+                class: CString::new(CStr::from_ptr((*pw).pw_class).to_bytes()).unwrap(),
+                #[cfg(not(any(target_os = "linux", target_os = "android")))]


The OSes need to be alphabetized. Additionally, these sould not use a negation and instead explicitly list all OSes.

alphabetical order done; avoiding negations seems quite verbose to me, and cfg(not( seems to occur more througout the existing part as well.

Susurrus · 2018-04-03T04:41:09Z

 }
+
+
+#[derive(Debug, Clone, PartialEq, Default)]


Attributes should got under the doccomments.

Susurrus · 2018-04-03T04:41:29Z

+    /// group ID
+    pub gid: Gid,
+    #[cfg(not(target_os = "android"))]
+    /// user information


Doccomments go above the attributes.

Susurrus · 2018-04-03T04:42:26Z

+}
+
+pub trait Queryable<Q> where Self: ::std::marker::Sized {
+    fn query(q: Q) -> Option<Result<Self>>;


This probably should be public, yes?

Additionally, what's with the weird Option<Result<_>> type?

The Option<Result<_>> type makes sense in this case due to the lower level API.

If you receive Some(Ok(_)), it means that there was a user found and its data fit in the buffer and is now wrapped in the result.
If you receive a Some(Err(_)), it means that there was a user found but its data did not fit in the buffer, and you should recall with one of the WithBufsize variants with a higher size.
If you receive a None, it means that no user was found.

I'm wondering if we shouldn't use our own return type to specify these different states. I haven't thought hard about this, so maybe one doesn't exist, but glancing at this I'd think we could have a more ergonomic API here.

With all due respect, I'm not sure what that would look like and don't think there's anything wrong with the current return type...maybe an alternate return type would look better, but using standard types like this is certainly more ergonomic IMO.

I think a better API would be for the caller to supply an optional bufsize. If not provided, then Nix will choose a bufsize, retrying with a larger buffer if necessary. Also, why is there a need for the Queryable trait? I get that User and Group have a similar API. But if there's never going to be any generic function with an indiscriminate Queryable type in its signature, then there's no need to define a trait. I might do it like this:

pub enum GroupQuery<'a> { Gid(gid), Name(&'a str) } impl Group { fn query(q: GroupQuery, bufsize: Option<usize>) -> Option<Result<Self>>; } let res = Group::query(GroupQuery::Name("root"), Some(2048)).unwrap();

However, it would be even simpler to eliminate the GroupQuery type entirely, replacing it with two separate constructors:

impl Group { fn from_gid(gid: Gid, bufsize: Option<usize>) -> Option<Result<Self>>; fn from_name(name: &str, bufsize: Option<usize>) -> Option<Result<Self>>; } let res = Group::from_name("root", Some(2048)).unwrap();

What @Susurrus was suggesting would be to create a bespoke return type to get rid of the double unwrap wart. It would look like this:

enum GroupResult { /// No group found None, /// An error was encountered while searching Err(Error), Some(Group) }

Such a type could still support most of the common operators that are defined for std::option::Option and std::result::Result, i.e. unwrap, is_some, is_err, is_none, etc.

ctrlcctrlv · 2018-07-03T08:43:57Z

OK, sorry everyone, the priorities of my company changed so I hadn't had much time to work on this, but the microservice I wrote which uses this API is back under scrutiny again so I'll be soon working on getting this commit into nix since we do not want to use private branches of modules if we can help it for important parts of our code.

otavio · 2019-08-21T00:57:32Z

Yes, that's the right way to handle the vectors. But you need to use std::mem::MaybeUninit for passwd rather than mem::uninitialized.

Done.

otavio · 2019-08-21T01:07:04Z

I wonder how getspnam(3) and friends (getspent, setspent, endspent, ...) could (or should) fit in this, if at all.

I think this should be handled in another PR as seems not related.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers

That looks like a step in the right direction. It certainly reduced duplication.

asomers · 2019-08-22T14:38:51Z

+                // Trigger the internal buffer resizing logic of `Vec` by requiring
+                // more space than the current capacity.
+                unsafe { cbuf.set_len(cbuf.capacity()); }
+                cbuf.reserve(1);


Extending by only one byte at a time could take awhile. It would be better to double the storage instead. That's what the C library does. Also, you shouldn't extend the buffer ad infinitum. At some point you should give up and return ERANGE. The C library does that at 1MB.

I think we can add a top limit, however, the same strategy is used inside std. For example: https://doc.rust-lang.org/src/alloc/vec.rs.html#949

Also, inside nix code it is also used. See: https://github.com/nix-rust/nix/blob/master/src/unistd.rs#L579-L583

The link from inside of Vec is very different. In that case, it really is only 1 additional element's worth of space that is needed. Here, an unknown amount of additional space is needed. That's why I suggest doubling the storage. Nix's getcwd() method should probably do the same.

The link from inside of Vec is very different. In that case, it really is only 1 additional element's worth of space that is needed. Here, an unknown amount of additional space is needed. That's why I suggest doubling the storage. Nix's getcwd() method should probably do the same.

From reading the std code, it seems different. See:

https://github.com/rust-lang/rust/blob/bdfd698f37184da42254a03ed466ab1f90e6fb6c/src/liballoc/raw_vec.rs#L494-L500

and

https://github.com/rust-lang/rust/blob/bdfd698f37184da42254a03ed466ab1f90e6fb6c/src/liballoc/raw_vec.rs#L421-L434

So it seems todo exactly this; it seems good to keep the code simple but if really desired, I can rework this.

That's just the current implementation. Nix shouldn't rely upon it. We should actually try to reserve a reasonable amount of space for what our application needs.

asomers · 2019-08-22T14:50:31Z

 }

 impl User {
+    fn from_anything(f: impl Fn(*mut libc::passwd,


This signature is so big that it would probably be written better with a where clause:

fn from_anything<F>(f: F) -> Result<Option<Self>> where F: Fn( *mut libc::passwd, *mut libc::c_char, libc::size_t, *mut *mut libc::passwd, ) -> libc::c_int

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers · 2019-09-04T14:11:11Z

+        return Err(Error::Sys(Errno::ERANGE))
+    }
+
+    unsafe { buf.set_len(buf.capacity()) };


No, absolutely not. This is very unsafe and totally unnecessary. Did you mean to commit this line?

asomers · 2019-09-04T14:11:26Z

+fn reserve_buffer_size<T>(buf: &mut Vec<T>, limit: usize) -> Result<()> {
+    use std::cmp::min;
+
+    if buf.len() == limit {


This should be >=, not ==

asomers · 2019-09-04T14:12:17Z

+    unsafe { buf.set_len(buf.capacity()) };
+
+    let capacity = min(buf.capacity() * 2, limit);
+    buf.reserve_exact(capacity);


No need to override the standard library. You can use reserve instead of reserve_exact. I just don't want to rely on reserve(1) actually reserving more space than 1.

asomers · 2019-09-04T14:17:47Z

-        let bufsize = match sysconf(SysconfVar::GETPW_R_SIZE_MAX) {
+        let cbuf_max = match sysconf(SysconfVar::GETPW_R_SIZE_MAX) {
            Ok(Some(n)) => n as usize,
            Ok(None) | Err(_) => 1024 as usize,


Not that it's defined in any publicly visible place, but FreeBSD internally sets this limit to 1MB.

asomers · 2019-09-04T14:36:32Z

        };

-        let mut cbuf = Vec::with_capacity(bufsize);
+        let mut cbuf = Vec::with_capacity(512);


You were right the first time. GETPW_R_SIZE_MAX is the suggested initial size of the buffer for password entries, not the maximum size (earlier versions of POSIX did specify the maximum size).

asomers · 2019-09-04T14:37:54Z

        };

-        let mut cbuf = Vec::with_capacity(bufsize);
+        let mut cbuf = Vec::with_capacity(512);


As above, bufsize should be the intial sizes of the vec, not the maximum.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers · 2019-09-06T16:12:22Z

            } else if Errno::last() == Errno::ERANGE {
                // Trigger the internal buffer resizing logic.
-                reserve_buffer_size(&mut cbuf, cbuf_max)?;
+                reserve_double_buffer_size(&mut cbuf, bufsize)?;


The sets the initial and maximum sizes to the same value. You should probably choose a larger maximum size. As I said, FreeBSD internally uses 1MB.

And how we can inquiry the OS to get this information? Blindly put a number does not sounds right.

You can't. There's no way to query it. And there's no need to, for consumers of the C API. It's irrelevant for getpwnam_r since the user provides his own buffer, and the user doesn't need to know it when using getpwnam. But if we want to provide a Rusty API that is both reentrant and allocates the buffer internally, then we need to choose a sane limit ourselves. Do you have a better idea?

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers

I'm curious. Why 16KB for the pwent limit?

asomers · 2019-09-12T17:21:47Z

    // groups has changed since the first call to getgroups() and the buffer is
    // now too small.
-    let mut groups = Vec::<Gid>::with_capacity(Errno::result(ret)? as usize);
+    let mut groups = Vec::<Gid>::with_capacity(Errno::result(ngroups)? as usize);


I think the code would be more readable if you moved Errno::result(...)? closer to the call site on line 1339.

Reworked. Also, the 16Kb was taken from https://linux.die.net/man/3/getpwuid_r example code.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers · 2019-09-22T02:56:38Z

@@ -1337,15 +1337,18 @@ pub fn setgid(gid: Gid) -> Result<()> {
 pub fn getgroups() -> Result<Vec<Gid>> {
    // First get the number of groups so we can size our Vec
    let ngroups = unsafe { libc::getgroups(0, ptr::null_mut()) };


What I meant is that it would be more idiomatic to write it like this:

let ngroups = unsafe { libc::getgroups(0, ptr::null_mut()) }? as usize;

1139: Add `Users` and `Group` related functions r=asomers a=otavio This was a collaborative work between Johannes Schilling <dario@deaktualisierung.org>, Fredrick Brennan <copypaste@kittens.ph> and myself. This PR is a split-off from #864 so we merge ready parts first. Next, we work on the iterators. Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> Co-authored-by: Otavio Salvador <otavio@ossystems.com.br>

ctrlcctrlv · 2020-01-30T14:13:36Z

I'm closing this because I'm tired of seeing it in my PR list. I won't delete the underlying branch, please just open a new PR if you will try to revive whatever wasn't included in #1139.

Should you care to know why, I've written a small blog about it: https://gist.github.com/ctrlcctrlv/978b3ee4f55d4b4ec415a985e01cb1c9

We have one UNIX group that contains most of our users whose size is about 20 kB, so `Group::from_name` is failing with ERANGE. The discussion on PR nix-rust#864 suggests that 1 MB is a reasonable maximum - it follows what FreeBSD's libc does. (glibc appears to have no maximum on the _r function and will just double the buffer until malloc fails, but that's not particularly Rusty.)

1409: unistd: Increase maximum passwd/group buffer to 1MB r=asomers a=geofft We have one UNIX group that contains most of our users whose size is about 20 kB, so `Group::from_name` is failing with ERANGE. The discussion on PR #864 suggests that 1 MB is a reasonable maximum - it follows what FreeBSD's libc does. (glibc appears to have no maximum on the _r function and will just double the buffer until malloc fails, but that's not particularly Rusty.) Co-authored-by: Geoffrey Thomas <geofft@twosigma.com>

ctrlcctrlv mentioned this pull request Feb 25, 2018

getpwnam is missing #862

Closed

kristate reviewed Feb 26, 2018

View reviewed changes

asomers requested changes Feb 27, 2018

View reviewed changes

ctrlcctrlv added a commit to ctrlcctrlv/libc that referenced this pull request Mar 1, 2018

Add passwd/group APIs needed for nix-rust/nix#864

5f9538d

bors added a commit to rust-lang/libc that referenced this pull request Mar 3, 2018

Auto merge of #934 - ctrlcctrlv:master, r=alexcrichton

936e16b

Add passwd/group APIs needed for nix-rust/nix#864 Hope I did this right. I only added platforms I could personally test. . . cc: @gnzlbg

asomers reviewed Mar 3, 2018

View reviewed changes

asomers requested changes Mar 3, 2018

View reviewed changes

asomers requested changes Mar 7, 2018

View reviewed changes

asomers reviewed Mar 11, 2018

View reviewed changes

Susurrus suggested changes Apr 3, 2018

View reviewed changes

otavio added 4 commits August 22, 2019 00:59

Avoid using From<*mut libc::...> using reference

de1ed63

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Reduce code duplication

51d39cf

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Handle ERANGE

3b75439

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Use Result<Option<User|Group>> so it is more ergonimic

b3d7716

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers reviewed Aug 22, 2019

View reviewed changes

Improve from_anything definition using where clause

b361db0

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

otavio force-pushed the master branch from 5f46631 to b361db0 Compare August 28, 2019 21:41

Add a generic reserve_buffer_size method

76e33fe

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers requested changes Sep 4, 2019

View reviewed changes

otavio added 3 commits September 4, 2019 22:26

Rework generic reserve_buffer_size code

5eda92f

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Use reserve_double_buffer_size<T> as method name

d0b23f3

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Fix unused code warnings

56b0cd1

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

otavio force-pushed the master branch from 5214fb0 to 56b0cd1 Compare September 5, 2019 02:04

asomers reviewed Sep 6, 2019

View reviewed changes

otavio added 3 commits September 11, 2019 00:50

Drop Default from User as this should be inquired

cc9f81c

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Use 16384 as limit

65ba0e5

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Rework more reservation code to use reserve_double_buffer_size

9111d12

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers reviewed Sep 12, 2019

View reviewed changes

Rework getgroups() according to @asomers suggestions

3fe2c01

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

asomers reviewed Sep 22, 2019

View reviewed changes

otavio mentioned this pull request Oct 19, 2019

Add Users and Group related functions #1139

Merged

ctrlcctrlv closed this Jan 30, 2020

geofft mentioned this pull request Mar 25, 2021

unistd: Increase maximum passwd/group buffer to 1MB #1409

Merged

Uh oh!

Conversation

ctrlcctrlv commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristate left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctrlcctrlv commented Mar 3, 2018

Uh oh!

ctrlcctrlv commented Mar 3, 2018

Uh oh!

ctrlcctrlv commented Mar 3, 2018

Uh oh!

ctrlcctrlv commented Mar 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctrlcctrlv commented Feb 23, 2018 •

edited

Loading