code review 64740043: archive/tar: Added support for GNU sparse files.

archive/tar: Added support for GNU sparse files. Supports all the current GNU tar sparse formats, including the old GNU format and the GNU PAX format versions 0.0, 0.1, and 1.0. Fixes issue 3864.

[davidthomas, 2014-02-16 22:09:00]

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[dave_cheney.net, 2014-02-16 23:49:11]

Thanks for working on this, I haven't made it all the way through the file yet. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader.go File src/pkg/archive/tar/reader.go (right): https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:187: var sp []sparseEntry var err error https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:190: fallthrough delete https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:191: case "0.1": case "0.0", "0.1": https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:192: var err error sp, err = readGNUSparceMap0x1(headers) https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:195: return nil, err delete error handling https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:201: return nil, err same https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:204: return sp, nil always return sp, err. The caller cannot assume sp is valid without checking err https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:516: rfr := &regFileReader{r: tr.r, nb: nb} why create a new var here ? why not tr.curr = &regFileReader{....} https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:527: tr.err = ErrHeader if tr.err isn't nil, the overwrite the error ? that doesn't sound right. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:531: tr.curr = &sparseFileReader{rfr: rfr, sp: sp, tot: hdr.Size} then it is clear that you are wrapping the old reader tr.curr = &sparceFileReader{rfr: tr.curr, ...} https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:548: func (tr *Reader) readOldGNUSparseMap(header []byte) []sparseEntry { magic numbers, 4, 12, 31, 482 and the supplementary, 504. Can these be constants with references to their definition please. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:591: sparseHeader := buf[:blockSize] did you mean sparseHeader := make([]byte, blocksize, 2*blocksize) ? https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:608: sparseHeader = buf[:newLen] you can reslice beyond len(slice) provided it is below cap(slice)

[davidthomas, 2014-02-17 01:33:45]

