code review 110480043: archive/tar: fix writing of pax headers

archive/tar: fix writing of pax headers

"archive/tar: reuse temporary buffer in writeHeader" introduced a
change which was supposed to help lower the number of allocations from
512 bytes for every call to writeHeader. This change broke the writing
of PAX headers.

writeHeader calls writePAXHeader and writePAXHeader calls writeHeader
again. writeHeader will end up writing the PAX header twice.

example broken header:
PaxHeaders.4007/NetLock_Arany_=Class_Gold=_Ftanstvny.crt0000000000000000000000000000007112301216634021512 xustar0000000000000000
PaxHeaders.4007/NetLock_Arany_=Class_Gold=_Ftanstvny.crt0000000000000000000000000000007112301216634021512 xustar0000000000000000

example correct header:
PaxHeaders.4290/NetLock_Arany_=Class_Gold=_Ftanstvny.crt0000000000000000000000000000007112301216634021516 xustar0000000000000000
0100644000000000000000000000270412301216634007250 0ustar0000000000000000

This commit adds a dedicated buffer for pax headers to the Writer
struct. This change increases the size of the struct by 512 bytes, but
allows tar/writer to avoid allocating 512 bytes for all written
headers and it avoids allocating 512 more bytes for pax headers.

[unclejack, 2014-07-10 15:07:17]

Hello,

This CL fixes a problem introduced by a CL I've sent before.

Unfortunately, this problem wasn't picked up by the tests after I've switched it
from sync.Pool to a buffer to be kept in the struct.

writeHeader is calling writePAXHeader and writePAXHeader ends up calling
writeHeader again. This was overwriting tw.hdrBuff.

This CL adds a similar paxHdrBuff buffer to be used when writing PAX headers.

I can update this CL to do any of the following if adding paxHdrBuff isn't
desired:
1) revert the hdrBuff change
2) keep hdrBuff and allocate a slice of bytes when writing the PAX header

[iant, 2014-07-10 15:42:30]

Is it possible to write a test for this?

[unclejack, 2014-07-10 15:50:41]

I'll look into it. You'll receive another notification when I've added a test.

On 2014/07/10 15:42:30, iant wrote:
> Is it possible to write a test for this?

[unclejack, 2014-07-11 20:08:44]

You'll find a test in the latest patch set. This test checks if the Typeflag is
the expected one.

[unclejack, 2014-07-12 08:47:52]

