- Extract all the 'if ptr.is_null() .. else ..' logic into a function that's basically the ptr_as_ref feature.
- Start using 'if let' syntax for some of these methods, which I'm not sure was around when I started writing them.
- vec![] -> Vec::new()
- loop -> for i in 0..
- Better comments, too
- Replace the calls to from_utf8_unchecked to from_utf8_lossy. I don't think OSes will actually check the validity of these username strings, and I'm not sure what the best thing to do here is! Error out?
- Remove the casts to *const i8. I think these stemmed from when I didn't know what I was doing.
- Document what this module does
This tries to mimic the Metadata struct in libc by providing a struct with hidden fields, then an OS-dependent trait that gives you accessors to those fields.
Its goal is to remove any OS-specific things about users and groups -- which there will be a lot of -- from the default, publicly-accessible struct.
This may not seem like a big problem, but you'll see it a lot in mocking code: "unimportant" fields, such as the user's home directory and shell, can now be mocked to include a "sensible" value even when they're not used.
It's more descriptive to call it a cache when there are functions that do all the *actual* OS users stuff separately now.
Similarly, we may as well call its constructor 'new' instead of 'empty_cache', because it's obvious that it's a cache now.
Remove `c_int` from the libc public exports list while I'm at it. This might break someone's code, but it shouldn't have been used from this library anyway (only for user switching functions!)
It was getting crowded in there! Not only are two groups of six easier to manage than one group of twelve, but the groups functions didn't actually have anything to do with users...
The primary goal of this commit was to avoid having to clone the users and groups it produces. The old implementation was written by me when I hadn't *fully* internalised how lifetimes worked, and just used `clone` to get around the error messages. It was only later that I realised how inefficient this actually is -- the values stored in the cache are never used, only their clones are!
Several attempt at this later, I settled on using `Arc` for everything and cloning *those* values instead.
All the gory details of what happened are written in a comment in the OS module. The short version is that it's now possible to change the `Users` trait's methods from taking `&mut self` to just taking `&self`. This is within the allowed uses of `Cell`: implementation details of a method that does actually return the same thing each time, which this is.
The main casualty is that the user now has to know about `Arc` and initialise a user with a username in such a container, which I hope to solve in a later commit.
The pairing of forward and backward maps was repeated twice: once for users, and once for groups. This commit refactors them into a BiMap struct that contains HashMaps of the relevant types.
Note that these new structs are just dumb structs: they don't implement any BiMap functionality, and all the other pieces of code don't do anything different with the maps -- they just refer to the struct's fields, making this a smaller change than it could be.
This commit does two things:
Firstly, it swaps the locations in the code where the actual, unsafe C library operations are done from being done in the OSUsers struct to in the stand-alone functions. This makes more sense, as previously, the stand-alone functions would instantiate a new cache, run a method on it, and then immediately discard the cache. Now the cache uses the methods, not the other way round.
Secondly, it extracts all of the caching methods to its own module, leaving `lib` with a bare-bones users implementation.
Ben S
Alexander Polakov