Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish ss example mapping connections to pids #151

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adrianlzt
Copy link

@adrianlzt adrianlzt commented Jan 25, 2021

Implementation similar to ss
https://fossies.org/linux/iproute2/misc/ss.c

@andresrc
Copy link

@elastic/agent

@andresrc
Copy link

andresrc commented Feb 2, 2021

@fearful-symmetry can you take a look?


fds, err := pidDir.Readdirnames(0)
if err != nil {
continue

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For errors like this, where there's no expectation that they might fail, we might want to actually fail, or at least comment if we expect them to fail for some reason.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could fail if the pid has finished and the dir does not exists any more.

Failing there will generate a lot of useless "warnings" IMHO

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case, can we at least add a comment to clarify?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gomacro> import "os"
gomacro> a, err := os.Open("/proc/955374/fd")
gomacro> err
<nil>   // error
<--- here I kill process 955374
gomacro> a.Readdirnames(0)
[]      // []string
readdirent: no such file or directory   // error
gomacro>

It is commented in line 126

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Considering that we can hit errors relating to non-existent PIDs in a few places, can we move that comment block in 126 out of that if block and move it somewhere, perhaps before the Open call just to make the comment a little more explicit?

for _, fd := range fds {
link, err := os.Readlink("/proc/" + string(pid) + "/fd/" + fd)
if err != nil {
continue

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here.

@elasticmachine
Copy link

elasticmachine commented Feb 16, 2021

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2022-04-05T14:20:58.277+0000

  • Duration: 2 min 48 sec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants