code review 162047: Minor fixes for tar/archive.

Minor fixes for tar/archive.

1. If all data is exhausted using .Read() then a following
   .Next() will fail as if it saw EOF.

   A test case for this was added.  It verifies the data using MD5.

2. The 'Seeker' check isn't robust so we get seek failures on
   non-seekable descriptors.  Fix to fallback to doing a read.

   A test case for this was added.

3. It's common (normal?) to get numerous 'empty' Headers at
   the end of a tar file from .Next() before we get an EOF.
   Suppress these.

   One of the test cases (TestReader) was modified to
   accommodate this.

[_cw_, 2009-11-28 01:35:47]

Some minor changes to archive/tar/reader.go

Arguably, these changes could be be broken up into separate issues.

[_cw_, 2009-11-28 02:02:06]

Hello r, rsc,

I'd like you to review the following change.

[rsc, 2009-11-30 00:01:41]

Thanks for tracking these down.
I think the fix might be slightly
different, as noted below.

I think once we see two empty
headers, we should stop reading
the tar file instead of continuing on
to look for EOF.

Looping in dsymonds, who wrote
the original code.

http://codereview.appspot.com/162047/diff/1001/1002
File src/pkg/archive/tar/reader.go (right):

http://codereview.appspot.com/162047/diff/1001/1002#newcode53
src/pkg/archive/tar/reader.go:53: // loop until we get a usable header or error
This shouldn't be necessary: the assumption is that
if tr.readHeader returns hdr == nil, then it has set
tr.err != nil.  This appears not to be true, but then the
fix would be in readHeader.

http://codereview.appspot.com/162047/diff/1001/1002#newcode104
src/pkg/archive/tar/reader.go:104: consumewithread := true;
s/consumewithread/doCopyn/

http://codereview.appspot.com/162047/diff/1001/1002#newcode146
src/pkg/archive/tar/reader.go:146: return nil;
I think this should say tr.err = os.EOF; return nil;.

http://codereview.appspot.com/162047/diff/1001/1002#newcode231
src/pkg/archive/tar/reader.go:231: // EOF on entry is not EOF on entire tar file
Why not?

http://codereview.appspot.com/162047/diff/1001/1003
File src/pkg/archive/tar/reader_test.go (right):

http://codereview.appspot.com/162047/diff/1001/1003#newcode15
src/pkg/archive/tar/reader_test.go:15: "fmt";
Please move up to keep list sorted.

[dsymonds, 2009-11-30 01:41:42]

http://codereview.appspot.com/162047/diff/1001/1002
File src/pkg/archive/tar/reader.go (right):

http://codereview.appspot.com/162047/diff/1001/1002#newcode110
src/pkg/archive/tar/reader.go:110: // Seeker failed; fallback to read
I'm not sure we want to do this. If the seek failed then we should consider the
input stream to be "broken" (just like a read failing). Do you have a situation
in mind for when a seek fails and the io.Reader is still readable?

Also, the Seek returns a count; you should probably compensate for that if it's
a partial seek.

[rsc, 2009-11-30 02:12:22]

Yes, you can't seek on pipes but they have a Seek
method because they are ordinary file descriptors.


On Sun, Nov 29, 2009 at 17:41,  <dsymonds@golang.org> wrote:
>
> http://codereview.appspot.com/162047/diff/1001/1002
> File src/pkg/archive/tar/reader.go (right):
>
> http://codereview.appspot.com/162047/diff/1001/1002#newcode110
> src/pkg/archive/tar/reader.go:110: // Seeker failed; fallback to read
> I'm not sure we want to do this. If the seek failed then we should
> consider the input stream to be "broken" (just like a read failing). Do
> you have a situation in mind for when a seek fails and the io.Reader is
> still readable?
>
> Also, the Seek returns a count; you should probably compensate for that
> if it's a partial seek.
>
> http://codereview.appspot.com/162047
>

[rsc, 2009-11-30 02:13:04]

Also, seek returns an absolute offset, not a count,
so it should probably be ignored if there is an error.
The new code works on pipes (there is a test)
so I'm happy with the semantics.

[_cw_, 2009-11-30 06:22:58]

updated comments, updated code to follow shortly

rsc:

"The new code works on pipes (there is a test)
so I'm happy with the semantics"

what do you mean by that?  i checked for updates and there is no seek test other
than what i offered?  is there something unmerged i'm missing?

http://codereview.appspot.com/162047/diff/1001/1002
File src/pkg/archive/tar/reader.go (right):

http://codereview.appspot.com/162047/diff/1001/1002#newcode53
src/pkg/archive/tar/reader.go:53: // loop until we get a usable header or error
On 2009/11/30 00:01:41, rsc wrote:
> This shouldn't be necessary: the assumption is that
> if tr.readHeader returns hdr == nil, then it has set
> tr.err != nil.  This appears not to be true, but then the
> fix would be in readHeader.

i agree, logically readHeader should just return sane headers.  let me test that
with the other changes.

http://codereview.appspot.com/162047/diff/1001/1002#newcode104
src/pkg/archive/tar/reader.go:104: consumewithread := true;
On 2009/11/30 00:01:41, rsc wrote:
> s/consumewithread/doCopyn/

will do

http://codereview.appspot.com/162047/diff/1001/1002#newcode110
src/pkg/archive/tar/reader.go:110: // Seeker failed; fallback to read
On 2009/11/30 01:41:42, dsymonds1 wrote:
> I'm not sure we want to do this. If the seek failed then we should consider
the
> input stream to be "broken" (just like a read failing). Do you have a
situation
> in mind for when a seek fails and the io.Reader is still readable?

the seek can fail if the input comes from a pipe or a socket

arguable we should probably check for ESPIPE to be sure in those cases, that
seems fair

something like

  if err2, ok := e.(*os.PathError); ok && err2.Error == os.ESPIPE {
...
}

might work; I'll have to test it 

> Also, the Seek returns a count; you should probably compensate for that if
it's
> a partial seek.

is a partial seek ever possible?  i've never seen that

i don't mind a fix for this, but it should be done separately surely?

http://codereview.appspot.com/162047/diff/1001/1002#newcode146
src/pkg/archive/tar/reader.go:146: return nil;
On 2009/11/30 00:01:41, rsc wrote:
> I think this should say tr.err = os.EOF; return nil;.

let me test that, there is another bug i have here (when the source is
bytes.Buffer{} and we're at the end) that i want to look into and this might
help that

http://codereview.appspot.com/162047/diff/1001/1002#newcode231
src/pkg/archive/tar/reader.go:231: // EOF on entry is not EOF on entire tar file
On 2009/11/30 00:01:41, rsc wrote:
> Why not?

i'm not sure what you're asking here

should the comment be more clear?

i'm saying that trdr.Read(...) will return EOF once you've consumed the entire
'item' from the tar file, so that's EOF

however, the tar reader itself is NOT at eof, setting it so means trdr.Next()
won't get the next header

http://codereview.appspot.com/162047/diff/1001/1003
File src/pkg/archive/tar/reader_test.go (right):

http://codereview.appspot.com/162047/diff/1001/1003#newcode15
src/pkg/archive/tar/reader_test.go:15: "fmt";
On 2009/11/30 00:01:41, rsc wrote:
> Please move up to keep list sorted.
> 

sorry, that was an oversight, normally i do (maybe i'll make a note that gofmt
could sort for us at some point)

[rsc, 2009-11-30 07:27:27]

On Sun, Nov 29, 2009 at 22:22,  <chris.wedgwood@gmail.com> wrote:
> updated comments, updated code to follow shortly
>
> rsc:
>
> "The new code works on pipes (there is a test)
> so I'm happy with the semantics"
>
> what do you mean by that?  i checked for updates and there is no seek
> test other than what i offered?  is there something unmerged i'm
> missing?

this was in defense of your changes, replying to
comments by david symonds.

russ

[_cw_, 2009-11-30 09:51:42]

*** Abandoned ***