[RFC] implementing WalkFollowingSymlinks

654 views
Skip to first unread message

Manlio Perillo

unread,
Jan 14, 2015, 10:57:59 AM1/14/15
to golan...@googlegroups.com
Regards.

I'm trying to implement a WalkFollowingSymlinks function for the path/filepath
package that walks a filesystem tree, following symlinks.
The main problem is detecting loops, however this is not an impossible tasks, and
there are already existing use cases like with the find POSIX utility.

Unfortunately the problem is made harder by the interface of os.FileInfo.
This interface is too much(?) minimal. I assume there is a rationale for
this [1], but it is not at big problem due the availability of the Sys method.

In order to detect loops I need an unique identifier for a file.
On Unix systems this is the pair of device id and inode number.
On Windows this is the pair of volume serial number and file index.

However in os/path_windows.go the volume serial identifier and file index are not available
from the Sys method, but are only stored in the internal fileStat struct, for the use by SameFile,
I assume.  This makes it impossible to store the unique file identifier in a map.

Is it possible to change the Stat function for Windows, in order to store the
syscall.ByHandleFileInformation instance in the sys field?


[1] There is not even an AccessTime, maybe due to common relatime
      and noatime mount options?


Thanks  Manlio Perillo

brainman

unread,
Jan 14, 2015, 5:50:50 PM1/14/15
to golan...@googlegroups.com
> In order to detect loops I need an unique identifier for a file.

Isn't os.SameFile useful here?

Alex

Matthew Dempsky

unread,
Jan 14, 2015, 6:55:34 PM1/14/15
to brainman, golang-dev
On Wed, Jan 14, 2015 at 2:50 PM, brainman <alex.b...@gmail.com> wrote:
> In order to detect loops I need an unique identifier for a file.

Isn't os.SameFile useful here?

Checking O(N) files for repetitions requires O(N^2) calls to os.SameFile(), whereas it would take just O(N) map operations with a unique identifier usable as a map key.

ron minnich

unread,
Jan 14, 2015, 7:12:43 PM1/14/15
to Matthew Dempsky, brainman, golang-dev
I had this problem a few years back (in gproc) and took a slightly different path. The kernel knows how to follow symlinks. It also has rules about cycles (well, too many links really but ...) You don't necessarily know all the kernel's rules, and you need to follow whatever rules the kernel has. Some symlink chains you are ok with might make the kernel upset.

If you're worried about a cycle, or some other problem, do a stat with the pathname first. The kernel will follow a symlink if that's what the path  is. You can get back things like ELOOP, in which case you know there's an issue, or you'll get back some useful info. 

If the stat works, you can then do an lstat and figure out what you're dealing with. 

The common thing people do is an lstat first and then switch behavior once they find it's a symlink, and go into the 'find out if it is a loop' mode. If you do the stat first, that helps you determine if the subsequent lstat, if it shows a link, is a link (or set of links) you can actually do something useful with. Got an error? The chain of symlinks has an issue.

When I took this approach the existing interfaces were more than sufficient.

Sorry if this is not that clear, and it may not help, but it worked very well for me, and in fact replaced the kind of implementation you describe (where the Go code was looking for a cycle on its own by saving pathnames in a map). The kernel has the chops to find problems with symlinks and it pays to let it do that work.

The second thought is you can do the simple thing the kernel does: don't follow too many symlinks. In that case, you probably don't need to look for 'same file', just 'gee this is the 20th symlink in this chain, I give up'.

ron

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

brainman

unread,
Jan 14, 2015, 7:13:18 PM1/14/15
to golan...@googlegroups.com, alex.b...@gmail.com
Sure, it will be slower then it can be. Is it important? Do we want to change things to make it faster?

Alex

Bakul Shah

