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 /proc/iomem #216

Merged
merged 4 commits into from
Dec 17, 2022
Merged

Add /proc/iomem #216

merged 4 commits into from
Dec 17, 2022

Conversation

tatref
Copy link
Contributor

@tatref tatref commented Nov 30, 2022

Hi,

This PR adds support for parsing /proc/iomem

The content looks like:

00100000-dffeffff : System RAM
  49200000-4a202387 : Kernel code
  4a400000-4b00bfff : Kernel rodata
  4b200000-4b7f503f : Kernel data
  4bfdb000-4c5fffff : Kernel bss
  c3000000-deffffff : Crash kernel
dfff0000-dfffffff : ACPI Tables
e0000000-fdffffff : PCI Bus 0000:00
  e0000000-e0ffffff : 0000:00:02.0

Some values are constant like System RAM, Kernel code... but others depend on the hardware and kernel modules: vmwgfx probe, PCI Bus xxxx...

Should we try to enumerate all of these in an enum like MMapPath, with a special variant holding a String?
Or we can keep it like this, with a simple String?

Another point is that this requires root or sudo, otherwise all addresses are 0. At first I added a panic! in the example, but I guess this would break CI?

What do you think?

@eminence
Copy link
Owner

Should we try to enumerate all of these in an enum like MMapPath, with a special variant holding a String?
Or we can keep it like this, with a simple String?

From first glance, it looks like there might be too many variants to try to enumerate all of them. So my vote is to just store them as Strings.

Another point is that this requires root or sudo, otherwise all addresses are 0. At first I added a panic! in the example, but I guess this would break CI?

Let's make sure the code can handle the case when all addresses are zero. In the test, the assertions can do things like: if i am root, make sure the parsed addresses are non-zero.

Also, I have no idea what's up with the CI failures. I'll look at that later

@tatref
Copy link
Contributor Author

tatref commented Nov 30, 2022

The code already handle the 0 addresses, so it's fine

I fixed CI, I was missing a colon in a docstring

src/iomem.rs Outdated
use super::{FileWrapper, ProcResult};
use crate::split_into_num;

pub struct IoMem;
Copy link
Owner

Choose a reason for hiding this comment

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

It seems a little weird to have an empty struct here. Instead of IoMem::new() what about fn iomem() -> ProcResult<Vec<PhysicalMemoryMap>>? This would be consistent with other functions that return a list of things (like diskstats() and locks())

@eminence
Copy link
Owner

eminence commented Dec 4, 2022

One last question: the data in /proc/iomem looks hierarchical, based on the indentation. Is that structure something we should capture?

@tatref
Copy link
Contributor Author

tatref commented Dec 4, 2022

One last question: the data in /proc/iomem looks hierarchical, based on the indentation. Is that structure something we should capture?

Yes we could add indentation.

Should we construct a real tree, or just record the indentation index?

@eminence
Copy link
Owner

I think having a tree would be best. But maybe we can leave that for a followup pull request. I don't think tracking just the indentation is the right thing to do.

Something strange is going on with one of your commits: 9e7aff9. This commit somehow includes some of the smaps_rollup changes from #214. I'm not quite sure how this happened, but can you take a look? Rebasing on to the latest master branch might be one way to resolve this.

@tatref
Copy link
Contributor Author

tatref commented Dec 17, 2022

I rebased from master, the commits look good.

The present version includes the indentation. Do you want me to remove it before we settle on some kind of tree structure?

@eminence
Copy link
Owner

Do you want me to remove it before we settle on some kind of tree structure?

No, we can leave it for now. But let's find something better before the next procfs release.

@eminence eminence merged commit b7bcec1 into eminence:master Dec 17, 2022
@tatref tatref deleted the iomem branch January 11, 2023 21: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