code review 2125042: archive/zip: new package for reading ZIP files

src/pkg/archive/zip/reader.go

+// Copyright 2010 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+/*
+The zip package provides support for reading ZIP archives as described here:·
+http://www.pkware.com/documents/casestudies/APPNOTE.TXT
+
+This package does not support ZIP64 or disk spanning.
+*/
+package zip
+
+import (
+        "compress/flate"
+        "encoding/binary"
+        "io"
+        "os"
+)
+
+var (
+        ShortReadError      = os.NewError("Short read")
+        WrongSignatureError = os.NewError("Wrong signature for this header")
+        NotFoundError       = os.NewError("The signature could not be found")
+        UnsupportedMethod   = os.NewError("This file uses an unsupported compression method")
+)
+
+type Reader struct {
+        directoryEnd

[rsc1, 2010/09/08 14:55:50]

Does this need to be in the struct?

[adg, 2010/09/09 10:47:44]

No, removed.

+        r       io.ReaderAt
+        Files   []*File

[rsc1, 2010/09/08 14:55:50]

we're not entirely consistent about this but
most of the go code uses singular here:

File []*File

so that you write File[i].Name

[adg, 2010/09/09 10:47:44]

Done.

+        Comment string
+}
+
+// File represents a single file within a Zip archive.
+type File struct {
+        directoryHeader

[rsc1, 2010/09/08 14:55:50]

you have to expose this if you are trying to expose the fields.
otherwise godoc won't tell people about it.  see archive/tar.

I'd suggest making this FileHeader

[adg, 2010/09/09 10:47:44]

Done.

+        r          *io.SectionReader
+        header     *fileHeader
+        bodyOffset int64
+        Filename   string

[rsc1, 2010/09/08 14:55:50]

It's already in type File.  s/Filename/Name/

[adg, 2010/09/09 10:47:44]

Done.

+}
+
+// ReadFile will open the Zip file specified by name and return a Reader.
+func ReadFile(name string) (r *Reader, err os.Error) {

[rsc1, 2010/09/08 14:55:50]

s/ReadFile/OpenReader/?

[adg, 2010/09/09 10:47:44]

Done.

+        f, err := os.Open(name, os.O_RDONLY, 0644)
+        if err != nil {
+                return
+        }
+        fi, err := f.Stat()
+        if err != nil {
+                return
+        }
+        return NewReader(f, fi.Size)
+}
+
+// NewReader takes a ReaderAt and stream length,·

[rsc1, 2010/09/08 14:55:50]

You can drop the "takes" notes: it's in the code.
If you want to mention the names it's better to define their meanings.

// NewReader returns a new Reader reading from r, which is assumed
// to have the given size in bytes.

[adg, 2010/09/09 10:47:44]

Done.

