search

eminence / procfs

Rust library for reading the Linux procfs filesystem
Other
362 stars 106 forks source link

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

Closed afranchuk closed 1 year ago

afranchuk commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Great, thanks!