catchpointdump: fix panic when processing gzip files#5210
Conversation
| if isCompressed { | ||
| // gzip'ed file is about 3-6 times smaller than tar | ||
| // modify catchpointFileSize to make the progress bar more-less reflecting the state | ||
| catchpointFileSize = 3 * catchpointFileSize |
There was a problem hiding this comment.
or you could change the calculation of progress below to be
if isCompressed {progress, _ := catchpointFile.Seek(0, io.SeekCurrent)}
There was a problem hiding this comment.
seek most likely will be a system call, not good.
There was a problem hiding this comment.
You could use your estimate to choose a few good times to update the actual progress
There was a problem hiding this comment.
btw, it does not look Seek is available on any readers. Anyway, this a debugging tool and I guess the size adjustment for progress estimation is fine
Codecov Report
@@ Coverage Diff @@
## master #5210 +/- ##
==========================================
+ Coverage 52.48% 53.55% +1.07%
==========================================
Files 441 441
Lines 55098 55098
==========================================
+ Hits 28917 29510 +593
+ Misses 23830 23305 -525
+ Partials 2351 2283 -68 see 62 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
algonautshant
left a comment
algonautshant
left a comment
There was a problem hiding this comment.
Great!
Thanks for the changes.
| } | ||
| if isCompressed { | ||
| // gzip'ed file is about 3-6 times smaller than tar | ||
| // modify catchpointFileSize to make the progress bar more-less reflecting the state |
There was a problem hiding this comment.
very small nit: *more-or-less
| } | ||
|
|
||
| func getCatchpointTarReader(catchpointReader *bufio.Reader, catchpointFileSize int64) (*tar.Reader, error) { | ||
| func getCatchpointTarReader(catchpointReader *bufio.Reader, catchpointFileSize int64) (*tar.Reader, bool, error) { |
There was a problem hiding this comment.
this bool just represents if the reader is a compressed reader? could use a comment to help point that out
AlgoAxel
left a comment
AlgoAxel
left a comment
There was a problem hiding this comment.
nothing blocking from me
4 participants
Summary
Fix a regression introduced in #4743. Accumulated (read so far) size might exceed the file size that causes the progress printer to crash.
Test Plan
Tested manually