code review 12561043: archive/tar: Fix support for long links and improve PAX...

archive/tar: Fix support for long links and improve PAX support.

The tar/archive code from golang has a problem with linknames with length >
100. A pax header is added but the original header still written with a too
long field length.

As it is clear that pax support is incomplete I have added missing
implementation parts.

This commit contains code from the golang project in the folder tar/archiv.

The following pax header records are now automatically written:

- gname)
- linkpath
- path
- uname

The following fields can be written with PAX, but the default is to use the
star binary extension.

- gid  (value > 2097151)
- size (value > 8589934591)
- uid (value > 2097151)

The string fields are written when the value is longer as the field or if the
string contains a char that is not encodable as 7-bit ASCII value.

The change was tested against a current ubuntu-cloud image tarball comparing
the compressed result.

+ added some automated tests for the new functionality.

Fixes issue 6056.

[mhennings, 2013-08-06 19:52:59]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
marco.hennings@freiheit.com),

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

[r, 2013-08-06 22:34:48]

R=adg

[adg, 2013-08-12 05:38:35]

R=dsymonds

He wrote this stuff.

[dsymonds, 2013-08-13 04:18:26]

A few places I've asked you to merge/revert things because the changes are
unnecessary. Please try to keep the diff minimal so it's easier to review and
there's less chance of an unexpected change sneaking in.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:180: PAX_ATIME    = "atime"
unless users actually need to use these constants (and I'm not sure they do),
unexport them.

Either way, ditch the underscore. This should either be paxAtime or PaxAtime.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:278: func isASCII7Bit(s string) bool {
isASCII is fine

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:279: for _, character := range s {
for _, c := range s {
  if c >= 0x80 {
    return false
  }
}
return true


and do similar below

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:287: func stripTo7Bits(s string) string {
toASCII

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:288: var buffer bytes.Buffer
buffer -> buf

also, this function is generating unnecessary garbage in the common case (all
ASCII). Start with
  if isASCII(s) {
    return s
  }

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:297: func stripTo7BitsAndShorten(s string, maxLen
int) string {
I don't think there's a need for this. Just use toASCII and take a subslice.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:27: errFieldTooLongNoAscii =
errors.New("archive/tar: header field too long or contains invalid values")
errInvalidHeader

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:84: func (tw *Writer) fillHeaderField(b []byte,
paxHeader map[string]string, paxKeyword string, s string) {
merge this into cString

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:124: func (tw *Writer) fillNumericHeaderField(b
[]byte, paxHeader map[string]string, paxKeyword string, x int64) {
merge these two into numeric and pass a bool called long.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:161: //  As this method is called internally by
writePax header it allows to
drop the space before "As"

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:332: func paxHeader(keyword string, value string)
string {
I don't see any merit in changing this function. It's the same as passing in
keyword+"="+value. Please revert.

[mhennings, 2013-08-13 12:56:23]

I've updated the code based on your comments. Please continue the review.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:180: PAX_ATIME    = "atime"
On 2013/08/13 04:18:26, dsymonds wrote:
> unless users actually need to use these constants (and I'm not sure they do),
> unexport them.
> 
> Either way, ditch the underscore. This should either be paxAtime or PaxAtime.

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:278: func isASCII7Bit(s string) bool {
On 2013/08/13 04:18:26, dsymonds wrote:
> isASCII is fine

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:279: for _, character := range s {
On 2013/08/13 04:18:26, dsymonds wrote:
> for _, c := range s {
>   if c >= 0x80 {
>     return false
>   }
> }
> return true
> 
> 
> and do similar below

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:287: func stripTo7Bits(s string) string {
On 2013/08/13 04:18:26, dsymonds wrote:
> toASCII

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:288: var buffer bytes.Buffer
On 2013/08/13 04:18:26, dsymonds wrote:
> buffer -> buf
> 
> also, this function is generating unnecessary garbage in the common case (all
> ASCII). Start with
>   if isASCII(s) {
>     return s
>   }

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:297: func stripTo7BitsAndShorten(s string, maxLen
int) string {
On 2013/08/13 04:18:26, dsymonds wrote:
> I don't think there's a need for this. Just use toASCII and take a subslice.

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:27: errFieldTooLongNoAscii =
errors.New("archive/tar: header field too long or contains invalid values")
On 2013/08/13 04:18:26, dsymonds wrote:
> errInvalidHeader

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:84: func (tw *Writer) fillHeaderField(b []byte,
paxHeader map[string]string, paxKeyword string, s string) {
On 2013/08/13 04:18:26, dsymonds wrote:
> merge this into cString

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:124: func (tw *Writer) fillNumericHeaderField(b
[]byte, paxHeader map[string]string, paxKeyword string, x int64) {
On 2013/08/13 04:18:26, dsymonds wrote:
> merge these two into numeric and pass a bool called long.

To be consistent with the naming on writePaxHeader I called the bool allowPax. I
hope this matches what you required good enough.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:161: //  As this method is called internally by
writePax header it allows to
On 2013/08/13 04:18:26, dsymonds wrote:
> drop the space before "As"

Done.

https://codereview.appspot.com/12561043/diff/12001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:332: func paxHeader(keyword string, value string)
string {
On 2013/08/13 04:18:26, dsymonds wrote:
> I don't see any merit in changing this function. It's the same as passing in
> keyword+"="+value. Please revert.

Done.

[dsymonds, 2013-08-15 01:06:32]

getting close. thanks for this.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:295: buf.WriteRune(c)
here you know it's ASCII, so use buf.WriteByte(byte(c))

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:71: func (tw *Writer) cString(b []byte, s string,
paxHeader map[string]string, paxKeyword string, allowPax bool) {
put the allowPax arg before paxHeader and paxKeyword so it's clearer that if
it's false the remaining args will be ignored. Put paxKeyword next, and
paxHeader last.

func (tw *Writer) cString(b []byte, s string, allowPax bool, paxKeyword string,
paxHeader map[string]string)

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:143: // As this method is called internally by
writePax header it allows to
s/it allows to/to allow it to/

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:157: paxHeaderRecords := make(map[string]string)
paxHeader is the name for this map elsewhere, so rename this var.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:240: if allowPax {
if !allowPax {
  return errInvalidHeader
}
if err := tw.writePAXHeader(...); err != nil {
  return err
}

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:249: tw.pad = -tw.nb & (blockSize - 1) //
blockSize is a power of two
can you rephrase this so it doesn't rely on integer two's complement properties?

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:278: func (tw *Writer) writePAXHeader(hdr *Header,
paxHeaderRecords map[string]string) error {
paxHeader

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:293: if len(ascii) > 100 {
should this really be silently truncating the name?

[mhennings, 2013-08-15 09:52:45]

I have updated changeset based on your comments.

Please continue the review.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:295: buf.WriteRune(c)
On 2013/08/15 01:06:33, dsymonds wrote:
> here you know it's ASCII, so use buf.WriteByte(byte(c))

Done.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:71: func (tw *Writer) cString(b []byte, s string,
paxHeader map[string]string, paxKeyword string, allowPax bool) {
On 2013/08/15 01:06:33, dsymonds wrote:
> put the allowPax arg before paxHeader and paxKeyword so it's clearer that if
> it's false the remaining args will be ignored. Put paxKeyword next, and
> paxHeader last.
> 
> func (tw *Writer) cString(b []byte, s string, allowPax bool, paxKeyword
string,
> paxHeader map[string]string)

Done.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:143: // As this method is called internally by
writePax header it allows to
On 2013/08/15 01:06:33, dsymonds wrote:
> s/it allows to/to allow it to/

Done.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:157: paxHeaderRecords := make(map[string]string)
On 2013/08/15 01:06:33, dsymonds wrote:
> paxHeader is the name for this map elsewhere, so rename this var.

There is a nameconflict with the function paxHeader. So i've renamed all
occurances to paxHeaders.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:240: if allowPax {
On 2013/08/15 01:06:33, dsymonds wrote:
> if !allowPax {
>   return errInvalidHeader
> }
> if err := tw.writePAXHeader(...); err != nil {
>   return err
> }

Done.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:249: tw.pad = -tw.nb & (blockSize - 1) //
blockSize is a power of two
On 2013/08/15 01:06:33, dsymonds wrote:
> can you rephrase this so it doesn't rely on integer two's complement
properties?

I have moved the line from above, as it is neccessary to calculate the padding
after writing the pax header. 

But i agree that is strange to see an optimization like this. I've replaced it
with a simpler to understand term.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:278: func (tw *Writer) writePAXHeader(hdr *Header,
paxHeaderRecords map[string]string) error {
On 2013/08/15 01:06:33, dsymonds wrote:
> paxHeader

Done.

https://codereview.appspot.com/12561043/diff/19001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer.go:293: if len(ascii) > 100 {
On 2013/08/15 01:06:33, dsymonds wrote:
> should this really be silently truncating the name?

yes, this is the same behaviour that gnu tar provides.

The name of the pax header is not relevant for the name of the extracted file,
as it is stored in the pax header content.

[dsymonds, 2013-08-16 04:46:18]

LGTM

[dsymonds, 2013-08-16 04:47:25]

This is ready to go in; you just need to sign the CLA:
  http://golang.org/doc/contribute.html#copyright

[mhennings, 2013-08-16 12:12:00]

On 2013/08/16 04:47:25, dsymonds wrote:
> This is ready to go in; you just need to sign the CLA:
>   http://golang.org/doc/contribute.html#copyright

Sounds good.

I had allready signed when i first uploaded the review. To be sure I just signed
again.

[dsymonds, 2013-08-16 12:30:16]

I checked before I asked (but can't check right now). Did you sign the CLA
with the same email address that you have used to mail this CL?

[mhennings, 2013-08-16 12:34:39]

On 2013/08/16 12:30:16, dsymonds wrote:
> I checked before I asked (but can't check right now). Did you sign the CLA
> with the same email address that you have used to mail this CL?


Yes, as an individual contributor using the online form.

But maybe i did mistype. If so the second one should be fine.

[dsymonds, 2013-08-19 00:45:50]

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

archive/tar: Fix support for long links and improve PAX support.

The tar/archive code from golang has a problem with linknames with length >
100. A pax header is added but the original header still written with a too
long field length.

As it is clear that pax support is incomplete I have added missing
implementation parts.

This commit contains code from the golang project in the folder tar/archiv.

The following pax header records are now automatically written:

- gname)
- linkpath
- path
- uname

The following fields can be written with PAX, but the default is to use the
star binary extension.

- gid  (value > 2097151)
- size (value > 8589934591)
- uid (value > 2097151)

The string fields are written when the value is longer as the field or if the
string contains a char that is not encodable as 7-bit ASCII value.

The change was tested against a current ubuntu-cloud image tarball comparing
the compressed result.

+ added some automated tests for the new functionality.

Fixes issue 6056.

R=dsymonds
CC=golang-dev
https://codereview.appspot.com/12561043

Committer: David Symonds <dsymonds@golang.org>