Hello dsymonds@golang.org, dave@cheney.net, iant@golang.org (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[dsymonds, 2014-07-13 23:30:41]

https://codereview.appspot.com/110480043/diff/80001/src/pkg/archive/tar/write...
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/110480043/diff/80001/src/pkg/archive/tar/write...
src/pkg/archive/tar/writer.go:169: header = tw.paxHdrBuff[:]
this looks backwards; if PAX is not allowed you are using the PAX buffer?

https://codereview.appspot.com/110480043/diff/80001/src/pkg/archive/tar/write...
src/pkg/archive/tar/writer.go:170: copy(header, zeroBlock)
factor the copy out of the conditional

[unclejack, 2014-07-14 14:25:52]

I've updated the patch. You'll find some new comments in the revised patch.
These should explain we're actually writing PAX headers when allowPax is false.

https://codereview.appspot.com/110480043/diff/80001/src/pkg/archive/tar/write...
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/110480043/diff/80001/src/pkg/archive/tar/write...
src/pkg/archive/tar/writer.go:169: header = tw.paxHdrBuff[:]
On 2014/07/13 23:30:40, dsymonds wrote:
> this looks backwards; if PAX is not allowed you are using the PAX buffer?

For the first call to writeHeader, we allow writeHeader to write PAX headers
(allowPax == true). allowPax will be true when WriteHeader calls writeHeader, so
writeHeader will generate and write PAX headers if they're required.

However, if writePAXHeader has been called by writeHeader because some PAX
headers had to be written, writePAXHeader will call writeHeader and it won't
allow PAX headers to be generated again in writeHeader (allowPax == false).

allowPax needs to be false when writing PAX headers, otherwise the resulting
writes are invalid.

We're using the paxHdrBuff slice to avoid overwriting the regular header with
the PAX header.

https://codereview.appspot.com/110480043/diff/80001/src/pkg/archive/tar/write...
src/pkg/archive/tar/writer.go:170: copy(header, zeroBlock)
On 2014/07/13 23:30:40, dsymonds wrote:
> factor the copy out of the conditional

Done.

[unclejack, 2014-07-15 21:22:32]

Hello,

Is there anything I could do to help with the review of this CL?

[dsymonds, 2014-07-15 23:25:16]

On 16 July 2014 07:22,  <unclejacksons@gmail.com> wrote:

> Is there anything I could do to help with the review of this CL?

Just patience in waiting for us to get back to it.

[dsymonds, 2014-07-16 01:54:43]

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
src/pkg/archive/tar/writer.go:167: // use paxHdrBuff when writing PAX headers to
avoid overwriting the other header
Okay, I understand the issue now. This comment needs a bit more work though. We
can also slightly restructure the logic to make it clearer which is the "normal"
path, and which is the special PAX path.


// We need to select which scratch buffer to use carefully,
// since this method is called recursively to write PAX headers.
// If allowPax is true, this is the non-recursive call, and we will use hdrBuff.
// If allowPax is false, we are being called by writePAXHeader, and hdrBuff is
// already being used by the non-recursive call, so we must use paxHdrBuff.
header := tw.hdrBuff[:]
if !allowPax {
  header = tw.paxHdrBuff[:]
}

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
src/pkg/archive/tar/writer.go:178: // the slicer is used to specify the target
ranges of the writes to be made to the header (currently empty)
drop this comment. It isn't giving us anything.

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
File src/pkg/archive/tar/writer_test.go (right):

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
src/pkg/archive/tar/writer_test.go:488: t.Fatalf("Typeflag should've been 0,
found %d\n", header.Typeflag)
drop \n

[unclejack, 2014-07-16 08:12:47]

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
src/pkg/archive/tar/writer.go:167: // use paxHdrBuff when writing PAX headers to
avoid overwriting the other header
On 2014/07/16 01:54:42, dsymonds wrote:
> Okay, I understand the issue now. This comment needs a bit more work though.
We
> can also slightly restructure the logic to make it clearer which is the
"normal"
> path, and which is the special PAX path.
> 
> 
> // We need to select which scratch buffer to use carefully,
> // since this method is called recursively to write PAX headers.
> // If allowPax is true, this is the non-recursive call, and we will use
hdrBuff.
> // If allowPax is false, we are being called by writePAXHeader, and hdrBuff is
> // already being used by the non-recursive call, so we must use paxHdrBuff.
> header := tw.hdrBuff[:]
> if !allowPax {
>   header = tw.paxHdrBuff[:]
> }

Done.

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
src/pkg/archive/tar/writer.go:178: // the slicer is used to specify the target
ranges of the writes to be made to the header (currently empty)
On 2014/07/16 01:54:43, dsymonds wrote:
> drop this comment. It isn't giving us anything.

Done.

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
File src/pkg/archive/tar/writer_test.go (right):

https://codereview.appspot.com/110480043/diff/120001/src/pkg/archive/tar/writ...
src/pkg/archive/tar/writer_test.go:488: t.Fatalf("Typeflag should've been 0,
found %d\n", header.Typeflag)
On 2014/07/16 01:54:43, dsymonds wrote:
> drop \n

Done.

[dsymonds, 2014-07-16 08:49:28]

LGTM

I'll submit this tomorrow if no-one raises any objections. Thanks for doing
this.

[dsymonds, 2014-07-17 00:00:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=1b17b3426e3c ***

archive/tar: fix writing of pax headers

"archive/tar: reuse temporary buffer in writeHeader" introduced a
change which was supposed to help lower the number of allocations from
512 bytes for every call to writeHeader. This change broke the writing
of PAX headers.

writeHeader calls writePAXHeader and writePAXHeader calls writeHeader
again. writeHeader will end up writing the PAX header twice.

example broken header:
PaxHeaders.4007/NetLock_Arany_=Class_Gold=_Ftanstvny.crt0000000000000000000000000000007112301216634021512
xustar0000000000000000
PaxHeaders.4007/NetLock_Arany_=Class_Gold=_Ftanstvny.crt0000000000000000000000000000007112301216634021512
xustar0000000000000000

example correct header:
PaxHeaders.4290/NetLock_Arany_=Class_Gold=_Ftanstvny.crt0000000000000000000000000000007112301216634021516
xustar0000000000000000
0100644000000000000000000000270412301216634007250 0ustar0000000000000000

This commit adds a dedicated buffer for pax headers to the Writer
struct. This change increases the size of the struct by 512 bytes, but
allows tar/writer to avoid allocating 512 bytes for all written
headers and it avoids allocating 512 more bytes for pax headers.

LGTM=dsymonds
R=dsymonds, dave, iant
CC=golang-codereviews
https://codereview.appspot.com/110480043

Committer: David Symonds <dsymonds@golang.org>

[gobot, 2014-07-17 02:03:58]

This CL appears to have broken the plan9-386-cnielsen builder.
See http://build.golang.org/log/881aabe9a7d0879310a5cc4f26eade3f1c396041

[unclejack, 2014-10-01 15:53:34]

*** Abandoned ***