+// reads the Zip file metadata,
+// and returns a *Reader with its Files element populated
+// with the Zip file's contents.
+func NewReader(r io.ReaderAt, length int64) (z *Reader, err os.Error) {

[rsc1, 2010/09/08 14:55:50]

s/length/size/ to match os.FileInfo

[adg, 2010/09/09 10:47:44]

Done.

+        rs := io.NewSectionReader(r, 0, length)
+        offs, err := findDirectoryEndOffset(rs)

[rsc1, 2010/09/08 14:55:50]

s/offs/off/
offs sounds plural

[adg, 2010/09/09 10:47:44]

Done.

+        if err != nil {
+                return
+        }
+        if _, err = rs.Seek(offs, 0); err != nil {
+                return
+        }
+        end, err := readDirectoryEnd(rs)
+        if err != nil {
+                return
+        }
+        z = &Reader{
+                directoryEnd: *end,
+                r:            r,
+                Files:        make([]*File, end.directoryRecords),
+                Comment:      end.Comment,
+        }
+        if _, err = rs.Seek(int64(z.directoryOffset), 0); err != nil {
+                return
+        }
+        for i := range z.Files {
+                dh, err := readDirectoryHeader(rs)
+                if err != nil {
+                        return nil, err

[rsc1, 2010/09/08 14:55:50]

I don't like using named return values in 
anything but trivial functions, for exactly this reason.
I won't insist but please consider dropping the names
from all the return values in this file.  It has the
added benefit that the signatures are easier for
humans to read quickly: as a client I don't care
about seeing "z" and "err", just "*Reader, os.Error".

[adg, 2010/09/09 10:47:44]

Done.

+                }
+                z.Files[i] = &File{
+                        directoryHeader: *dh,
+                        Filename:        dh.Filename,
+                }
+        }
+        for i, f := range z.Files {
+                offs := int64(f.fileHeaderOffset)

[rsc1, 2010/09/08 14:55:50]

s/offs/off/

[adg, 2010/09/09 10:47:44]

Done.

+                var max int64
+                if i == len(z.Files)-1 {
+                        max = int64(z.directoryOffset)
+                } else {
+                        max = int64(z.Files[i+1].fileHeaderOffset)
+                }
+                max -= offs
+                f.r = io.NewSectionReader(r, offs, max)
+        }
+        return
+}
+
+// Open returns a ReadCloser that provides access to the File's contents.
+func (f *File) Open() (rc io.ReadCloser, err os.Error) {
+        if f.header == nil {
+                if _, err = f.r.Seek(0, 0); err != nil {
+                        return
+                }
+                if f.header, err = readFileHeader(f.r); err != nil {
+                        return
+                }
+                if f.bodyOffset, err = f.r.Seek(0, 1); err != nil {
+                        return
+                }
+        }
+        reader := io.NewSectionReader(f.r, f.bodyOffset, int64(f.header.CompressedSize))
+        switch f.Method {
+        case 0: // store (no compression)
+                rc = nopCloser{reader}
+        case 8: // DEFLATE
+                rc = flate.NewReader(reader)
+        default:
+                err = UnsupportedMethod
+        }
+        return
+}
+
+type nopCloser struct {
+        io.Reader
+}
+
+func (f nopCloser) Close() os.Error { return nil }
+
+func readFileHeader(r io.Reader) (f *fileHeader, err os.Error) {
+        defer func() {
+                if rerr, ok := recover().(os.Error); rerr != nil && ok {

[rsc1, 2010/09/08 14:55:50]

rerr != nil is implied by ok

[adg, 2010/09/09 10:47:44]

Done.

+                        err = rerr
+                }
+        }()
+        f = new(fileHeader)
+        read(r, &f.signature)
+        if f.signature != fileHeaderSignature {
+                return nil, WrongSignatureError
+        }
+        read(r, &f.ReaderVersion)
+        read(r, &f.flags)
+        read(r, &f.Method)
+        read(r, &f.modifiedTime)
+        read(r, &f.modifiedDate)
+        read(r, &f.CRC32)
+        read(r, &f.CompressedSize)
+        read(r, &f.UncompressedSize)
+        read(r, &f.filenameLength)
+        read(r, &f.extraLength)
+        f.Filename = string(readByteSlice(r, f.filenameLength))
+        f.Extra = readByteSlice(r, f.extraLength)
+        return
+}
+
+func readDirectoryHeader(r io.Reader) (d *directoryHeader, err os.Error) {
+        defer func() {
+                if rerr, ok := recover().(os.Error); rerr != nil && ok {

[rsc1, 2010/09/08 14:55:50]

same

[adg, 2010/09/09 10:47:44]

Done.

+                        err = rerr
+                }
+        }()
+        d = new(directoryHeader)
+        read(r, &d.signature)
+        if d.signature != directoryHeaderSignature {
+                return nil, WrongSignatureError
+        }
+        read(r, &d.CreatorVersion)
+        read(r, &d.ReaderVersion)
+        read(r, &d.flags)
+        read(r, &d.Method)
+        read(r, &d.modifiedTime)
+        read(r, &d.modifiedDate)
+        read(r, &d.CRC32)
+        read(r, &d.CompressedSize)
+        read(r, &d.UncompressedSize)
+        read(r, &d.filenameLength)
+        read(r, &d.extraLength)
+        read(r, &d.commentLength)
+        read(r, &d.startDiskNumber)
+        read(r, &d.internalAttributes)
+        read(r, &d.externalAttributes)
+        read(r, &d.fileHeaderOffset)
+        d.Filename = string(readByteSlice(r, d.filenameLength))
+        d.Extra = readByteSlice(r, d.extraLength)
+        d.Commment = string(readByteSlice(r, d.commentLength))
+        return
+}
+
+func readDirectoryEnd(r io.Reader) (d *directoryEnd, err os.Error) {
+        defer func() {
+                if rerr, ok := recover().(os.Error); rerr != nil && ok {
+                        err = rerr
+                }
+        }()
+        d = new(directoryEnd)
+        read(r, &d.signature)
+        if d.signature != directoryEndSignature {
+                return nil, WrongSignatureError
+        }
+        read(r, &d.diskNbr)
+        read(r, &d.dirDiskNbr)
+        read(r, &d.dirRecordsThisDisk)
+        read(r, &d.directoryRecords)
+        read(r, &d.directorySize)
+        read(r, &d.directoryOffset)
+        read(r, &d.commentLen)
+        d.Comment = string(readByteSlice(r, d.commentLen))
+        return
+}
+
+func findDirectoryEndOffset(rs io.ReadSeeker) (offset int64, err os.Error) {
+        const minRecordSize = 4 + 2 + 2 + 2 + 2 + 4 + 4 + 2
+        offset, err = rs.Seek(-minRecordSize, 2) // relative to end-of-file
+        if err != nil {
+                return
+        }
+        for ; offset >= 0; offset-- {
+                rs.Seek(offset, 0)
+                // TODO: this is very inefficient, but it works.
+                // Better to write the little-endian representation of
+                // directoryEndSignature to a []byte and then
+                // seek backwards comparing subslices of the read buffer.
+                var sig uint32
+                err = binary.Read(rs, binary.LittleEndian, &sig)
+                if err != nil {
+                        return 0, err
+                }
+                if sig == directoryEndSignature {
+                        return
+                }
+        }
+        return 0, NotFoundError
+}
+
+func read(r io.Reader, data interface{}) {
+        if err := binary.Read(r, binary.LittleEndian, data); err != nil {
+                panic(err)
+        }
+}
+
+func readByteSlice(r io.Reader, l uint16) []byte {
+        b := make([]byte, l)
+        if l == 0 {
+                return b
+        }
+        n, err := r.Read(b)
+        if err != nil {
+                panic(err)
+        }
+        if n != int(l) {
+                panic(ShortReadError)
+        }
+        return b
+}