code review 5798062: os: make FileInfo.Size() portable (zero on non-regular ...

os: make FileInfo.Size() portable (zero on non-regular files)

This behavior was different between operating systems and bit
us when writing tar files.

Define the FileInfo.Size to be zero on non-regular files.

[bradfitz, 2012-03-12 06:22:04]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[dsymonds, 2012-03-12 06:24:51]

LGTM

[bradfitz, 2012-03-12 06:29:52]

I don't think this is risky or tricky, but it *is* late.

Waiting on more LGTMs.

I do think pkg os is supposed to be portable, though.  If people want
*syscall.Stat_t, they know where to find it.


On Sun, Mar 11, 2012 at 11:22 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> os: make FileInfo.Size() portable (zero on non-regular files)
>
> This behavior was different between operating systems and bit
> us when writing tar files.
>
> Define the FileInfo.Size to be zero on non-regular files.
>
> Please review this at
http://codereview.appspot.com/**5798062/<http://codereview.appspot.com/5798062/>
>
> Affected files:
>  M src/pkg/os/os_test.go
>  M src/pkg/os/stat_darwin.go
>  M src/pkg/os/stat_freebsd.go
>  M src/pkg/os/stat_linux.go
>  M src/pkg/os/stat_netbsd.go
>  M src/pkg/os/stat_openbsd.go
>  M src/pkg/os/stat_plan9.go
>  M src/pkg/os/stat_windows.go
>  M src/pkg/os/types.go
>
>
> Index: src/pkg/os/os_test.go
> ==============================**==============================**=======
> --- a/src/pkg/os/os_test.go
> +++ b/src/pkg/os/os_test.go
> @@ -1047,3 +1047,16 @@
>                t.Errorf("files should be different")
>        }
>  }
> +
> +func TestStatDirectorySizeZero(t *testing.T) {
> +       fi, err := Stat(".")
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       if !fi.IsDir() {
> +               t.Fatal("expecting dir")
> +       }
> +       if fi.Size() != 0 {
> +               t.Fatalf("got directory size %d; want 0", fi.Size())
> +       }
> +}
> Index: src/pkg/os/stat_darwin.go
> ==============================**==============================**=======
> --- a/src/pkg/os/stat_darwin.go
> +++ b/src/pkg/os/stat_darwin.go
> @@ -18,7 +18,6 @@
>  func fileInfoFromStat(st *syscall.Stat_t, name string) FileInfo {
>        fs := &fileStat{
>                name:    basename(name),
> -               size:    int64(st.Size),
>                modTime: timespecToTime(st.Mtimespec),
>                sys:     st,
>        }
> @@ -35,7 +34,7 @@
>        case syscall.S_IFLNK:
>                fs.mode |= ModeSymlink
>        case syscall.S_IFREG:
> -               // nothing to do
> +               fs.size = int64(st.Size)
>        case syscall.S_IFSOCK:
>                fs.mode |= ModeSocket
>        }
> Index: src/pkg/os/stat_freebsd.go
> ==============================**==============================**=======
> --- a/src/pkg/os/stat_freebsd.go
> +++ b/src/pkg/os/stat_freebsd.go
> @@ -18,7 +18,6 @@
>  func fileInfoFromStat(st *syscall.Stat_t, name string) FileInfo {
>        fs := &fileStat{
>                name:    basename(name),
> -               size:    int64(st.Size),
>                modTime: timespecToTime(st.Mtimespec),
>                sys:     st,
>        }
> @@ -35,7 +34,7 @@
>        case syscall.S_IFLNK:
>                fs.mode |= ModeSymlink
>        case syscall.S_IFREG:
> -               // nothing to do
> +               fs.size = int64(st.Size)
>        case syscall.S_IFSOCK:
>                fs.mode |= ModeSocket
>        }
> Index: src/pkg/os/stat_linux.go
> ==============================**==============================**=======
> --- a/src/pkg/os/stat_linux.go
> +++ b/src/pkg/os/stat_linux.go
> @@ -18,7 +18,6 @@
>  func fileInfoFromStat(st *syscall.Stat_t, name string) FileInfo {
>        fs := &fileStat{
>                name:    basename(name),
> -               size:    int64(st.Size),
>                modTime: timespecToTime(st.Mtim),
>                sys:     st,
>        }
> @@ -35,7 +34,7 @@
>        case syscall.S_IFLNK:
>                fs.mode |= ModeSymlink
>        case syscall.S_IFREG:
> -               // nothing to do
> +               fs.size = int64(st.Size)
>        case syscall.S_IFSOCK:
>                fs.mode |= ModeSocket
>        }
> Index: src/pkg/os/stat_netbsd.go
> ==============================**==============================**=======
> --- a/src/pkg/os/stat_netbsd.go
> +++ b/src/pkg/os/stat_netbsd.go
> @@ -18,7 +18,6 @@
>  func fileInfoFromStat(st *syscall.Stat_t, name string) FileInfo {
>        fs := &fileStat{
>                name:    basename(name),
> -               size:    int64(st.Size),
>                modTime: timespecToTime(st.Mtim),
>                sys:     st,
>        }
> @@ -35,7 +34,7 @@
>        case syscall.S_IFLNK:
>                fs.mode |= ModeSymlink
>        case syscall.S_IFREG:
> -               // nothing to do
> +               fs.size = int64(st.Size)
>        case syscall.S_IFSOCK:
>                fs.mode |= ModeSocket
>        }
> Index: src/pkg/os/stat_openbsd.go
> ==============================**==============================**=======
> --- a/src/pkg/os/stat_openbsd.go
> +++ b/src/pkg/os/stat_openbsd.go
> @@ -18,7 +18,6 @@
>  func fileInfoFromStat(st *syscall.Stat_t, name string) FileInfo {
>        fs := &fileStat{
>                name:    basename(name),
> -               size:    int64(st.Size),
>                modTime: timespecToTime(st.Mtim),
>                sys:     st,
>        }
> @@ -35,7 +34,7 @@
>        case syscall.S_IFLNK:
>                fs.mode |= ModeSymlink
>        case syscall.S_IFREG:
> -               // nothing to do
> +               fs.size = int64(st.Size)
>        case syscall.S_IFSOCK:
>                fs.mode |= ModeSocket
>        }
> Index: src/pkg/os/stat_plan9.go
> ==============================**==============================**=======
> --- a/src/pkg/os/stat_plan9.go
> +++ b/src/pkg/os/stat_plan9.go
> @@ -18,7 +18,6 @@
>  func fileInfoFromStat(d *Dir) FileInfo {
>        fs := &fileStat{
>                name:    d.Name,
> -               size:    int64(d.Length),
>                modTime: time.Unix(int64(d.Mtime), 0),
>                sys:     d,
>        }
> @@ -35,6 +34,9 @@
>        if d.Mode&syscall.DMTMP != 0 {
>                fs.mode |= ModeTemporary
>        }
> +       if fs.mode&ModeType == 0 {
> +               fs.size = int64(d.Length)
> +       }
>        return fs
>  }
>
> Index: src/pkg/os/stat_windows.go
> ==============================**==============================**=======
> --- a/src/pkg/os/stat_windows.go
> +++ b/src/pkg/os/stat_windows.go
> @@ -26,13 +26,17 @@
>        if e != nil {
>                return nil, &PathError{"**GetFileInformationByHandle",
> file.name, e}
>        }
> -       return &fileStat{
> +       fs := &fileStat{
>                name:    basename(file.name),
> -               size:    mkSize(d.FileSizeHigh, d.FileSizeLow),
>                modTime: mkModTime(d.LastWriteTime),
>                mode:    mkMode(d.FileAttributes),
>                sys:     mkSysFromFI(&d),
> -       }, nil
> +       }
> +       if fs.Mode()&ModeType == 0 {
> +               // Only for regular files.
> +               fs.size = mkSize(d.FileSizeHigh, d.FileSizeLow)
> +       }
> +       return fi, nil
>  }
>
>  // Stat returns a FileInfo structure describing the named file.
> Index: src/pkg/os/types.go
> ==============================**==============================**=======
> --- a/src/pkg/os/types.go
> +++ b/src/pkg/os/types.go
> @@ -15,7 +15,7 @@
>  // A FileInfo describes a file and is returned by Stat and Lstat
>  type FileInfo interface {
>        Name() string       // base name of the file
> -       Size() int64        // length in bytes
> +       Size() int64        // length in bytes (0 if not a regular file)
>        Mode() FileMode     // file mode bits
>        ModTime() time.Time // modification time
>        IsDir() bool        // abbreviation for Mode().IsDir()
>
>
>

[r2, 2012-03-12 07:08:54]

Where does this CL define that property? I see code but no definition.
Nor am I even convinced it's the correct thing to do. There may be
systems where directories have a valid non-zero size. There certainly
used to be, Unix among them (although directories are made "special"
these days) and package os's purpose is not to hide every detail, just
to provide a consistent interface. The operating system may do
different things with that interface, and often will. I don't see the
contract going all the way to defining the properties of directory
stats. It may even be a dangerous precedent to overstep like that, by
arguing that package os will hide valid differences.

To be facetious, you had a bug; why take it out on os?

I expect to be outvoted, but if this CL is going in, it needs
documentation and justification.

-rob

[remyoudompheng, 2012-03-12 07:27:17]

Same thing for symbolic links. I am used to symbolic links size to be the length
of the string they contain on Linux.
What kind of system gives a zero size for directories and symlinks?

[bradfitz, 2012-03-12 15:17:20]

On Mon, Mar 12, 2012 at 12:08 AM, Rob 'Commander' Pike <r@google.com> wrote:

> Where does this CL define that property? I see code but no definition.
>

types.go.


> Nor am I even convinced it's the correct thing to do. There may be
> systems where directories have a valid non-zero size. There certainly
> used to be, Unix among them (although directories are made "special"
> these days)


Go already doesn't let you read directories' bytes, though:

func main() {
f, _ := os.Open(".")
buf, err := ioutil.ReadAll(f)
fmt.Printf("read: %q, %v\n", buf, err)
}

read: "", read .: is a directory

So any Size() on them is misleading at best.  It's a size of something you
can't get at.

and package os's purpose is not to hide every detail, just
> to provide a consistent interface.


I'm just trying to make it more consistent and more simple.

I write a fair bit of backup/VFS code and I'd like it to be more defined.
 I argued for the current FileInfo.Sys docs saying that it may be nil, for
instance, as I was afraid of code that assume Sys() was non-nil or of a
certain type.  I likewise fear (irrationally?) that people will rely on
directories having a Size() for some reason.

Remy argued for symlink sizes too, but that's just len(ReadLink's result).
 There's no ReadLinkIntoBytes anyway, so no need to allocate the buffer
