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

[brad...@golang.org]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@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/

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()

[David Symonds]

LGTM

[Brad Fitzpatrick]

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.

[Rob 'Commander' Pike]

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

[remyoud...@gmail.com]

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?

http://codereview.appspot.com/5798062/

[Brad Fitzpatrick]

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 Hăvărneanu]

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.

[Brad Fitzpatrick]

That was on Go on a Mac.

[Russ Cox]

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 Hăvărneanu]

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.

[Brad Fitzpatrick]

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

[Russ Cox]

On Mon, Mar 12, 2012 at 13:11, Brad Fitzpatrick <brad...@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

[Brad Fitzpatrick]

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

On Mon, Mar 12, 2012 at 13:11, Brad Fitzpatrick <brad...@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.

[remyoud...@gmail.com]

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.



http://codereview.appspot.com/5798062/