unread,
Jan 14, 2015, 7:53:14 PM1/14/15
to ron minnich, Matthew Dempsky, brainman, golang-dev
On Thu, 15 Jan 2015 00:12:40 GMT ron minnich <rmin...@gmail.com> wrote:
>
> I had this problem a few years back (in gproc) and took a slightly
> different path. The kernel knows how to follow symlinks. It also has rules
> about cycles (well, too many links really but ...) You don't necessarily
> know all the kernel's rules, and you need to follow whatever rules the
> kernel has. Some symlink chains you are ok with might make the kernel upset.
>
> If you're worried about a cycle, or some other problem, do a stat with the
> pathname first. The kernel will follow a symlink if that's what the path
> is. You can get back things like ELOOP, in which case you know there's an
> issue, or you'll get back some useful info.

A program that follows symlinks and recursively goes down
directories can potentially run forever if it can't determine
whether a directory has been visited before. Consider:

ln -s .. foo

Another case where you need a file's identity is for copying a
dir tree *while* maintaining hard links. Consider:

mkdir a b
cd a
date > 1
ln 1 2
tar cf - . | (cd ..b;tar xf )
ls -li ../a ../b

Note that hardlinked files need not be in the same dir.

More simply, can a compatible tar or rsync be implemented in
Go without O(N^2) use of os.SameFile()?

Ian Lance Taylor

unread,
Jan 14, 2015, 8:28:46 PM1/14/15
to Manlio Perillo, golan...@googlegroups.com
On Wed, Jan 14, 2015 at 7:57 AM, Manlio Perillo
<manlio....@gmail.com> wrote:
>
> Is it possible to change the Stat function for Windows, in order to store
> the
> syscall.ByHandleFileInformation instance in the sys field?

To answer this specific question, we can not change the sys field in
this way, because it would break the Go 1 guarantee.

It sounds like your code will have to be OS-dependent anyhow, so you
can just do the same thing that os.Lstat does on Windows.

That said, I'm not sure it makes sense to add WalkFollowingSymlinks to
path/filepath. That seems special purpose to me and I suspect
different programs will have different requirements for unusual
situations. Of course it could be a useful package outside of the
standard library.

Ian

ron minnich

unread,
Jan 14, 2015, 8:31:35 PM1/14/15
to Bakul Shah, Matthew Dempsky, brainman, golang-dev
On Wed Jan 14 2015 at 4:53:12 PM Bakul Shah <ba...@bitblocks.com> wrote:


A program that follows symlinks and recursively goes down
directories can potentially run forever if it can't determine
whether a directory has been visited before.  Consider:

    ln -s .. foo

If that program doesn't do the simple thing the kernel does, yes. But if that program does do the simple thing the kernel does, namely, refuse to go more than 'n' hops down a chain of symlinks, problem solved. 

Further, I'm advocating that the program do an even simpler thing: stat the path, and if it gets EMLOOP, then the kernel has told you there's an issue with the path.  Let the kernel do the heavy lifting. This worked well for me in gproc when I was shipping whole file system trees over the network.

 ron

Matthew Dempsky

unread,
Jan 14, 2015, 8:47:04 PM1/14/15
to Ian Lance Taylor, Manlio Perillo, golan...@googlegroups.com
On Wed, Jan 14, 2015 at 5:28 PM, Ian Lance Taylor <ia...@golang.org> wrote:
> On Wed, Jan 14, 2015 at 7:57 AM, Manlio Perillo
> <manlio....@gmail.com> wrote:
> >
> > Is it possible to change the Stat function for Windows, in order to store
> > the
> > syscall.ByHandleFileInformation instance in the sys field?
>
> To answer this specific question, we can not change the sys field in
> this way, because it would break the Go 1 guarantee.

A possible Go 1 compatible solution would be:

- Add an opaque/per-OS struct type "SameFileKey".
- Add a "func SameFileKeyFor(fi FileInfo) (SameFileKey, error)"
- Change SameFile's implementation to:

func SameFile(fi1, fi2 FileInfo) bool {
k1, err := SameFileKeyFor(fi1)
if err != nil {
return false
}
k2, err := SameFileKeyFor(fi2)
if err != nil {
return false
}
return k1 == k2
}


