Parse fdinfo and inotify stats from /proc/[pid]/fdinfo/[fd] - #115
Conversation
pgier
left a comment
pgier
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I think this is a good start. To be more consistent with the rest of the package, the code should be in a file called proc_fdinfo.go, and it should parse the data read instead of returning strings and byte slice.
| "regexp" | ||
| "strings" | ||
| ) | ||
|
|
There was a problem hiding this comment.
I think it would be good to create an additional struct here called FDInfo which contains pos, flags, and mnt_id since those fields appear to be consistently available. The FDInfo struct could contain a slice of InotifyStat containing the detailed information for inotify file descriptors.
And to be more consistent with the rest of the API, the file should be called proc_fdinfo.go.
| // InotifyStat represents a file descriptor's inotify watch count. | ||
| type InotifyStat struct { | ||
| // File descriptor | ||
| FD string |
There was a problem hiding this comment.
This could be moved into the FDInfo struct.
| // File descriptor | ||
| FD string | ||
| // List of inotify lines in the fdinfo file | ||
| Lines []string |
There was a problem hiding this comment.
Instead of just loading the line here, I think it would be better to parse the values into their expected fields/types. Or at least add the values to map for easy lookup.
| return p.fs.Path(append([]string{strconv.Itoa(p.PID)}, pa...)...) | ||
| } | ||
|
|
||
| func (p Proc) fileDescriptorInfo(fd string) ([]byte, error) { |
There was a problem hiding this comment.
This needs to capitalized to be available from outside of the procfs package. Also, I think this should return a FDInfo with the data already parsed, to make it easier to use.
|
Agree with pgier and don't have more to add. And sorry for not responding earlier! |
|
Hi folks, just pushed some updates based on your feedback. I replaced I'm sure this still needs some tweaks. Also, my go-foo isn't too good to begin with :) Hope it at least makes some sense! |
pirxthepilot
changed the title
|
@pirxthepilot Looks good, but would you mind rebasing on latest master? I think the staticcheck CI failures are related to an outdated Makefile or some other build config. |
|
@pgier whoops, sorry bout that! done! |
pgier
left a comment
pgier
left a comment
There was a problem hiding this comment.
@pirxthepilot Would you mind adding some basic tests before we merge?
|
@pirxthepilot I think this still needs unit tests before we merge. |
d97b4dd to
26c06f6
Compare
|
@pgier sorry for the delayed response. I pushed a new commit with the requested changes. Just want to run it by you as I start writing the tests. As for the tests, how should I go about adding fixtures? Do I just append data by hand in |
pgier
left a comment
pgier
left a comment
There was a problem hiding this comment.
Looking at this again, I noticed a couple more minor things that might be improved.
For updating the fixtures there is some info in README.md. Just extract the fixtures, make the updates you want, then pack them up again.
37bb438 to
1b9a28b
Compare
|
Changes made and tests written. I think the test in |
pgier
left a comment
pgier
left a comment
There was a problem hiding this comment.
Thanks for the update, tests look good. It looks like you are using an out of date fixtures.ttar, you may need to rebase on latest master to avoid conflict in the fixtures.
|
I think the fixtures issue is a separate issue and should be fixed by PR #185, so you can ignore that part. Edit: if you temporarily change the base branch of the PR and then change it back to master, it should update the diff to remove the bad fixtures stuff. |
Signed-off-by: Joon Guillen <joon@modulogeek.com>
Signed-off-by: Joon Guillen <joon@modulogeek.com>
Signed-off-by: Joon Guillen <joon@modulogeek.com>
Signed-off-by: Joon Guillen <joon@modulogeek.com>
b0fbe08 to
b6b6455
Compare
- Use slices instead of arrays to get FileDescriptorsInfo - Move regex declarations into global scope Signed-off-by: Joon Guillen <joon@modulogeek.com>
|
Updated! |
…us#115) Signed-off-by: Joon Guillen <joon@modulogeek.com>
|
Ug, sorry, the commit message lost the signoff when I did a squash merge. I had to amend the commit and do a force push in #189. |
…us#115) Signed-off-by: Joon Guillen <joon@modulogeek.com>
Add support for a bunch of low hanging fruits
3 participants
First attempt at adding functionality to support #107. Not quite sure if my approach here is correct - guidance/feedback would be very much appreciated. Also thought I'd let you guys review first before I write the tests.
Thanks!