Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sysinfo::Groups implementation #1210

Merged
merged 7 commits into from
Feb 25, 2024
Merged

Add sysinfo::Groups implementation #1210

merged 7 commits into from
Feb 25, 2024

Conversation

sisungo
Copy link
Contributor

@sisungo sisungo commented Feb 22, 2024

This PR tries to implement #1198. It contains:

  • NEW PUBLIC API: sysinfo::Groups
  • Structure Change: sysinfo::Group's internal structure is changed to have a GroupInner.
  • Changes to users modules, in order to adapt the structure change.

When trying to implement the feature, I found on Windows, gid is always recognized as 0, but a SID is available. This PR didn't change Windows GID to SID as it may break public interface.

@@ -3213,8 +3384,7 @@ impl User {
/// ```
#[derive(PartialEq, Eq, PartialOrd, Ord, Debug)]
pub struct Group {
pub(crate) id: Gid,
Copy link
Owner

@GuillaumeGomez GuillaumeGomez Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this change? Gid is already generated based on the OS so what needed for a GroupInner type to be created?

name: to_str(entry.lgrui0_name),
id: Gid(0),
inner: GroupInner {
gid: Gid(0),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the main reason why I didn't provide a Group API: Windows gid is pointless, making the API kinda useless and incoherent on this platform.

@GuillaumeGomez
Copy link
Owner

It's a great start, thanks! I have a few questions and also an annoying blocker: this new API is currently not useable as is on Windows (get_by_id doesn't work there, and it's an issue). So please remove these helpers, people have access to the data already with list and list_mut.

@sisungo
Copy link
Contributor Author

sisungo commented Feb 23, 2024

getgrent, setgrent and endgrent are likely unavailable on Android?

@GuillaumeGomez
Copy link
Owner

It's available on android but:

#if __POSIX_VISIBLE >= 200112 || __XPG_VISIBLE
/* Android has thousands and thousands of ids to iterate through */
struct group* getgrent(void) __attribute__((warning("getgrent is inefficient on Android")));
void setgrent(void);
void endgrent(void);
int getgrgid_r(gid_t, struct group *, char *, size_t, struct group **);
int getgrnam_r(const char *, struct group *, char *, size_t, struct group **);
#endif

So I suppose the current approach is ok. Also, it's libc which might not have it, but it's available on android.

@GuillaumeGomez
Copy link
Owner

I checked a bit further and it's discouraged to use getgrent (as we can see in the code I copied) so I think it might be better to parse the file as you originally did (sorry).

About GroupInner, since it's exactly the same type for all platforms, here what I suggest: create the type in common.rs as pub(crate) and then implement GroupInner in unix, unknown and windows.

I see you also removed the helpers on Groups. Well done! 👍

So with this, I think it'll be complete. Sorry again for the back and forth. ^^'

src/common.rs Outdated
@@ -3190,9 +3330,15 @@ impl User {
}
}

#[derive(PartialEq, Eq, PartialOrd, Ord, Debug)]
pub(crate) struct BaseGroupInner {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not possible to declare GroupInner here and to implement in the sub modules?

@GuillaumeGomez
Copy link
Owner

Code implementation looks good! Just one thing missing: a test. Can you add a test checking that Groups is not empty on supported platforms please? Something like:

#[test]
fn test_groups() {
    if !crate::IS_SUPPORTED_SYSTEM {
        return;
    }
    assert!(!Groups::new_with_refresh_list().is_empty());
}

@sisungo
Copy link
Contributor Author

sisungo commented Feb 25, 2024

Unit tests added.

@GuillaumeGomez
Copy link
Owner

Great work, thanks a lot!

@GuillaumeGomez GuillaumeGomez merged commit 26402d2 into GuillaumeGomez:master Feb 25, 2024
65 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants