code review 5448079: os: replace interface{} by *Stat_t in FileInfo.Sys (issue 5448079)

[n13m...@gmail.com]

Reviewers: rsc,

Message:
Hello r...@golang.org (cc: golan...@googlegroups.com),

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

Description:
os: replace interface{} by *Stat_t in FileInfo.Sys

This reduces a bit the overhead necessary to work with
OS-specific file details, while preserving the separation
of concerns, effectively turning expressions such as:

stat.(*os.FileInfo).Sys.(*syscall.Stat_t).Uid

Onto:

stat.(*os.FileInfo).Sys.Uid

There's precedent for this approach in the Sys field
of os.ProcAttr.

Please review this at http://codereview.appspot.com/5448079/

Affected files:
M src/pkg/os/os_unix_test.go
M src/pkg/os/stat_linux.go
M src/pkg/os/types.go

Index: src/pkg/os/os_unix_test.go
===================================================================
--- a/src/pkg/os/os_unix_test.go
+++ b/src/pkg/os/os_unix_test.go
@@ -17,7 +17,7 @@
if err != nil {
t.Fatalf("Stat %q (looking for uid/gid %d/%d): %s", path, uid, gid, err)
}
- sys := dir.(*FileStat).Sys.(*syscall.Stat_t)
+ sys := dir.(*FileStat).Sys
if int(sys.Uid) != uid {
t.Errorf("Stat %q: uid %d want %d", path, sys.Uid, uid)
}
@@ -51,7 +51,7 @@
if err = Chown(f.Name(), -1, gid); err != nil {
t.Fatalf("chown %s -1 %d: %s", f.Name(), gid, err)
}
- sys := dir.(*FileStat).Sys.(*syscall.Stat_t)
+ sys := dir.(*FileStat).Sys
checkUidGid(t, f.Name(), int(sys.Uid), gid)

// Then try all the auxiliary groups.
Index: src/pkg/os/stat_linux.go
===================================================================
--- a/src/pkg/os/stat_linux.go
+++ b/src/pkg/os/stat_linux.go
@@ -10,8 +10,8 @@
)

func sameFile(fs1, fs2 *FileStat) bool {
- sys1 := fs1.Sys.(*syscall.Stat_t)
- sys2 := fs2.Sys.(*syscall.Stat_t)
+ sys1 := fs1.Sys
+ sys2 := fs2.Sys
return sys1.Dev == sys2.Dev && sys1.Ino == sys2.Ino
}

@@ -52,5 +52,5 @@

// For testing.
func atime(fi FileInfo) time.Time {
- return timespecToTime(fi.(*FileStat).Sys.(*syscall.Stat_t).Atim)
+ return timespecToTime(fi.(*FileStat).Sys.Atim)
}
Index: src/pkg/os/types.go
===================================================================
--- a/src/pkg/os/types.go
+++ b/src/pkg/os/types.go
@@ -96,7 +96,11 @@
mode FileMode
modTime time.Time

- Sys interface{}
+ // Operating system-specific details about the file.
+ // Note that accessing this field means that your program
+ // may not execute properly or even compile on some
+ // operating systems.
+ Sys *syscall.Stat_t
}

func (fs *FileStat) Name() string { return fs.name }

[Russ Cox]

This fails on systems without a Stat_t, sorry.

[Russ Cox]

On Thu, Dec 1, 2011 at 16:06, Russ Cox <r...@golang.org> wrote:
> This fails on systems without a Stat_t, sorry.

Also, and this might be worse, it means that you
can get at syscall-specific data without importing
syscall, which means you don't even know that
you're doing it.

I'd like to sit on this for a few days and think a bit
more about how to make this easier. I don't mind
it being easier but I like very much the fact that
Unix-specific programs are marked as such, and
I would prefer to see a way to make things
easier that preserves that property.

Russ

[Gustavo Niemeyer]

> This fails on systems without a Stat_t, sorry.

These may easily define it as struct{}, or interface{} for that matter.

(...)

> I'd like to sit on this for a few days and think a bit
> more about how to make this easier.  I don't mind
> it being easier but I like very much the fact that
> Unix-specific programs are marked as such, and
> I would prefer to see a way to make things
> easier that preserves that property.

I appreciate that people know they are touching system specific data
as well, and I'll be happy to see an approach that preserves that
property. The problem I'm trying to address is that while migrating
actual code to the new approach, it's looking overly verbose to do
things that are often necessary when people are interacting with
common O.S. specific file details.

Let me try a slightly different approach that you may like better.

--
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[Russ Cox]

On Thu, Dec 1, 2011 at 16:20, Gustavo Niemeyer <n13m...@gmail.com>
wrote:>> This fails on systems without a Stat_t, sorry.>> These may

easily define it as struct{}, or interface{} for that matter.

I started down that path.  struct{} is useless, becauseyou do want to
be able to store some associated dataon those systems, and interface{}
is surprising, becausethen *Stat_t is *interface{}.  And on Windows
there is noteven a single struct type: depending on whether the info
came from Readdir or Stat, it has different types.

> I appreciate that people know they are touching system specific data
> as well, and I'll be happy to see an approach that preserves that
> property. The problem I'm trying to address is that while migrating
> actual code to the new approach, it's looking overly verbose to do
> things that are often necessary when people are interacting with
> common O.S. specific file details.

I understand and appreciate that. However, it is worth pointing
out that you don't have to migrate actual code to the new
approach today. This is a weekly snapshot, not a release.
I'd be happy to discuss proposals about ways to make this
better, but let's do that (discuss) instead of sending CLs.

Russ

[Russ Cox]

On Thu, Dec 1, 2011 at 16:26, Russ Cox <r...@golang.org> wrote:
> On Thu, Dec 1, 2011 at 16:20, Gustavo Niemeyer <n13m...@gmail.com>
> wrote:>> This fails on systems without a Stat_t, sorry.>> These may
> easily define it as struct{}, or interface{} for that matter.
> I started down that path.  struct{} is useless, becauseyou do want to
> be able to store some associated dataon those systems, and interface{}
> is surprising, becausethen *Stat_t is *interface{}.  And on Windows
> there is noteven a single struct type: depending on whether the info
> came from Readdir or Stat, it has different types.

I apologize on Chrome's behalf.

[Gustavo Niemeyer]

What about introducing in syscall:

   func Stat(s Iface) *Stat_t

where Iface is defined as:

    type Iface interface {
        func SysStat() interface{}
    }

and replacing the Sys field by SysStat() in os.FileInfo?

On Dec 1, 2011 7:26 PM, "Russ Cox" <r...@golang.org> wrote:

[Gustavo Niemeyer]

> I understand and appreciate that.  However, it is worth pointing
> out that you don't have to migrate actual code to the new
> approach today.  This is a weekly snapshot, not a release.
> I'd be happy to discuss proposals about ways to make this
> better, but let's do that (discuss) instead of sending CLs.

Also, FWIW, one of the points of migrating now is precisely so that I
can get a feeling for the API so that we can have that conversation
while it's still comfortable for everybody to improve the interface.

[Gustavo Niemeyer]

> What about introducing in syscall:

I've experimented a bit with this idea and, to be honest, even moving
the system-specific value to the Sys() function onto the FileInfo
interface itself would already feel much better:

stat.Sys().(*syscall.Stat_t)

[n13m...@gmail.com]

> stat.Sys().(*syscall.Stat_t)

I have updated the CL to implement this approach, in case someone is
interested in observing how it would look like.

http://codereview.appspot.com/5448079/

[c...@f00f.org]

For posix-like systems might we not just have something like

pstat := os.PosixStat(someFileInfo)

then pstat.Dev could be used directly similar to what was possible
before?

http://codereview.appspot.com/5448079/