Judging by current per-OS SameFile implementations, that would work
fine on current platforms. And opaque structs are usable as map keys,
which meets Manlio's needs.

Bakul Shah

unread,
Jan 14, 2015, 9:17:28 PM1/14/15
to ron minnich, Bakul Shah, Matthew Dempsky, brainman, golang-dev
With a file id it is solved more simply and efficiently
(compared to the BSD copout).

Of course, an even simpler solution is not follow a final
symlink!

But for "maintaining hard links in a dirtree copy " problem
efficiently you do need a file id. Not that many people care
about that any more.

Manlio Perillo

unread,
Jan 15, 2015, 6:46:01 AM1/15/15
to Ian Lance Taylor, golan...@googlegroups.com
On Thu, Jan 15, 2015 at 2:28 AM, Ian Lance Taylor <ia...@golang.org> wrote:
On Wed, Jan 14, 2015 at 7:57 AM, Manlio Perillo
<manlio....@gmail.com> wrote:
>
> Is it possible to change the Stat function for Windows, in order to store
> the
> syscall.ByHandleFileInformation instance in the sys field?

To answer this specific question, we can not change the sys field in
this way, because it would break the Go 1 guarantee.


I this really true?

I found in the commit history that Sys() method was already changed in the past to return more useful informations on Windows, but it was decided to return a copy of syscall.Win32FileAttributeData, instead of a reference of the already available syscall.ByHandleFileInformation.


Looking at the FindNextFile Win32 API function, it seems that this was necessary since WIN32_FIND_DATA struct does not contain the same data as 
BY_HANDLE_FILE_INFORMATION struct, and Win32FileAttributeData just happens to contain common data.

The only solution is to call lstat, in readdir windows implementation, as it is done for all other systems.  It is not possible (IMHO) that Windows gets special treatment resulting in a less usable/portable API.


Thanks and regards  Manlio Perillo

Manlio Perillo

unread,
Jan 15, 2015, 8:25:22 AM1/15/15
to golan...@googlegroups.com, ia...@golang.org
Il giorno giovedì 15 gennaio 2015 12:46:01 UTC+1, Manlio Perillo ha scritto:
> [...]
Looking at the FindNextFile Win32 API function, it seems that this was necessary since WIN32_FIND_DATA struct does not contain the same data as 
BY_HANDLE_FILE_INFORMATION struct, and Win32FileAttributeData just happens to contain common data.

The only solution is to call lstat, in readdir windows implementation, as it is done for all other systems.  It is not possible (IMHO) that Windows gets special treatment resulting in a less usable/portable API.


Well, I was wrong.

A better solution (IMHO, of course) is to make the FileInfo returned by Readdir return WIN32_FIND_DATA, and the FileInfo returned by Stat and Lstat return BY_HANDLE_FILE_INFORMATION.
This avoids the need to create an intermediate Win32FileAttributeData and, most important, avoids to throw away precious information returned by the operating system.


Regards   Manlio Perillo

Ian Lance Taylor

unread,
Jan 15, 2015, 11:14:30 AM1/15/15
to Manlio Perillo, golan...@googlegroups.com
On Thu, Jan 15, 2015 at 3:45 AM, Manlio Perillo
<manlio....@gmail.com> wrote:
> On Thu, Jan 15, 2015 at 2:28 AM, Ian Lance Taylor <ia...@golang.org> wrote:
>>
>> On Wed, Jan 14, 2015 at 7:57 AM, Manlio Perillo
>> <manlio....@gmail.com> wrote:
>> >
>> > Is it possible to change the Stat function for Windows, in order to
>> > store
>> > the
>> > syscall.ByHandleFileInformation instance in the sys field?
>>
>> To answer this specific question, we can not change the sys field in
>> this way, because it would break the Go 1 guarantee.
>>
>
> I this really true?
>
> I found in the commit history that Sys() method was already changed in the
> past to return more useful informations on Windows, but it was decided to
> return a copy of syscall.Win32FileAttributeData, instead of a reference of
> the already available syscall.ByHandleFileInformation.

Hmmm, you're right. I guess that we can change the dynamic type that
the Sys() method returns on Windows without necessarily breaking the
Go 1 guarantee.

Ian

Manlio Perillo

unread,
Jan 15, 2015, 12:31:57 PM1/15/15
to golan...@googlegroups.com, manlio....@gmail.com
Il giorno giovedì 15 gennaio 2015 02:28:46 UTC+1, Ian Lance Taylor ha scritto:
On Wed, Jan 14, 2015 at 7:57 AM, Manlio Perillo
<manlio....@gmail.com> wrote:
>
> Is it possible to change the Stat function for Windows, in order to store
> the
> syscall.ByHandleFileInformation instance in the sys field?

> [...]
 
That said, I'm not sure it makes sense to add WalkFollowingSymlinks to
path/filepath.  That seems special purpose to me and I suspect
different programs will have different requirements for unusual
situations.  Of course it could be a useful package outside of the
standard library.

The reason I want to add WalkFollowingSymlinks to path/filepath is that I would like go tools
to follow symlinks in the go workspaces.

I recently updated my go workspace, replacing my projects with symlinks, and found that
the "..." pattern does not work with symlinks.
The reason why I'm using symlinks is that, as a system admin of my PC:

1) I don't want to take backups of external projects
2) Since I have a *lot* of projects in *different* programming languages, I would
    like to have *all* my projects in the same place.

It is not a big reason, of course, and I can think of some hack to obtain what I want
without using symlinks. But IHMO it is a nice feature to have in future.

Manlio Perillo

unread,
Jan 15, 2015, 1:55:48 PM1/15/15
to golan...@googlegroups.com, ba...@bitblocks.com, mdem...@google.com, alex.b...@gmail.com
Il giorno giovedì 15 gennaio 2015 02:31:35 UTC+1, ron minnich ha scritto:


On Wed Jan 14 2015 at 4:53:12 PM Bakul Shah <ba...@bitblocks.com> wrote:


A program that follows symlinks and recursively goes down
directories can potentially run forever if it can't determine
whether a directory has been visited before.  Consider:

    ln -s .. foo

If that program doesn't do the simple thing the kernel does, yes. But if that program does do the simple thing the kernel does, namely, refuse to go more than 'n' hops down a chain of symlinks, problem solved. 
 
Further, I'm advocating that the program do an even simpler thing: stat the path, and if it gets EMLOOP, then the kernel has told you there's an issue with the path.

If you are referring to
then this is what filepath.EvalSymlinks does, AFAIK.

I have some doubts that stat is capable of detecting loops caused by hard directory links
or something like `mount --bind`.

> [...]

Regards  Manlio Perillo 

Ian Lance Taylor

unread,
Jan 15, 2015, 3:17:56 PM1/15/15
to Manlio Perillo, golan...@googlegroups.com
On Thu, Jan 15, 2015 at 9:31 AM, Manlio Perillo
<manlio....@gmail.com> wrote:
>
> The reason I want to add WalkFollowingSymlinks to path/filepath is that I
> would like go tools
> to follow symlinks in the go workspaces.

I see. I don't see that the go tool needs a general facility for
this. It can check for symlinks in the existing calls to Walk
(fi.Mode()&ModeSymlink != 0) and see if os.Stat on the file returns a
directory. Then use os.Readlink and carry on with appropriate
bookkeeping. Keep a map of the names returned by os.Readlink and fail
if you see the same one more than once.

Ian

Ian Lance Taylor

unread,
Jan 15, 2015, 3:19:24 PM1/15/15
to Manlio Perillo, golan...@googlegroups.com
In particular the go tool doesn't care about loops as such. It cares
about the same directory appearing more than once in GOPATH--that
would make little sense. If we catch that case, we'll catch loops.

