Fix process iterator when used with a custom root #204

ludo-c · 2022-09-18T14:42:58Z

Hello,

My context is a lxc container with a limited version of /proc containing only processes that are running in the container.
The full proc mounted in another directory, /proc-full.

When I call all_processes_with_root with the root /proc-full, the iterator creates Process with the "default" constructor instead of the one with with_root: https://github.com/eminence/procfs/blob/master/src/process/mod.rs#L1541
The creation of the Process fails silently as there is no check for that and all_processes_with_root returns 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:

In the tests I change the failure crate to anyhow. failure has been marked as deprecated : https://github.com/rust-lang-deprecated/failure
add a new test for the loadavg_from_reader
Fix a cargo clippy warning

The test sys::kernel::random::tests::test_poolsize fails but it seems that is was failing before these modifications too.

ludo-c · 2022-09-18T15:14:53Z

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.

tatref · 2022-09-21T23:21:22Z

src/process/mod.rs

@@ -1528,6 +1531,7 @@ pub fn all_processes_with_root(root: impl AsRef<Path>) -> ProcResult<ProcessesIt
 /// for more information.
 #[derive(Debug)]
 pub struct ProcessesIter {
+    root: String,


This should probably be PathBuf or OsString, because a path on Linux can be any bytes, whereas a String is valid UTF-8.

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.

I squashed it !

CI is broken because of the version in Cargo.toml, I don't think I can change that.

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)

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 !

tatref · 2022-09-23T21:21:29Z

I submitted a PR to fix CI #205

There's still some issue with a dependency, but it should be fixed soon

eminence

Thanks for this fix @ludo-c ! The bug fix looks good to me in general, but I did leave two comments on things that need changing before merging.

eminence · 2022-09-24T17:59:05Z

Cargo.toml

@@ -30,7 +30,7 @@ serde = { version = "1.0", features = ["derive"], optional = true }
 [dev-dependencies]
 criterion = "0.3"
 procinfo = "0.4.2"
-failure = "0.1"
+anyhow = "1.0"


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

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.

eminence · 2022-09-24T18:11:42Z

src/process/mod.rs

-                            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)),


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.

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.

eminence · 2022-09-24T18:12:13Z

src/sys/kernel/random.rs

@@ -73,7 +73,7 @@ mod tests {
        // The kernel support section in the root lib.rs file says that we only aim to support >= 2.6 kernels,
        // so only test that case
        let poolsize = poolsize().unwrap();
-        assert!(poolsize == 4096)
+        assert_eq!(poolsize, 4096)


This will conflict with #205. That's OK, just FYI

I remove it too, that was only to have the value in CI.

src/lib.rs

eminence · 2022-09-24T19:27:24Z

(Also, please rebase onto the last master branch)

eminence · 2022-09-27T00:33:54Z

Thanks again for the fix!

ludo-c · 2022-11-29T06:49:04Z

Hi @eminence .
As there have been some fixes, would it be possible to bump a 0.15 version please ?

Thank you !

eminence · 2022-12-01T01:01:10Z

Sure, we can get a new release out in a few days

ludo-c force-pushed the fix-process-iterator branch 2 times, most recently from ca4166a to ca8cd6d Compare September 18, 2022 15:13

tatref reviewed Sep 21, 2022

View reviewed changes

ludo-c force-pushed the fix-process-iterator branch 2 times, most recently from 38bf2b5 to 979fa73 Compare September 22, 2022 09:39

eminence requested changes Sep 24, 2022

View reviewed changes

ludo-c force-pushed the fix-process-iterator branch 6 times, most recently from ac22f3d to 934c46b Compare September 24, 2022 19:10

ludo-c added 2 commits September 24, 2022 22:01

Add one test for test_loadavg_from_reader

26d21d5

Fix process iterator with a custom root

0516f66

ludo-c force-pushed the fix-process-iterator branch from 934c46b to 0516f66 Compare September 24, 2022 20:01

ludo-c requested a review from eminence September 25, 2022 18:10

eminence approved these changes Sep 27, 2022

View reviewed changes

eminence merged commit 56fe4c7 into eminence:master Sep 27, 2022

ludo-c deleted the fix-process-iterator branch September 27, 2022 05:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix process iterator when used with a custom root #204

Fix process iterator when used with a custom root #204

ludo-c commented Sep 18, 2022

ludo-c commented Sep 18, 2022

tatref Sep 21, 2022

ludo-c Sep 22, 2022

ludo-c Sep 22, 2022

tatref Sep 22, 2022

ludo-c Sep 23, 2022

tatref commented Sep 23, 2022

eminence left a comment

eminence Sep 24, 2022

ludo-c Sep 24, 2022 •

edited

Loading

eminence Sep 24, 2022

ludo-c Sep 24, 2022

eminence Sep 24, 2022

ludo-c Sep 24, 2022 •

edited

Loading

eminence commented Sep 24, 2022

eminence commented Sep 27, 2022

ludo-c commented Nov 29, 2022

eminence commented Dec 1, 2022

Fix process iterator when used with a custom root #204

Fix process iterator when used with a custom root #204

Conversation

ludo-c commented Sep 18, 2022

ludo-c commented Sep 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatref commented Sep 23, 2022

eminence left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludo-c Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludo-c Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

eminence commented Sep 24, 2022

eminence commented Sep 27, 2022

ludo-c commented Nov 29, 2022

eminence commented Dec 1, 2022

ludo-c Sep 24, 2022 •

edited

Loading

ludo-c Sep 24, 2022 •

edited

Loading