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

Expose /proc/<pid>/maps parsing (and others) #236

Closed
afranchuk opened this issue Jan 17, 2023 · 4 comments · Fixed by #237
Closed

Expose /proc/<pid>/maps parsing (and others) #236

afranchuk opened this issue Jan 17, 2023 · 4 comments · Fixed by #237

Comments

@afranchuk
Copy link
Contributor

We'd like to expose the parsing of /proc/<pid>/maps in the external interface to be able to use it on stored memory map information in rust-minidump. We currently parse the maps ourselves, however we are interested in using a separate crate so that we may re-use the parsing amongst multiple crates that we have. Since procfs basically already has this and much more, we figured it'd be great if we could use it instead. We also have other needs for parsing various procfs information so we would likely use it in the future for those purposes, too.

We're content with contributing this API exposition, however we wanted to check with the primary maintainer(s) that this is a desirable API to be added to the crate. Specifically, the procfs crate currently describes itself as "... an interface to the proc pseudo-filesystem on linux", and if we were to add functions which can operate on raw/stored procfs information, they would not technically be interfacing with the proc pseudo-filesystem at all (e.g. parsing the stored content of the maps file doesn't involve interacting directly with any mounted procfs). We don't expect this would be a problem, but it's worth noting the distinction.

Thanks!

@eminence
Copy link
Owner

Hi, thanks for opening the issue. What you're proposing sounds fine to me. In fact, there is some precedent already. For example, we have KernelStats::from_reader() which allows ones to pass in a arbitrary reader. Several other structures also have similar from_reader() methods, but not all. This inconsistency is not well justified, and I would be happy if every bit of parsing code in procfs was exposed more directly. (I'm not asking anyone to do this, I'm just noting that the lack of an exposed function to parse maps files isn't because of some fundamental objection to that functionality)

The most common use case I've seen for these from_reader() is when you have the procfs pseudo-filesystem mounted somewhere other than /proc/, but your use-case sounds equally valid to me.

If you wanted to open a PR, I would be grateful! My API preference is to have something like from_reader() (both in naming and arguments), simply because it'll be consistent with other parts of the crate, but if that particular API doesn't work for you, please let me know.

Thanks!

@afranchuk
Copy link
Contributor Author

A somewhat related question: is there a reason that MemoryMap doesn't parse the permissions string? Would you be opposed to that being added, or would you prefer to keep it as a string and add a separate function/struct to parse it?

Also, how opposed are you to making changes that break the current API? I'm adding a MemoryMaps struct (if only to have a logical thing to use with from_reader) and ideally Process::maps() and Process::smaps() would return that. It's easy to not making a breaking change there if desirable, but parsing the memory permissions would definitely be a breaking change unless it were added as a separate field in addition to the perms string.

@eminence
Copy link
Owner

No particular reason that the perms aren't parsed. Parsing that would be nice.

Breaking changes are generally fine. Version 0.14 is the latest on crates.io, but the latest code in the master branch already has some breaking API changes, so the next release will be a 0.15

@afranchuk
Copy link
Contributor Author

Great, thanks!

afranchuk added a commit to afranchuk/procfs that referenced this issue Jan 23, 2023
A new MemoryMaps struct is added, which parses all of `maps`, `smaps`,
and `smaps_rollup`.

Closes eminence#236.
afranchuk added a commit to afranchuk/procfs that referenced this issue Jan 23, 2023
A new MemoryMaps struct is added, which parses all of `maps`, `smaps`,
and `smaps_rollup`.

Closes eminence#236.
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 a pull request may close this issue.

2 participants