code review 7305072: archive/tar: add Header.FileInfo method. Add more cases...

archive/tar: add Header.FileInfo method. Add more cases to FileInfoHeader.

FileInfoHeader can now handle fifo, setuid, setgid and sticky bits.

Fixes issue 4695.

[u, 2013-02-09 20:21:11]

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

https://codereview.appspot.com/7305072/diff/2001/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:83: mode |= os.ModeNamedPipe
Could someone please verify that os.ModeNamedPipe is correct for c_ISFIFO.

https://codereview.appspot.com/7305072/diff/2001/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:85: // regular file
Should we remove this nop case?

https://codereview.appspot.com/7305072/diff/2001/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:99: // regular file
Should we remove nop cases ('0', '\x00', '7', 'x', 'g')?

https://codereview.appspot.com/7305072/diff/2001/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:102: mode |= os.ModeSymlink
Is there a way to differentiate a hard link from a symbolic link using
os.FileMode? Is os.ModeSymlink correct for hard links?

https://codereview.appspot.com/7305072/diff/2001/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:115: mode |= os.ModeNamedPipe
Could someone please verify that os.ModeNamedPipe is correct for '6'.

[u, 2013-02-09 20:22:44]

Hello golang-dev@googlegroups.com (cc: adg@golang.org),

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

[Donovan, 2013-02-10 11:14:48]

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

https://codereview.appspot.com/7305072/diff/7001/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:75: mode = os.FileMode(fi.h.Mode) & os.ModePerm
It's a bit obscure, but the mask 1777 allows for the preservation of the sticky
bit:
http://en.wikipedia.org/wiki/Sticky_bit
https://codereview.appspot.com/7203050

[minux1, 2013-02-10 18:22:17]

tests?

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

https://codereview.appspot.com/7305072/diff/7001/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:75: mode = os.FileMode(fi.h.Mode) & os.ModePerm
On 2013/02/10 11:14:48, Donovan wrote:
> It's a bit obscure, but the mask 1777 allows for the preservation of the
sticky
tar supports setuid and setgid, so perhaps you need 07777.

https://codereview.appspot.com/7305072/diff/7001/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:98: case '0', '\x00':
please use TypeReg, TypeRegA here.

[u, 2013-02-10 18:43:07]

Hello golang-dev@googlegroups.com, donovanhide@gmail.com, r.eklind.87@gmail.com,
minux.ma@gmail.com (cc: adg@golang.org, golang-dev@googlegroups.com),

Please take another look.

[u, 2013-02-10 18:45:00]

On 2013/02/10 18:22:17, minux wrote:
> tests?

I want to include test but don't know yet which way to do it. Should we extend
the previous test and add more members the the struct so it can also test
Header.FileInfo or should we create a new struct and duplicate some of the data?

Either way we should probably add at least one more tar testdata file, which
includes FIFO, socket, device and other special files.

> https://codereview.appspot.com/7305072/diff/7001/src/pkg/archive/tar/common.go
> File src/pkg/archive/tar/common.go (right):
> 
>
https://codereview.appspot.com/7305072/diff/7001/src/pkg/archive/tar/common.g...
> src/pkg/archive/tar/common.go:75: mode = os.FileMode(fi.h.Mode) & os.ModePerm
> On 2013/02/10 11:14:48, Donovan wrote:
> > It's a bit obscure, but the mask 1777 allows for the preservation of the
> sticky
> tar supports setuid and setgid, so perhaps you need 07777.

It now handles sticky bit, setuid and setgid properly. We can't copy them
directly since Go uses other bits to represent these platform independently.

>
https://codereview.appspot.com/7305072/diff/7001/src/pkg/archive/tar/common.g...
> src/pkg/archive/tar/common.go:98: case '0', '\x00':
> please use TypeReg, TypeRegA here.

The code has been updated to use Type*.

Did you see my first message? https://codereview.appspot.com/7305072/#msg1