yourself.


> The operating system may do
> different things with that interface, and often will. I don't see the
> contract going all the way to defining the properties of directory
> stats. It may even be a dangerous precedent to overstep like that, by
> arguing that package os will hide valid differences.
>

Package os already does.  We have the os.FileMode interface, os.FileMode
type, Windows permissions reduce down to 0777 (kinda?).


> To be facetious, you had a bug; why take it out on os?
>

Because it brought this to my attention.  I would've argued for this
independently, had I known.


> I expect to be outvoted, but if this CL is going in, it needs
> documentation and justification.


I likewise expect to get outvoted, but here it is.

At the least, I propose the following one-line documentation change in
types.go:

 type FileInfo interface {
   ...
   Size() int64   // length in bytes; undefined for non-regular files
   ...
}

[aram, 2012-03-12 16:36:24]

Brad Fitzpatrick wrote:
>
> Go already doesn't let you read directories' bytes, though:
>
> func main() {
> f, _ := os.Open(".")
>  buf, err := ioutil.ReadAll(f)
> fmt.Printf("read: %q, %v\n", buf, err)
> }
>
> read: "", read .: is a directory
>

I believe that's Linux, not Go. There are some Unices where you can still 
read directories, e.g. Solaris.

[bradfitz, 2012-03-12 16:59:52]