Ian

brainman

unread,
Jan 15, 2015, 5:33:27 PM1/15/15
to golan...@googlegroups.com, ia...@golang.org
On Friday, 16 January 2015 00:25:22 UTC+11, Manlio Perillo wrote:

A better solution (IMHO, of course) is to make the FileInfo returned by Readdir return WIN32_FIND_DATA, and the FileInfo returned by Stat and Lstat return BY_HANDLE_FILE_INFORMATION.
This avoids the need to create an intermediate Win32FileAttributeData and, most important, avoids to throw away precious information returned by the operating system.


Please create a new issue https://github.com/golang/go/issues/new with this request. If it is accepted, I will try to implement it. Unless someone beats me to it. Thank you.

Alex

minux

unread,
Jan 15, 2015, 6:18:38 PM1/15/15
to Ian Lance Taylor, Manlio Perillo, golan...@googlegroups.com
Why? Changing the dynamic type will break all the code that is using .Sys
field. This is not covered by the api test, so I'd like to believe the old change
is an oversight rather than a precedent.

Lucio De Re

unread,
Jan 16, 2015, 1:22:46 AM1/16/15
to minux, Ian Lance Taylor, Manlio Perillo, golan...@googlegroups.com
On 1/16/15, minux <mi...@golang.org> wrote:

> Why? Changing the dynamic type will break all the code that is using .Sys
> field. This is not covered by the api test, so I'd like to believe the old
> change is an oversight rather than a precedent.
>
I expect you're right, but if the first change did not cause chaos,
it's worth considering that a second one may also be tolerable. Which
means that this particular aspect need not be as important a
consideration as other, more interesting ones.

That's me just adding a perspective, not making any recommendations.

Lucio.

Manlio Perillo

unread,
Jan 16, 2015, 5:55:43 AM1/16/15
to minux, Ian Lance Taylor, golan...@googlegroups.com
On Fri, Jan 16, 2015 at 12:18 AM, minux <mi...@golang.org> wrote:

> [...]

Hmmm, you're right.  I guess that we can change the dynamic type that
the Sys() method returns on Windows without necessarily breaking the
Go 1 guarantee.
Why? Changing the dynamic type will break all the code that is using .Sys
field.

The documentation of the Sys method says:
"underlying data source (can return nil)"

This means two things for me:

1) The dynamic type is implementation defined.
2) The dynamic type is supposed to return the actual type, if any,
    used by the implementation.
    The Windows implementation does not do this.

So it should be ok to:

1) Change the dynamic type.
2) Return a different dynamic type based on how the FileInfo
    was obtained.

> [...]

Regards  Manlio

Manlio Perillo

unread,
Jan 16, 2015, 5:59:36 AM1/16/15
to Ian Lance Taylor, golan...@googlegroups.com
On Thu, Jan 15, 2015 at 9:19 PM, Ian Lance Taylor <ia...@golang.org> wrote:
> [...]

> I see.  I don't see that the go tool needs a general facility for
> this.  It can check for symlinks in the existing calls to Walk
> (fi.Mode()&ModeSymlink != 0) and see if os.Stat on the file returns a
> directory.  Then use os.Readlink and carry on with appropriate
> bookkeeping.  Keep a map of the names returned by os.Readlink and fail
> if you see the same one more than once.

In particular the go tool doesn't care about loops as such.  It cares
about the same directory appearing more than once in GOPATH--that
would make little sense.  If we catch that case, we'll catch loops.

Then it should be easy and efficient to guard against this kind of loop
using symlinks, in a WalkFollowingSymlinks function.

I'm working on the code.  To make development easy I have created an external
package that mimics the standard path/filepath package.


Regards  Manlio

Manlio Perillo

unread,
Jan 16, 2015, 6:22:39 AM1/16/15
to brainman, golan...@googlegroups.com, Ian Lance Taylor

