eminence / procfs

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

Revive RFC: Change Process to use directory fd #171

Closed sunfishcode closed 2 years ago

sunfishcode commented 2 years ago

This updates and polishes the patch from #127, which seems to be abandoned, as the author is listed as @ghost. It uses openat and readlinkat to protect against PID reuse.

This also makes two additional changes: it adds a dependency on rustix, which provides safe APIs for openat, readlinkat, and so on, and it bumps the MSRV to 1.48. I know you just recently bumped the MSRV to 1.42, but 1.48 is worth considering, as it's the version of Rust currently in Debian stable, it's newer than the version in the oldest still-supported Fedora release, and it eliminates the need for special instructions for the hex and bitflags dependencies. That said, people have different needs, and of course it's your call as to whether these changes are appropriate for procfs.

Fixes #125.

eminence commented 2 years ago

Hey, this is pretty neat, thanks for the PR. There's a lot to unpack (and to review) here, so please bear with me...

I took a quick skim and despite the large diff, everything is pretty readable. But I do want to spend some more time looking at all the changes. A few brief comments for now:

sunfishcode commented 2 years ago
* I'm not opposed to the port to `rustix`, but I do want to study it a bit more.  In particular, a small number of `rustix` types are exposed as part of the public API of this crate, so the port to `rustix` is not quite an entirely internal implementation details.

Some of those things were APIs which don't actually need to be public, and others were things that could be avoided. I've now fixed them, so rustix doesn't appear in the public API at all.

eminence commented 2 years ago

Thanks, I appreciate that

sunfishcode commented 2 years ago

Is there anything else you'd like to see in this PR?

eminence commented 2 years ago

No, sorry for my delinquency here. I did spend some time trying to update a project to using this new version, and I will say that the fact that Process is no longer clone and the fact that there is no longer a pre-populated stat field was surprisingly annoying. I think I'm going to want to think about a new quality-of-life APIs before releasing this.

But that of course doesn't need to help up this PR.

One final question for you: How closely should I be following rustix versions? Do you think the current minimum version of 0.34.0 will be fine for the foreseeable future?

sunfishcode commented 2 years ago

Yeah, 0.34.0 should be good for a while. There will likely be a 0.35 at some point over the next few months, to synchronize rustix with the io_safety API in Rust as it stabilizes, but I'm expecting to backport any important bug fixes to 0.34 point releases.