From 0d9a3911fd5089c4cb686372638df0dd606c4fc5 Mon Sep 17 00:00:00 2001
From: Ben S <ogham@bsago.me>
Date: Fri, 29 Jan 2016 13:52:13 +0000
Subject: [PATCH] Add docs and fix warnings

---
 src/base.rs  | 27 ++++++++++++++++++++++++++-
 src/cache.rs |  8 ++++++--
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/base.rs b/src/base.rs
index 6ed9260..98169b1 100644
--- a/src/base.rs
+++ b/src/base.rs
@@ -389,11 +389,36 @@ pub fn get_effective_groupname() -> Option<String> {
 }
 
 
+/// An iterator over every user present on the system.
+///
+/// This struct actually requires no fields, but has one hidden one to make it
+/// `unsafe` to create.
 pub struct AllUsers(());
 
 impl AllUsers {
+
+    /// Creates a new iterator over every user present on the system.
+    ///
+    /// ## Unsafety
+    ///
+    /// This constructor is marked as `unsafe`, which is odd for a crate
+    /// that's meant to be a safe interface. It *has* to be unsafe because
+    /// we cannot guarantee that the underlying C functions,
+    /// `getpwent`/`setpwent`/`endpwent` that iterate over the system's
+    /// `passwd` entries, are called in a thread-safe manner.
+    ///
+    /// These functions [modify a global
+    /// state](http://man7.org/linux/man-pages/man3/getpwent.3.html#
+    /// ATTRIBUTES), and if any are used at the same time, the state could
+    /// be reset, resulting in a data race. We cannot even place it behind
+    /// an internal `Mutex`, as there is nothing stopping another `extern`
+    /// function definition from calling it!
+    ///
+    /// So to iterate all users, construct the iterator inside an `unsafe`
+    /// block, then make sure to not make a new instance of it until
+    /// iteration is over.
     pub unsafe fn new() -> AllUsers {
-        unsafe { setpwent() };
+        setpwent();
         AllUsers(())
     }
 }
diff --git a/src/cache.rs b/src/cache.rs
index 74dadbd..ad35449 100644
--- a/src/cache.rs
+++ b/src/cache.rs
@@ -120,15 +120,19 @@ impl Default for UsersCache {
 
 impl UsersCache {
 
-    /// Create a new empty cache.
+    /// Creates a new empty cache.
     pub fn new() -> UsersCache {
         UsersCache::default()
     }
 
+    /// Creates a new cache that contains all the users present on the system.
+    ///
+    /// This is `unsafe` because we cannot prevent data races if two caches
+    /// were attempted to be initialised on different threads at the same time.
     pub unsafe fn with_all_users() -> UsersCache {
         let cache = UsersCache::new();
 
-        for user in unsafe { AllUsers::new() } {
+        for user in AllUsers::new() {
             let uid = user.uid();
             let user_arc = Arc::new(user);
             cache.users.forward.borrow_mut().insert(uid, Some(user_arc.clone()));