Grokbase › Groups › Go › golang-dev › November 2012
FAQ
https://codereview.appspot.com/6700047/diff/29004/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/6700047/diff/29004/src/pkg/archive/tar/writer.go#newcode202
src/pkg/archive/tar/writer.go:202: fmt.Fprintf(&buf, "%d%s", size, msg)
The pax standard is weird here. It states that the %d must be the length
of the entire message (including) %d. So somehow we have to add that
length on. I'm convinced what I have here is wrong, I'll try and submit
something better. Here is an example of a real live pax header:

"30 mtime=1350244992.023960108\n"

I'm submitting code which I think does the length adjustment correctly.
On 2012/10/31 12:16:25, dsymonds wrote:
I am confused by this construction. Shouldn't it be something more
like this?
msg := fmt.Sprintf(" path=%s\n", hdr.Name)
fmt.Fprintf(&buf, "%d %s", len(msg), msg)
https://codereview.appspot.com/6700047/diff/29004/src/pkg/archive/tar/writer.go#newcode212
src/pkg/archive/tar/writer.go:212: err := tw.WriteHeader(extendedHdr)
On 2012/10/31 12:16:25, dsymonds wrote:
snuggle these where possible.
if err := tw.WriteHeader(ext); err != nil {
return err
}
etc.
Done.

https://codereview.appspot.com/6700047/diff/29004/src/pkg/archive/tar/writer_test.go
File src/pkg/archive/tar/writer_test.go (right):

https://codereview.appspot.com/6700047/diff/29004/src/pkg/archive/tar/writer_test.go#newcode193
src/pkg/archive/tar/writer_test.go:193: t.Fatal("os.Stat: " +
err.Error())
On 2012/10/31 12:16:25, dsymonds wrote:
t.Fatalf("os.Stat: %v", err)
Done.

https://codereview.appspot.com/6700047/diff/29004/src/pkg/archive/tar/writer_test.go#newcode196
src/pkg/archive/tar/writer_test.go:196: long_name :=
strings.Repeat("ab", 100)
On 2012/10/31 12:16:25, dsymonds wrote:
long_name -> longName
Done.