https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader.go File src/pkg/archive/tar/reader.go (right): https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:187: var sp []sparseEntry On 2014/02/16 23:49:11, dfc wrote: > var err error Done. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:190: fallthrough On 2014/02/16 23:49:11, dfc wrote: > delete Done. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:191: case "0.1": On 2014/02/16 23:49:11, dfc wrote: > case "0.0", "0.1": Done. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:192: var err error On 2014/02/16 23:49:11, dfc wrote: > sp, err = readGNUSparceMap0x1(headers) Done. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:195: return nil, err On 2014/02/16 23:49:11, dfc wrote: > delete error handling Done. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:201: return nil, err On 2014/02/16 23:49:11, dfc wrote: > same Done. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:204: return sp, nil On 2014/02/16 23:49:11, dfc wrote: > always return sp, err. The caller cannot assume sp is valid without checking err Done. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:527: tr.err = ErrHeader On 2014/02/16 23:49:11, dfc wrote: > if tr.err isn't nil, the overwrite the error ? that doesn't sound right. I intended this to be for the same reason as on line 505 (which I didn't write). But you're right, it's not really appropriate here since it's after a method call that may have set it. I could fix up the error inside the method instead, but not after the call. I'll fix this. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:531: tr.curr = &sparseFileReader{rfr: rfr, sp: sp, tot: hdr.Size} On 2014/02/16 23:49:11, dfc wrote: > then it is clear that you are wrapping the old reader > tr.curr = &sparceFileReader{rfr: tr.curr, ...} The types don't line up. It would have to be tr.curr = &sparseFileReader{rfr: tr.curr.(*regFileReader), ...} Alternatively, sparseFileReader could have a numBytesReader (interface) field instead of a regFileReader field. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:548: func (tr *Reader) readOldGNUSparseMap(header []byte) []sparseEntry { On 2014/02/16 23:49:11, dfc wrote: > magic numbers, 4, 12, 31, 482 and the supplementary, 504. Can these be constants > with references to their definition please. I was following the style used in the existing code. I'll go ahead and change this, though. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:591: sparseHeader := buf[:blockSize] On 2014/02/16 23:49:11, dfc wrote: > did you mean > > sparseHeader := make([]byte, blocksize, 2*blocksize) ? buf is reused later. The idea is that we never need more than two blocks at a time. But we may read more than two blocks. So rather than allocating more and more blocks, just reuse the buffer we started with. Throughout this function, sparseHeader refers to various slices of buf, but buf is always the whole two-block buffer. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:608: sparseHeader = buf[:newLen] On 2014/02/16 23:49:11, dfc wrote: > you can reslice beyond len(slice) provided it is below cap(slice) But this isn't below cap(slice). That's what makes this case different from the next. Reslicing here would panic. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:643: // The first line contains Oops. I forgot to finish typing this comment.

[davidthomas, 2014-02-17 14:04:22]

https://codereview.appspot.com/64740043/diff/80001/src/pkg/archive/tar/reader.go File src/pkg/archive/tar/reader.go (right): https://codereview.appspot.com/64740043/diff/80001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:111: mergePAX(hdr, headers) The call to tr.skipUnread() or the call to tr.readHeader() could set tr.err and cause hdr to be nil. This would likely make mergePAX panic. This is technically a different bug entirely, but I was really tempted to fix it since I add code directly after it. Should I open a different issue for this?

[davidthomas, 2014-02-17 21:09:09]

https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader.go File src/pkg/archive/tar/reader.go (right): https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:516: rfr := &regFileReader{r: tr.r, nb: nb} On 2014/02/16 23:49:11, dfc wrote: > why create a new var here ? why not > > tr.curr = &regFileReader{....} Done. https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:531: tr.curr = &sparseFileReader{rfr: rfr, sp: sp, tot: hdr.Size} On 2014/02/17 01:33:46, davidthomas wrote: > On 2014/02/16 23:49:11, dfc wrote: > > then it is clear that you are wrapping the old reader > > tr.curr = &sparceFileReader{rfr: tr.curr, ...} > The types don't line up. It would have to be > tr.curr = &sparseFileReader{rfr: tr.curr.(*regFileReader), ...} > Alternatively, sparseFileReader could have a numBytesReader (interface) field > instead of a regFileReader field. Done.

[davidthomas, 2014-02-17 21:14:05]

https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader.go File src/pkg/archive/tar/reader.go (right): https://codereview.appspot.com/64740043/diff/60001/src/pkg/archive/tar/reader... src/pkg/archive/tar/reader.go:548: func (tr *Reader) readOldGNUSparseMap(header []byte) []sparseEntry { On 2014/02/16 23:49:11, dfc wrote: > magic numbers, 4, 12, 31, 482 and the supplementary, 504. Can these be constants > with references to their definition please. Done.

[davidthomas, 2014-02-17 21:14:22]

Hello golang-codereviews@googlegroups.com, dave@cheney.net (cc: golang-codereviews@googlegroups.com), Please take another look.

[gobot, 2014-02-18 00:57:24]

R=golang-dev (assigned by dave@cheney.net)

[davidthomas, 2014-02-21 04:36:07]

Hello golang-codereviews@googlegroups.com, dave@cheney.net, gobot@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

[gobot, 2014-02-24 15:10:16]

R=dsymonds@golang.org (assigned by rsc@golang.org)

[gobot, 2014-03-04 05:13:12]

R=dave@cheney.net (assigned by dsymonds@golang.org)

[dsymonds, 2014-03-04 05:14:13]

I'm not sure I follow what's going on with sparse files here. Mr Cheney, want to take this over?

[gobot, 2014-03-17 13:20:26]

R=dave@cheney.net (assigned by LeoLiu.PKU@gmail.com)

[gobot, 2014-04-02 08:01:44]

R=golang-dev (assigned by dave@cheney.net)

[rsc, 2014-04-03 19:58:28]

LGTM

[rsc, 2014-04-03 20:01:15]

*** Submitted as https://code.google.com/p/go/source/detail?r=11dcaf7474d7 *** archive/tar: add support for GNU sparse files. Supports all the current GNU tar sparse formats, including the old GNU format and the GNU PAX format versions 0.0, 0.1, and 1.0. Fixes issue 3864. LGTM=rsc R=golang-codereviews, dave, gobot, dsymonds, rsc CC=golang-codereviews https://codereview.appspot.com/64740043 Committer: Russ Cox <rsc@golang.org>

[gobot, 2014-04-03 22:57:02]

This CL appears to have broken the darwin-amd64-race-cheney builder. See http://build.golang.org/log/72bb0efbfafb1d203bac1ad749858746a3a6da59