On Mon, Mar 12, 2012 at 9:36 AM, Aram Hăvărneanu <aram@mgk.ro> wrote:

> Brad Fitzpatrick wrote:
>>
>> Go already doesn't let you read directories' bytes, though:
>>
>> func main() {
>> f, _ := os.Open(".")
>>  buf, err := ioutil.ReadAll(f)
>> fmt.Printf("read: %q, %v\n", buf, err)
>> }
>>
>> read: "", read .: is a directory
>>
>
> I believe that's Linux, not Go. There are some Unices where you can still
> read directories, e.g. Solaris.
>

That was on Go on a Mac.

[rsc, 2012-03-12 17:07:30]

I think we should not make promises about Size here.
Implementations will inevitably fail to honor them.

Also, the fact that ls -l /dev/hda doesn't show me the
size of the disk on Linux is a bug anyway.  I don't want
to require this behavior.

If this CL were requiring 0 size only for directories, I might
go along with it, but even then I think it is probably safer
to write code that doesn't depend on this assumption.
I know it sucks.

Russ

[aram, 2012-03-12 17:11:44]

Brad Fitzpatrick wrote:
>
> I believe that's Linux, not Go. There are some Unices where you can still 
>> read directories, e.g. Solaris.
>>
>
> That was on Go on a Mac.
>

