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