Fix process iterator when used with a custom root #204
Fix process iterator when used with a custom root #204eminence merged 2 commits intoeminence:masterfrom
Conversation
ludo-c
force-pushed
the
fix-process-iterator
branch
2 times, most recently
from
ca4166a to
ca8cd6d
Compare
|
I removed the commit with the documentation change. -#![deny(broken_intra_doc_links)]
+#![deny(rustdoc::broken_intra_doc_links)]but it brokes the build, the commit has been removed from the PR. |
src/process/mod.rs
Outdated
| /// for more information. | ||
| #[derive(Debug)] | ||
| pub struct ProcessesIter { | ||
| root: String, |
There was a problem hiding this comment.
This should probably be PathBuf or OsString, because a path on Linux can be any bytes, whereas a String is valid UTF-8.
There was a problem hiding this comment.
Indeed, PathBuf is better, I changed it, thank you !
If you are ok with the last commit I will squash it into the "Fix process iterator with a custom root" one.
There was a problem hiding this comment.
I squashed it !
CI is broken because of the version in Cargo.toml, I don't think I can change that.
There was a problem hiding this comment.
There are multiple issues in CI atm, but not related to your changes
- rust 1.48 has an issue with the manifest format
- stable and nightly have an issue with sys::kernel::random::tests::test_poolsize. It seems related to a recent change in the kernel, see https://unix.stackexchange.com/questions/704737/kernel-5-10-119-caused-the-values-of-proc-sys-kernel-random-entropy-avail-and-p
However, I don't know if the change of failure to anyhow should be handled in this PR (I'm not the repo owner, though)
There was a problem hiding this comment.
As "failure" is used only in test and is deprecated, I thought it worst nothing to add it here.
If it is a problem I will of course remove it from the PR. The main issue is the real important thing to fix.
Thank you for your review !
ludo-c
force-pushed
the
fix-process-iterator
branch
2 times, most recently
from
38bf2b5 to
979fa73
Compare
|
I submitted a PR to fix CI #205 There's still some issue with a dependency, but it should be fixed soon |
Cargo.toml
Outdated
| criterion = "0.3" | ||
| procinfo = "0.4.2" | ||
| failure = "0.1" | ||
| anyhow = "1.0" |
There was a problem hiding this comment.
I'm OK with adding a new test for anyhow, but I don't see a reason to remove the test for failure. I would weakly prefer this to be in its own PR, though
There was a problem hiding this comment.
I did this because failure has been marked as deprecated : https://github.com/rust-lang-deprecated/failure
As it is not a big deal I didn't want to open a PR only for that.
I remove it from this PR.
src/process/mod.rs
Outdated
| break Some(Ok(proc)); | ||
| match Process::new_with_root(self.root.join(pid.to_string())) { | ||
| Ok(proc) => break Some(Ok(proc)), | ||
| Err(e) => break Some(Err(e)), |
There was a problem hiding this comment.
The documented behavior of the all_processes function is that Process objects that fail to be constructed won't be returned in the iterator. Your code here would modify that, and I don't think I want to do that right now.
If you have a use-case where you want to know about these failures, please let me know.
There was a problem hiding this comment.
It was useful only to find why I didn't have all the processes I expected (in my case /proc has less process than /proc-full)
With this fix it is not needed anymore, I remove it.
src/sys/kernel/random.rs
Outdated
| // so only test that case | ||
| let poolsize = poolsize().unwrap(); | ||
| assert!(poolsize == 4096) | ||
| assert_eq!(poolsize, 4096) |
There was a problem hiding this comment.
I remove it too, that was only to have the value in CI.
ludo-c
force-pushed
the
fix-process-iterator
branch
6 times, most recently
from
ac22f3d to
934c46b
Compare
|
(Also, please rebase onto the last master branch) |
ludo-c
force-pushed
the
fix-process-iterator
branch
from
934c46b to
0516f66
Compare
|
Thanks again for the fix! |
|
Hi @eminence . Thank you ! |
|
Sure, we can get a new release out in a few days |
3 participants
Hello,
My context is a lxc container with a limited version of
/proccontaining only processes that are running in the container.The full proc mounted in another directory,
/proc-full.When I call
all_processes_with_rootwith the root/proc-full, the iterator createsProcesswith the "default" constructor instead of the one withwith_root: https://github.com/eminence/procfs/blob/master/src/process/mod.rs#L1541The creation of the Process fails silently as there is no check for that and
all_processes_with_rootreturns successfully, but without processes.If I change that call with
Process::new_with_root, it works.As Rust is quite new to me I'm not sure that it is a good solution, can you have a look at it please ?
Thank you very much !
PS: I left some few commits from my last PR:
failurecrate toanyhow.failurehas been marked as deprecated : https://github.com/rust-lang-deprecated/failureloadavg_from_readercargo clippywarningThe test
sys::kernel::random::tests::test_poolsizefails but it seems that is was failing before these modifications too.