Is there a way to differentiate a hard link from a symbolic link using
os.FileMode? Is os.ModeSymlink correct for hard links?

Should we remove nop cases (TypeReg, TypeRegA, TypeCont, TypeXHeader and
TypeXGlobalHeader)?

[u, 2013-02-10 18:47:17]

On 2013/02/10 11:14:48, Donovan wrote:
> https://codereview.appspot.com/7305072/diff/7001/src/pkg/archive/tar/common.go
> File src/pkg/archive/tar/common.go (right):
> 
>
https://codereview.appspot.com/7305072/diff/7001/src/pkg/archive/tar/common.g...
> src/pkg/archive/tar/common.go:75: mode = os.FileMode(fi.h.Mode) & os.ModePerm
> It's a bit obscure, but the mask 1777 allows for the preservation of the
sticky
> bit:
> http://en.wikipedia.org/wiki/Sticky_bit
> https://codereview.appspot.com/7203050

Thank you Donovan. I've updated the code to handle the sticky, setid and setgid
bits. We can't copy them directly since Go uses other bits to represent these
platform independently.

[adg, 2013-02-11 01:10:53]

On 2013/02/10 18:45:00, u wrote:
> On 2013/02/10 18:22:17, minux wrote:
> > tests?
> 
> I want to include test but don't know yet which way to do it. 

Take a look at testHeaderRoundTrip from archive/zip

[u, 2013-02-11 11:12:47]

Hello golang-dev@googlegroups.com, donovanhide@gmail.com, r.eklind.87@gmail.com,
minux.ma@gmail.com, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2013-02-12 03:43:15]

Looks good, but what about the roundtrip test that I suggested?

[u, 2013-02-13 06:12:35]

Hello golang-dev@googlegroups.com, donovanhide@gmail.com, r.eklind.87@gmail.com,
minux.ma@gmail.com, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[u, 2013-02-13 06:13:36]

TestHeaderRoundTrip has now been implemented.

FileInfoHeader has also been extended to fifo, setuid, setgid and sticky bits.
Before FileInfoHeader could fail with the following error:
"archive/tar: unknown file mode prw-------"

The Go implementation of tar differs from the GNU tar command line tool, and I
would like to make sure that Go is doing the right thing. When creating tar
archives the GNU tar command line tool doesn't include all Header.Mode bits. As
far as I can tell it only includes c_ISUID, c_ISGID and c_ISVTX while ignoring
c_ISREG, c_ISDIR, etc.

Since Header.Typeflag can represent most of these it never seems to be an issue
with GNU tar.

I only want to verify that Go is doing the correct thing when including all of
these mode constants in Header.Mode. It does seem logical, but does anyone know
for sure?

Cheers /robin

[u, 2013-02-13 11:48:11]

Hello golang-dev@googlegroups.com, donovanhide@gmail.com, r.eklind.87@gmail.com,
minux.ma@gmail.com, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[u, 2013-02-13 11:48:53]

Updated to reflect changes in rev d4024b084e0c.

[adg, 2013-02-13 22:36:54]

LGTM

Great work, thanks.

[u, 2013-02-14 05:30:16]

On 2013/02/13 22:36:54, adg wrote:
> LGTM
> 
> Great work, thanks.

Thank you. The work was inspired by Donovan who started it in issue 4695 and CL
7203050. Thanks Donovan!

[adg, 2013-02-14 06:36:10]

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

archive/tar: add Header.FileInfo method. Add more cases to FileInfoHeader.

FileInfoHeader can now handle fifo, setuid, setgid and sticky bits.

Fixes issue 4695.

R=golang-dev, donovanhide, r.eklind.87, minux.ma, adg
CC=golang-dev
https://codereview.appspot.com/7305072

Committer: Andrew Gerrand <adg@golang.org>

[u, 2013-02-22 08:14:52]

*** Abandoned ***