Ian Lance Taylor

unread,
Jan 16, 2015, 10:33:54 AM1/16/15
to Manlio Perillo, golan...@googlegroups.com
On Fri, Jan 16, 2015 at 2:59 AM, Manlio Perillo
<manlio....@gmail.com> wrote:
> On Thu, Jan 15, 2015 at 9:19 PM, Ian Lance Taylor <ia...@golang.org> wrote:
>>
>> > [...]
>> > I see. I don't see that the go tool needs a general facility for
>> > this. It can check for symlinks in the existing calls to Walk
>> > (fi.Mode()&ModeSymlink != 0) and see if os.Stat on the file returns a
>> > directory. Then use os.Readlink and carry on with appropriate
>> > bookkeeping. Keep a map of the names returned by os.Readlink and fail
>> > if you see the same one more than once.
>>
>> In particular the go tool doesn't care about loops as such. It cares
>> about the same directory appearing more than once in GOPATH--that
>> would make little sense. If we catch that case, we'll catch loops.
>
>
> Then it should be easy and efficient to guard against this kind of loop
> using symlinks, in a WalkFollowingSymlinks function.

I'm not yet persuaded that a general WalkFollowingSymlinks function is
meaningful. I think it will have to adopt a specific strategy for
loops that may not be appropriate for all users. Of course I could be
wrong. This is not a reason not to write such a function, but it may
be a reason not to put it in the standard library.

My suggestion about the go tool does not require any change to what
the FileInfo.Sys method returns.

Ian

Manlio Perillo

unread,
Jan 16, 2015, 10:59:05 AM1/16/15
to Ian Lance Taylor, golan...@googlegroups.com
filepath is called 3 times in cmd/go/main.go, and all calls use a closure.
Adding custom support for symlinks may cause many non trivial changes, to an already complex code base.

> I'm not yet persuaded that a general WalkFollowingSymlinks function is
> meaningful.  I think it will have to adopt a specific strategy for
> loops that may not be appropriate for all users

WalkFollowingSymlinks can be implemented so that it works as required by the go tools.

Regards  Manlio Perillo

Ian Lance Taylor

unread,
Jan 16, 2015, 11:10:57 AM1/16/15
to Manlio Perillo, golan...@googlegroups.com
Sure, but why not put that version in the go tool sources rather than
in path/filepath?

path/filepath, like all other standard library packages, needs to have
a clean API and needs to work for a variety of users. If you can
write a version of WalkFollowingSymlinks that meets those criteria,
then, great. Do that. I suggest that you describe it here first to
see whether others agree the API.

Ian

minux

unread,
Jan 16, 2015, 7:28:05 PM1/16/15
to Manlio Perillo, Ian Lance Taylor, golan...@googlegroups.com
What problem are you trying to solve in the go tools?

The cmd/go tool itself doesn't handle symlinks in GOPATH, so nor should the
tools in the tools sub-repo.

minux

unread,
Jan 16, 2015, 7:32:10 PM1/16/15
to Manlio Perillo, Ian Lance Taylor, golan...@googlegroups.com
This might be OK, but I read your proposal correctly, you want to change the Sys()
from a FileInfo obtained through os.Stat.

That is problematic. Imagine how people will try to use that Sys() method. If we
changes what Sys() returns in Go 1.5, they will have to handle another type.

Your API proposal already needs to be platform specific, why not just use windows
syscalls and keep the os package as is?

Manlio Perillo

unread,
Jan 17, 2015, 11:49:59 AM1/17/15
to minux, Ian Lance Taylor, golan...@googlegroups.com
On Sat, Jan 17, 2015 at 1:27 AM, minux <mi...@golang.org> wrote:

> [...] 
WalkFollowingSymlinks can be implemented so that it works as required by the go tools.
What problem are you trying to solve in the go tools?

The cmd/go tool itself doesn't handle symlinks in GOPATH, so nor should the
tools in the tools sub-repo.

