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 a sysinfo wrapper #922

Merged
merged 1 commit into from Jul 5, 2018
Merged

Add a sysinfo wrapper #922

merged 1 commit into from Jul 5, 2018

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Jul 4, 2018

Closes #505

Returned values were also inspected manually to be correct.

}

/// Current number of processes.
pub fn process_count(&self) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you picked u32 instead of u16 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The max. process count on x86-64 Linux doesn't fit in a u16. The default does, but PID_MAX_LIMIT doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

It's defined as a __u16 in the Linux header file and in libc. Maybe the kernel can support >65536 processes, but sysinfo can't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's odd. I guess I can change it to use u16 then. Both the libc crate and man page use unsigned short though, which can technically be a u32.


/// Returns the amount of completely unused RAM in Bytes.
///
/// "Unused" in this context means that the RAM in neither actively used by programs, nor by the
Copy link
Member

Choose a reason for hiding this comment

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

80 columns per line, please.

}

fn scale_mem(&self, units: libc::c_ulong) -> u64 {
(units * self.0.mem_unit as libc::c_ulong) as u64
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this multiplication is overflowing on 32-bit architectures.

/// [See `sysinfo(2)`](http://man7.org/linux/man-pages/man2/sysinfo.2.html).
pub fn sysinfo() -> Result<SysInfo> {
let mut info: libc::sysinfo = unsafe { mem::uninitialized() };
match unsafe { libc::sysinfo(&mut info) } {
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified to let res = libc::sysinfo(...); Errno::result(res)

}

/// Current number of processes.
pub fn process_count(&self) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

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

It's defined as a __u16 in the Linux header file and in libc. Maybe the kernel can support >65536 processes, but sysinfo can't.

pub fn ram_unused(&self) -> u64 {
self.scale_mem(self.0.freeram)
}

fn scale_mem(&self, units: libc::c_ulong) -> u64 {
(units * self.0.mem_unit as libc::c_ulong) as u64
(units as u64 * self.0.mem_unit as u64) as u64
Copy link
Member

Choose a reason for hiding this comment

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

The final "as u64" is superfluous.

@asomers
Copy link
Member

asomers commented Jul 4, 2018

Ok, I'm satisfied. All it needs now is a squash. We can't squash ourselves, because we use bors.

@jonas-schievink
Copy link
Contributor Author

jonas-schievink commented Jul 4, 2018

arm-unknown-linux-gnueabi failed with more RAM available than installed (unused: 4117311488, total: 3548188672). Apparently the free RAM value is relatively meaningless and can even be negative, so I removed the test (I suspect that the free RAM value includes swap space, so it can be larger that the total installed RAM).

@asomers
Copy link
Member

asomers commented Jul 5, 2018

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 5, 2018

👎 Rejected by code reviews

@asomers
Copy link
Member

asomers commented Jul 5, 2018

Silly me. I have to hit "approve" before telling bors to merge.

bors r+

bors bot added a commit that referenced this pull request Jul 5, 2018
922: Add a sysinfo wrapper r=asomers a=jonas-schievink

Closes #505

Returned values were also inspected manually to be correct.

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 5, 2018

@bors bors bot merged commit 5091847 into nix-rust:master Jul 5, 2018
@jonas-schievink jonas-schievink deleted the sysinfo branch July 5, 2018 14:12
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.

None yet

2 participants