unistd: groups: Respond to minor PR feedback items

Author: JayH5

src/unistd.rs

@@ -995,17 +995,18 @@ pub fn getgroups() -> Result<Vec<Gid>> {
     // First get the number of groups so we can size our Vec
     use std::ptr;
     let ret = unsafe { libc::getgroups(0, ptr::null_mut()) };
-    let mut size = Errno::result(ret)?;
 
     // Now actually get the groups. We try multiple times in case the number of
     // groups has changed since the first call to getgroups() and the buffer is
-    // now too small
-    let mut groups = Vec::<Gid>::with_capacity(size as usize);
+    // now too small.
+    let mut groups = Vec::<Gid>::with_capacity(Errno::result(ret)? as usize);
     loop {
         // FIXME: On the platforms we currently support, the `Gid` struct has
         // the same representation in memory as a bare `gid_t`. This is not
         // necessarily the case on all Rust platforms, though. See RFC 1785.
-        let ret = unsafe { libc::getgroups(size, groups.as_mut_ptr() as *mut gid_t) };
+        let ret = unsafe {
+            libc::getgroups(groups.capacity() as c_int, groups.as_mut_ptr() as *mut gid_t)
+        };
 
         match Errno::result(ret) {
             Ok(s) => {
@@ -1018,7 +1019,6 @@ pub fn getgroups() -> Result<Vec<Gid>> {
                 let cap = groups.capacity();
                 unsafe { groups.set_len(cap) };
                 groups.reserve(1);
-                size = groups.capacity() as c_int;
             },
             Err(e) => return Err(e)
         }
@@ -1070,9 +1070,10 @@ pub fn setgroups(groups: &[Gid]) -> Result<()> {
     Errno::result(res).map(|_| ())
 }
 
-/// Calculate the supplementary group access list. Gets the group IDs of all
-/// groups that `user` is a member of. The additional group `group` is also
-/// added to the list.
+/// Calculate the supplementary group access list.
+///
+/// Gets the group IDs of all groups that `user` is a member of. The additional
+/// group `group` is also added to the list.
 ///
 /// [Further reading](http://man7.org/linux/man-pages/man3/getgrouplist.3.html)
 ///
@@ -1108,6 +1109,7 @@ pub fn getgrouplist(user: &CStr, group: Gid) -> Result<Vec<Gid>> {
                                groups.as_mut_ptr() as *mut getgrouplist_group_t,
                                &mut ngroups)
         };
+
         // BSD systems only return 0 or -1, Linux returns ngroups on success.
         if ret >= 0 {
             unsafe { groups.set_len(ngroups as usize) };
@@ -1134,9 +1136,11 @@ pub fn getgrouplist(user: &CStr, group: Gid) -> Result<Vec<Gid>> {
     }
 }
 
-/// Initialize the supplementary group access list. Sets the supplementary
-/// group IDs for the calling process using all groups that `user` is a member
-/// of. The additional group `group` is also added to the list.
+/// Initialize the supplementary group access list.
+///
+/// Sets the supplementary group IDs for the calling process using all groups
+/// that `user` is a member of. The additional group `group` is also added to
+/// the list.
 ///
 /// [Further reading](http://man7.org/linux/man-pages/man3/initgroups.3.html)
 ///
@@ -1146,7 +1150,7 @@ pub fn getgrouplist(user: &CStr, group: Gid) -> Result<Vec<Gid>> {
 /// another user. For example, given the user `www-data`, we could look up the
 /// UID and GID for the user in the system's password database (usually found
 /// in `/etc/passwd`). If the `www-data` user's UID and GID were `33` and `33`,
-/// respectively, one could switch user as follows:
+/// respectively, one could switch the user as follows:
 /// ```
 /// let user = CString::new("www-data").unwrap();
 /// let uid = Uid::from_raw(33);

test/test_unistd.rs

@@ -4,7 +4,7 @@ use nix::unistd::*;
 use nix::unistd::ForkResult::*;
 use nix::sys::wait::*;
 use nix::sys::stat;
-use std::{env, iter};
+use std::{self, env, iter};
 use std::ffi::CString;
 use std::fs::File;
 use std::io::Write;
@@ -110,11 +110,10 @@ mod linux_android {
 fn test_setgroups() {
     // Skip this test when not run as root as `setgroups()` requires root.
     if !Uid::current().is_root() {
-        use std::io;
-        let stderr = io::stderr();
+        let stderr = std::io::stderr();
         let mut handle = stderr.lock();
         writeln!(handle, "test_setgroups requires root privileges. Skipping test.").unwrap();
-        return
+        return;
     }
 
     #[allow(unused_variables)]
@@ -141,11 +140,10 @@ fn test_initgroups() {
     // Skip this test when not run as root as `initgroups()` and `setgroups()`
     // require root.
     if !Uid::current().is_root() {
-        use std::io;
-        let stderr = io::stderr();
+        let stderr = std::io::stderr();
         let mut handle = stderr.lock();
         writeln!(handle, "test_initgroups requires root privileges. Skipping test.").unwrap();
-        return
+        return;
     }
 
     #[allow(unused_variables)]
@@ -157,7 +155,8 @@ fn test_initgroups() {
     // It doesn't matter if the root user is not called "root" or if a user
     // called "root" doesn't exist. We are just checking that the extra,
     // made-up group, `123`, is set.
-    // FIXME: This only tests half of initgroups' functionality.
+    // FIXME: Test the other half of initgroups' functionality: whether the
+    // groups that the user belongs to are also set.
     let user = CString::new("root").unwrap();
     let group = Gid::from_raw(123);
     let group_list = getgrouplist(&user, group).unwrap();