Since I find symlinks in GOPATH useful I have patched the cmd/go tool to support symlinks on my system. They allow a local version of vanity imports.
Of course, as I wrote, I can live without them.

However I'm curious to know why symlinks in GOPATH are not supported.
Is the reason "political" or technical?

Thanks  Manlio Perillo

Manlio Perillo

unread,
Jan 17, 2015, 11:54:19 AM1/17/15
to minux, Ian Lance Taylor, golan...@googlegroups.com
On Sat, Jan 17, 2015 at 1:31 AM, minux <mi...@golang.org> wrote:

> [...] 
So it should be ok to:

1) Change the dynamic type. 
2) Return a different dynamic type based on how the FileInfo
    was obtained.
This might be OK, but I read your proposal correctly, you want to change the Sys()
from a FileInfo obtained through os.Stat.

That is problematic. Imagine how people will try to use that Sys() method. If we
changes what Sys() returns in Go 1.5, they will have to handle another type.


Yes, it is problematic.
However go already changed some implementation specific API.
 
Your API proposal already needs to be platform specific, why not just use windows
syscalls and keep the os package as is?

It can be done, of course.
But do we really want os.Stat to return incomplete/truncated informations from the OS? Since Go is a system language I find this is not acceptable.


Regards  Manlio

minux

unread,
Jan 17, 2015, 4:23:54 PM1/17/15
to Manlio Perillo, Ian Lance Taylor, golan...@googlegroups.com

On Sat, Jan 17, 2015 at 11:49 AM, Manlio Perillo <manlio....@gmail.com> wrote:
On Sat, Jan 17, 2015 at 1:27 AM, minux <mi...@golang.org> wrote:
> [...] 
WalkFollowingSymlinks can be implemented so that it works as required by the go tools.
What problem are you trying to solve in the go tools?

The cmd/go tool itself doesn't handle symlinks in GOPATH, so nor should the
tools in the tools sub-repo.
Since I find symlinks in GOPATH useful I have patched the cmd/go tool to support symlinks on my system. They allow a local version of vanity imports.
Of course, as I wrote, I can live without them.
See, this is the real problem. Without cmd/go support, adding the support to the tools in
x/tools subrepo is just going to create incompatibilities.

On Sat, Jan 17, 2015 at 1:31 AM, minux <mi...@golang.org> wrote:
> [...] 
So it should be ok to:

1) Change the dynamic type. 
2) Return a different dynamic type based on how the FileInfo
    was obtained.
This might be OK, but I read your proposal correctly, you want to change the Sys()
from a FileInfo obtained through os.Stat.

That is problematic. Imagine how people will try to use that Sys() method. If we
changes what Sys() returns in Go 1.5, they will have to handle another type.
Yes, it is problematic.
However go already changed some implementation specific API.
That's not the reason for making changes again, lightly. We should keep existing programs
using the Sys method of fileinfo returned from os.Stat working. Just because it's not in the
Go 1 API guarantee doesn't mean we can break those programs freely.

Besides, there is no fundamental problems with the current Win32FileAttributeData result.

For volume serial number and file index, just use GetFileInformationByHandle.
Actually, as os.SameFile just uses two FileInfo, and do not use any syscalls, so O(N^2)
behavior is not that bad. If that is truly inefficient, we might be able to introduce os.File.UniqueKey
method, but that's another discussion (I don't want it to return interface{}, I think it should return
string or fill in []byte, or return a Stringer). I imagine net/http might also need it to generate
ETag for ServeFile, so it's generally useful. Besides, portable interfaces is always better
than non-portable OS-specific interfaces.

Your API proposal already needs to be platform specific, why not just use windows
syscalls and keep the os package as is?

It can be done, of course.
But do we really want os.Stat to return incomplete/truncated informations from the OS? Since Go is a system language I find this is not acceptable.
The os package is not meant to cover all possible use cases.
(Nor is it possible to cover all possible use case while still provide a portable API.)

