Added general file descriptor logic and implemented for linux #30
Conversation
kinghrothgar
changed the title
Current coverage is 56.95%@@ master #30 diff @@
==========================================
Files 9 6 -3
Lines 1025 525 -500
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 733 299 -434
+ Misses 238 205 -33
+ Partials 54 21 -33
|
| if len(fields) == 6 { | ||
| self.SoftLimit, _ = strconv.ParseUint(fields[3], 10, 64) | ||
| self.HardLimit, _ = strconv.ParseUint(fields[4], 10, 64) | ||
| return false |
There was a problem hiding this comment.
I think this return false should be moved outside and after this if block. After finding Max open files you want to stop reading the file regardless of success/failure in parsing this line.
|
The changes look good, they just need some tests. For your questions:
|
kinghrothgar
force-pushed
the
addFileDescriptorUsage
branch
3 times, most recently
from
73318fd to
70932cd
Compare
kinghrothgar
changed the title
|
Alright, I believe I did everything needed for merge. Two things I'm a little unsure of:
|
| fdUsage, err := concreteSigar.GetFDUsage() | ||
| // if it's not implemented, don't test | ||
| if _, ok := err.(*sigar.ErrNotImplemented); ok { | ||
| println("Skipping *ConcreteSigar.GetFDUsage test because it is no implemented for " + runtime.GOOS) |
There was a problem hiding this comment.
"no implemented" -> "not implemented"
There was a problem hiding this comment.
Instead of println, you can you testing.Skipf().
|
The new changes look good. I just left some cosmetic comments. I think the location of new error is good. And the refactoring (#31) of the existing methods to use the new error is a larger task that someone (probably myself) can tackle in the future. Thanks! |
kinghrothgar
force-pushed
the
addFileDescriptorUsage
branch
from
70932cd to
6654280
Compare
|
All commented items fixed. |
Add support for #29.
I have implemented the initial logic for Linux and just wanted to get some input to make sure I'm on the right track before I move on to attempting some of the other OSes. A few things of note / questions:
/proc/{PID}/fdfails which is will for root processes if run as not root. Currently, it just returns 0 for the open count. I think this maybe the best route, but I am not sure.