fd leak and error handling#33713
Conversation
There was a problem hiding this comment.
I feel like this should be run at package init.
There was a problem hiding this comment.
@cpuguy83 Can you explain explicitly? I'm afraid I did't get your point of package init :(
There was a problem hiding this comment.
@x1022as Because this is effectively a package level constant.
There was a problem hiding this comment.
Does this one need an error check as well?
There was a problem hiding this comment.
I think errors should be ignored and continue the for loop, so maybe no need to check here.
|
janky failed seems not related. will push again to re-run it. |
Signed-off-by: Deng Guangxing <[email protected]>
|
ping @aaronlehmann PTAL |
| tmpOffset += s | ||
| } | ||
| r.Seek(tmpOffset+offset, os.SEEK_SET) | ||
| if _, err := r.Seek(tmpOffset+offset, os.SEEK_SET); err != nil { |
There was a problem hiding this comment.
Rather than calling Seek recursively here, which seems to do a lot of redundant work, couldn't we just set r.pos directly?
There was a problem hiding this comment.
@aaronlehmann @vieux if this is an important fix, and we want it to be in 17.07, perhaps we should carry to make sure it gets merged in time
|
This LGTM as-is, however I suggested a simplification for the existing code. The simplification could happen in a separate PR. |
6 participants
Signed-off-by: Deng Guangxing [email protected]
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)