brainman

unread,
Jan 18, 2015, 12:42:11 AM1/18/15
to golan...@googlegroups.com, mi...@golang.org, ia...@golang.org
> On Sunday, 18 January 2015 03:54:19 UTC+11, Manlio Perillo wrote:
> > On Sat, Jan 17, 2015 at 1:31 AM, minux <mi...@golang.org> wrote:
> > 
> > That is problematic. Imagine how people will try to use that Sys() method. If we
> > changes what Sys() returns in Go 1.5, they will have to handle another type.
> Yes, it is problematic.

It is. Imaging people, who's code is suddenly broken, complaining about it. We need some good story to explain why we changed that code. It has to sounds reasonable for them. We need to be able to show them they suffered for a good reason.  I don't see we have that story here.

> However go already changed some implementation specific API.

Maybe. But I don't see how is it relevant here.

> It can be done, of course.

So you have solution to your problem.

> But do we really want os.Stat to return incomplete/truncated informations from the OS? ...

syscall package is mainly for std repo developers to implement OS specific code. If it is useful for others it's a bonus.

I agree with minux here. We won't make that change. Sorry.

I am going to close issue #9611 you've raised about this as "unfortunate".

Alex

Manlio Perillo

unread,
Jan 20, 2015, 2:49:29 PM1/20/15
to golan...@googlegroups.com, manlio....@gmail.com


Il giorno venerdì 16 gennaio 2015 16:33:54 UTC+1, Ian Lance Taylor ha scritto:
> [...] 
>
> Then it should be easy and efficient to guard against this kind of loop
> using symlinks, in a WalkFollowingSymlinks function.

I'm not yet persuaded that a general WalkFollowingSymlinks function is
meaningful.  I think it will have to adopt a specific strategy for
loops that may not be appropriate for all users.  Of course I could be
wrong.

It seems you were right.
I could not came up with an optimal and generic implementation.

I tried to define an helper StatFollowingSymlinks.
The functions simply ignores any links evaluation errors, but some
users may want to be notified about such errors.

Then I decided to implement a reusable filesystem loop detection, but this is not possible due
to os.Stat not returning the required informations on Windows, and if the function must be called with
a FileInfo obtained by a custom Stat function, then it is not reusable at all since it can not be used with
filepath.Walk.

> [...]

Thanks for the discussion and sorry for the noise.
I solved my original problem (keep my Go projects separate from external projects) using
the *simple* solution of having two directories in the GOPATH environment variable.


Manlio Perillo 

Manlio Perillo

unread,
Jan 20, 2015, 2:56:08 PM1/20/15
to golan...@googlegroups.com, manlio....@gmail.com, ia...@golang.org

Il giorno sabato 17 gennaio 2015 22:23:54 UTC+1, minux ha scritto:

> [...]
 
Besides, there is no fundamental problems with the current Win32FileAttributeData result.

For volume serial number and file index, just use GetFileInformationByHandle.

I'm assuming you already have called os.Stat.
Making another syscall to obtain the same information you already have is not really
something a system language should do, IMHO.

Actually, as os.SameFile just uses two FileInfo, and do not use any syscalls, so O(N^2)
behavior is not that bad. If that is truly inefficient, we might be able to introduce os.File.UniqueKey
method, but that's another discussion (I don't want it to return interface{}, I think it should return
string or fill in []byte, or return a Stringer). I imagine net/http might also need it to generate
ETag for ServeFile, so it's generally useful. Besides, portable interfaces is always better
than non-portable OS-specific interfaces.

This is a good idea.
The reason I needed the change to FileInfo.Sys was precisely for being able to define a portable
interface atop of FileInfo.
But is such interface is implemented in os it's better.
 

Your API proposal already needs to be platform specific, why not just use windows
syscalls and keep the os package as is?


syscalls are not free.

> [...]


Regards  Manlio Perillo
Reply all
Reply to author
Forward
0 new messages