https://codereview.appspot.com/6700047/diff/29004/src/pkg/archive/tar/writer_test.go#newcode220
src/pkg/archive/tar/writer_test.go:220: if !(hdr.Name == long_name) {
On 2012/10/31 12:16:25, dsymonds wrote:
if hdr.Name != longName
Done.

https://codereview.appspot.com/6700047/

Search Discussions

  • Shanemhansen at Nov 1, 2012 at 2:42 am ⇧
    Hello golang-dev@googlegroups.com, dsymonds@golang.org, dave@cheney.net,
    rogpeppe@gmail.com, remyoudompheng@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6700047/
  • David Symonds at Nov 1, 2012 at 2:44 am ⇧

    On Thu, Nov 1, 2012 at 1:40 PM, wrote:

    The pax standard is weird here. It states that the %d must be the length
    of the entire message (including) %d. So somehow we have to add that
    length on. I'm convinced what I have here is wrong, I'll try and submit
    something better. Here is an example of a real live pax header:

    "30 mtime=1350244992.023960108\n"

    I'm submitting code which I think does the length adjustment correctly.
    That seems nuts. That would mean "9 a=1234\n" and "10 a=1234\n" mean
    the same thing. Did that header you pasted come from a real life tar
    implementation? Which one?
  • Shane Hansen at Nov 1, 2012 at 3:28 am ⇧
    It was produced by tar (GNU tar) 1.26
    Here's a script to reproduce the behaviour.
    http://pastebin.com/WVDnyGhR

    It's pretty weird. I also got that specific example from the pax.tar file
    in testdata/
    On Wed, Oct 31, 2012 at 8:44 PM, David Symonds wrote:
    On Thu, Nov 1, 2012 at 1:40 PM, wrote:

    The pax standard is weird here. It states that the %d must be the length
    of the entire message (including) %d. So somehow we have to add that
    length on. I'm convinced what I have here is wrong, I'll try and submit
    something better. Here is an example of a real live pax header:

    "30 mtime=1350244992.023960108\n"

    I'm submitting code which I think does the length adjustment correctly.
    That seems nuts. That would mean "9 a=1234\n" and "10 a=1234\n" mean
    the same thing. Did that header you pasted come from a real life tar
    implementation? Which one?
  • David Symonds at Nov 1, 2012 at 3:17 am ⇧
    Okay, just pull it out into a function as I suggest and carefully test it.
  • Dsymonds at Nov 1, 2012 at 2:51 am ⇧
    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go
    File src/pkg/archive/tar/writer.go (right):

    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode198
    src/pkg/archive/tar/writer.go:198: msg := fmt.Sprintf(" path=%s\n",
    hdr.Name)
    pull this block into its own function

    // paxHeader returns a PAX header for the given "key=value" message.
    func paxHeader(m string) string {
    ...
    }

    and just use
    fmt.Fprint(&buf, paxHeader(fmt.Sprintf("path=%s", hdr.Name)))

    I think such a function would warrant a test of its own.

    (also, please use "hg gofmt" before uploading new snapshots)

    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode213
    src/pkg/archive/tar/writer.go:213: _, err := tw.Write(buf.Bytes())
    snuggle

    https://codereview.appspot.com/6700047/
  • Chressie at Nov 5, 2012 at 3:51 pm ⇧
    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go
    File src/pkg/archive/tar/writer.go (right):

    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode192
    src/pkg/archive/tar/writer.go:192: dir, file := filepath.Split(hdr.Name)
    i think the delimiter in these archives is always a plain '/'. at least
    i couldn't find a counter example :)

    https://codereview.appspot.com/6700047/
  • Chressie at Nov 5, 2012 at 5:44 pm ⇧
    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go
    File src/pkg/archive/tar/writer.go (right):

    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode193
    src/pkg/archive/tar/writer.go:193: ext.Name = filepath.Join(dir,
    another issue i can see is that Join cleans the resulting path. maybe it
    makes sense to path.Clean(hdr.Name) anyway? but that seems to be an
    incompatibility with the go1 promise. any thoughts?

    https://codereview.appspot.com/6700047/
  • Shanemhansen at Nov 7, 2012 at 12:53 am ⇧
    Hello golang-dev@googlegroups.com, dsymonds@golang.org, dave@cheney.net,
    rogpeppe@gmail.com, remyoudompheng@gmail.com, chressie@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6700047/
  • Shanemhansen at Nov 7, 2012 at 12:59 am ⇧
    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go
    File src/pkg/archive/tar/writer.go (right):

    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode192
    src/pkg/archive/tar/writer.go:192: dir, file := filepath.Split(hdr.Name)
    According to my reading of the python tarfile implementation, they use
    /.
    On 2012/11/05 15:51:08, chressie1 wrote:
    i think the delimiter in these archives is always a plain '/'. at least i
    couldn't find a counter example :)
    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode193
    src/pkg/archive/tar/writer.go:193: ext.Name = filepath.Join(dir,
    Sorry, I'm not sure what you mean by cleaning the path.
    On 2012/11/05 17:44:47, chressie1 wrote:
    another issue i can see is that Join cleans the resulting path. maybe it makes
    sense to path.Clean(hdr.Name) anyway? but that seems to be an
    incompatibility
    with the go1 promise. any thoughts?
    https://codereview.appspot.com/6700047/
  • Dsymonds at Nov 7, 2012 at 1:02 am ⇧
    https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer.go
    File src/pkg/archive/tar/writer.go (right):

    https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer.go#newcode198
    src/pkg/archive/tar/writer.go:198: fmt.Fprint(&buf,
    paxHeader(fmt.Sprintf("path=%s", hdr.Name)))
    no need for fmt.Sprintf just to concatenate two strings.
    "path=" + hdr.Name

    same for hdr.Linkname

    https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer.go#newcode219
    src/pkg/archive/tar/writer.go:219: padding := 2 // Extra padding for
    space and newline
    const padding = 2

    https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer.go#newcode221
    src/pkg/archive/tar/writer.go:221: size += len(strconv.Itoa(size))
    This still can't be right. What if this += involves a decimal carry?
    Then the output will be one longer than what is computed here.

    please add a test case that covers this, and fix this code.

    https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer_test.go
    File src/pkg/archive/tar/writer_test.go (right):

    https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer_test.go#newcode228
    src/pkg/archive/tar/writer_test.go:228: paxTests :=
    map[string]string{"name=/etc/hosts": "19 name=/etc/hosts\n",
    one test case per line so we can read it please.

    also add a case that yields a single digit length.

    https://codereview.appspot.com/6700047/
  • Chressie at Nov 7, 2012 at 6:42 am ⇧
    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go
    File src/pkg/archive/tar/writer.go (right):

    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode192
    src/pkg/archive/tar/writer.go:192: dir, file := filepath.Split(hdr.Name)
    On 2012/11/07 00:59:48, shanemhansen wrote:
    According to my reading of the python tarfile implementation, they use
    /.

    exactly. and therefore you should use the path package (which handles /
    separated paths) and not path/filepath (which uses os.PathSeparator
    separated paths)

    https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode193
    src/pkg/archive/tar/writer.go:193: ext.Name = filepath.Join(dir,
    On 2012/11/07 00:59:48, shanemhansen wrote:
    Sorry, I'm not sure what you mean by cleaning the path.
    From [0]: "Clean returns the shortest path name equivalent to path by
    purely lexical processing."

    that means if someone wants to have a path like "a/b/../../c///" in his
    tarball, he'll get instead the cleaned path equivalent "c".

    that is bad because of 2 things: (1) it's not what the tarball creator
    expects, because its not documented; (2) the trailing slash is gone.

    i think cleaning the path would make sense, but then it needs to be done
    (1) for all hdr.Name (not only the ones which are >100 chars) and (2)
    the trailing slash needs to be appended (that's what gnu tar 1.26
    does).. but this might break go1 compatibility.

    [0]: http://golang.org/pkg/path/#Clean

    https://codereview.appspot.com/6700047/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 1, '12 at 2:40a
activeNov 7, '12 at 6:42a
posts12
users3
websitegolang.org

3 users in discussion

Shanemhansen: 5 posts Dsymonds: 4 posts Chressie: 3 posts

Content

People

Support

Translate

site design / logo © 2022 Grokbase