Conversation
propertone
left a comment
There was a problem hiding this comment.
- This only handles mode for pipes - we want to handle modes for all FDs
- Please format with cargo format
- There is a missing column heading for Mode
src/main.rs
Outdated
| // Query file descriptors only if necessary (not for --exe/--root/--cwd) | ||
| if fd_filter.query_fds() { | ||
| for fd in &process.fds { | ||
| let mode = if fd.mode() & READ == READ { 'r' } else { 'w' }; |
There was a problem hiding this comment.
Make mode String type then ?
| .then_some(FDTarget::Path(path_buf.clone())), | ||
| process::FDTarget::Pipe(inode) => fd_filter.query_pipe().then(|| { | ||
| let mode = if fd.mode() & READ == READ { 'r' } else { 'w' }; | ||
| FDTarget::Pipe(PipeInfo { |
There was a problem hiding this comment.
Let's also remove mode from PipeInfo as it's redundant with the fd mode
There was a problem hiding this comment.
Idk if we can do this because we're using mode for filtering while doing a lookup, see here
There was a problem hiding this comment.
That refers to PipeTarget not PipeInfo, but we're also using mode in fmt::Display for FDTarget, so that would be more complicated to refactor. So fine to leave it as is for now.
|
@propertone I made some logical changes in the code to support |
| process::FDTarget::Pipe(inode) => fd_filter.query_pipe().then(|| { | ||
| let mode = if fd.mode() & READ == READ { 'r' } else { 'w' }; | ||
| let pipe_mode = mode.chars().next().unwrap(); | ||
| FDTarget::Pipe(PipeInfo { |
There was a problem hiding this comment.
Why don't you make PipeInfo.mode a string as well to simplify this?
There was a problem hiding this comment.
@propertone
It was better to make the changes like this otherwise we have to refactor many things.
Even if I change Pipeinfo.mode mode still needs to be used as a char type in lookup_pipe2pd, because PipeTarget also uses char type mode.
My humble suggestion is (if we want to refactor) we scrap PipeTarget.mode and convert mode to String everywhere else.
src/main.rs
Outdated
| let mode = if fd.mode() & READ == READ { | ||
| String::from("r") | ||
| } else if fd.mode() & WRITE == WRITE { | ||
| String::from("w") | ||
| } else { | ||
| String::from("rw") | ||
| }; |
There was a problem hiding this comment.
This logic is wrong - the rw case is never reached
src/main.rs
Outdated
| targets.retain(|x| !(x.pid == pid && x.fd == fd)); | ||
| // exclude fds with the same mode | ||
| targets.retain(|x| x.mode != mode); | ||
|
|
src/main.rs
Outdated
| let mode_str = match &fd_entry.mode { | ||
| Some(mode) => mode, | ||
| None => &String::new(), | ||
| }; |
There was a problem hiding this comment.
You can instead do:
| let mode_str = match &fd_entry.mode { | |
| Some(mode) => mode, | |
| None => &String::new(), | |
| }; | |
| let mode_str = fd_entry.mode.clone().unwrap_or_default(); |
20a50f4 to
dcffe02
Compare
src/main.rs
Outdated
| .join(","); | ||
|
|
||
| format!("pipe {direction} {joined_endpoints}") | ||
| format!("pipe {joined_endpoints}") |
There was a problem hiding this comment.
Let's add an arrow here. The direction is now redundant with the mode, but without the arrow, it's confusing to know what pipe [12] means, for example.
| format!("pipe {joined_endpoints}") | |
| format!("pipe -> {joined_endpoints}") |
|
Oh can you also please sign the CLA here: https://www.deshaw.com/oss/cla |
Signed-off-by: Adv <adhvaithhundi.221ds003@nitk.edu.in>
|
Thanks!! Please let me know if there are more issues I can work on. |
propertone
left a comment
There was a problem hiding this comment.
Looks good - thank you for the contribution! I will merge & deploy later this week.
Fixes #8