Sure, s/Linux/Darwin kernel/. My point was that there (still) are some
systems where reading from directories is a valid operation :).

I'm against this change for special files. For directories I have arguments
both for and against, so I don't really know what to say.

[bradfitz, 2012-03-12 17:11:52]

What about the one-liner documentation change I proposed as an alternative
then?

On Mon, Mar 12, 2012 at 10:07 AM, Russ Cox <rsc@golang.org> wrote:

> I think we should not make promises about Size here.
> Implementations will inevitably fail to honor them.
>
> Also, the fact that ls -l /dev/hda doesn't show me the
> size of the disk on Linux is a bug anyway.  I don't want
> to require this behavior.
>
> If this CL were requiring 0 size only for directories, I might
> go along with it, but even then I think it is probably safer
> to write code that doesn't depend on this assumption.
> I know it sucks.
>
> Russ
>

[rsc, 2012-03-12 17:27:40]

On Mon, Mar 12, 2012 at 13:11, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> What about the one-liner documentation change I proposed as an alternative
> then?

Undefined makes it sound like I can return 42.

// length in bytes; system-dependent for device files, directories

[bradfitz, 2012-03-12 17:32:18]

On Mon, Mar 12, 2012 at 10:27 AM, Russ Cox <rsc@golang.org> wrote:

> On Mon, Mar 12, 2012 at 13:11, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > What about the one-liner documentation change I proposed as an
> alternative
> > then?
>
> Undefined makes it sound like I can return 42.
>

it can.  there's no documented meaning for it.


> // length in bytes; system-dependent for device files, directories
>

sure.

[remyoudompheng, 2012-03-12 18:35:56]

On 2012/03/12 17:07:30, rsc wrote:
> I think we should not make promises about Size here.
> Implementations will inevitably fail to honor them.
> 
> Also, the fact that ls -l /dev/hda doesn't show me the
> size of the disk on Linux is a bug anyway.  I don't want
> to require this behavior.

Note that Linux will also pretend most of files in /proc have zero size,
although they are regular and return content indeed.

Rémy.