WIP: Improved Stat() and Mode() handling for added files #1413
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tories For example, we want to crash if we hit a named pipe, not block forever trying to read from it. License: MIT Signed-off-by: W. Trevor King <[email protected]>
We need Stat() calls to figure out whether a file is a directory, regular file, or other object (e.g. a symlink, named pipe, etc.). This commit shifts our File interface to more closely match os.File. I'm a bit concerned that the backing Stat calls on any underlying os.File is done at wrapper-creation time and not at wrapper.Stat() time. That means we'll get stale data with a workflow like: 1. Create a new commands/files/... File (e.g. via NewSerialFile) 2. Edit the backing file (e.g. appending data to it). 3. Call f.Stat() on our wrapping file to get a FileInfo type. In that case, fi.Size() and fi.ModTime() on the returned FileInfo from (3) will reflect the values that the file had at (1), and not the values that it should have at (3). Maybe we're mostly using our File wrappers as read- or write-only entities and then throwing them away, in which case this discrepancy may not be too important. But it's still a race hole I'd like to close. License: MIT Signed-off-by: W. Trevor King <[email protected]>
Without the O_NONBLOCK flag:
Open("/path/to/named-pipe")
will hang in a syscall (on Linux 4.1.0-rc8 anyway). The Go
stack-trace looks like:
syscall.Syscall(0x2, 0xc20969e870, 0x80000, 0x0, 0x0, 0xc2096ad040, 0xc2089a7a78)
/usr/lib/go/src/syscall/asm_linux_amd64.s:21 +0x5 fp=0xc2089a79f8 sp=0xc2089a79f0
syscall.open(0xc20969e840, 0x8, 0x80000, 0x0, 0x0, 0x0, 0x0)
/usr/lib/go/src/syscall/zsyscall_linux_amd64.go:16 +0xaa fp=0xc2089a7a58 sp=0xc2089a79f8
syscall.Open(0xc20969e840, 0x8, 0x80000, 0x0, 0x12, 0x0, 0x0)
/usr/lib/go/src/syscall/syscall_linux.go:23 +0x5a fp=0xc2089a7a98 sp=0xc2089a7a58
os.OpenFile(0xc20969e840, 0x8, 0x0, 0x0, 0xc2089a7bd0, 0x0, 0x0)
/usr/lib/go/src/os/file_unix.go:78 +0x6f fp=0xc2089a7af0 sp=0xc2089a7a98
os.Open(0xc20969e840, 0x8, 0xc2089a7bd0, 0x0, 0x0)
/usr/lib/go/src/os/file.go:239 +0x55 fp=0xc2089a7b30 sp=0xc2089a7af0
github.com/ipfs/go-ipfs/commands/files.(*serialFile).NextFile(0xc20806dd80, 0x0, 0x0, 0x0, 0x0)
/home/wking/src/go/src/github.com/ipfs/go-ipfs/commands/files/serialfile.go:90 +0x3e0 fp=0xc2089a7c60 sp=0xc2089a7b30
...
With this non-blocking open, the open (unsurprisingly ;) doesn't
block. I haven't audited downstream usuage to make sure it handles a
non-blocking file, but with this change I get:
$ ipfs add -r /path/to/dir
Error: `/path/to/dir/named-pipe` is neither a directory nor a file
instead of the hung syscall when the named pipe has no data in it.
License: MIT
Signed-off-by: W. Trevor King <[email protected]>
Without the O_NONBLOCK flag:
Open("/path/to/named-pipe")
will hang in a syscall (on Linux 4.1.0-rc8 anyway). The Go
stack-trace looks like:
syscall.Syscall(0x2, 0xc208080d51, 0x80000, 0x0, 0x0, 0x1, 0x41a31f)
/usr/lib/go/src/syscall/asm_linux_amd64.s:21 +0x5 fp=0xc2080a75c8 sp=0xc2080a75c0
syscall.open(0x7ffc89956140, 0x8, 0x80000, 0x100000000, 0x0, 0x0, 0x0)
/usr/lib/go/src/syscall/zsyscall_linux_amd64.go:16 +0xaa fp=0xc2080a7628 sp=0xc2080a75c8
syscall.Open(0x7ffc89956140, 0x8, 0x80000, 0x0, 0x6, 0x0, 0x0)
/usr/lib/go/src/syscall/syscall_linux.go:23 +0x5a fp=0xc2080a7668 sp=0xc2080a7628
os.OpenFile(0x7ffc89956140, 0x8, 0x0, 0x0, 0x7f0f795e2408, 0x0, 0x0)
/usr/lib/go/src/os/file_unix.go:78 +0x6f fp=0xc2080a76c0 sp=0xc2080a7668
os.Open(0x7ffc89956140, 0x8, 0xc3b680, 0x0, 0x0)
/usr/lib/go/src/os/file.go:239 +0x55 fp=0xc2080a7700 sp=0xc2080a76c0
github.com/ipfs/go-ipfs/commands/cli.appendFile(0xc208080d70, 0x0, 0x1, 0xc20802a930, 0x1, 0x3, 0x11c3460, 0xc208080d01, 0x0, 0x0, ...)
/home/wking/src/go/src/github.com/ipfs/go-ipfs/commands/cli/parse.go:342 +0xbb fp=0xc2080a7810 sp=0xc2080a7700
...
With this non-blocking open, the open (unsurprisingly ;) doesn't
block. I haven't audited downstream usuage to make sure it handles a
non-blocking file, but with this change I get:
$ ipfs add -r /path/to/namrd-pipe
Error: `/path/to/named-pipe` is neither a directory nor a file
instead of the hung syscall when the named pipe has no data in it.
License: MIT
Signed-off-by: W. Trevor King <[email protected]>
wking
force-pushed
the
tk/check-for-non-file-or-dir-modes
branch
3 times, most recently
from
d593388
to
dd9bad0
Compare
License: MIT Signed-off-by: W. Trevor King <[email protected]>
For streaming responses (e.g. 'ipfs add -r /path/to/root/directory'), we may hit errors later on in the stream, after we've already written the initial 200 response and started in on the streaming body (e.g. /path/to/root/directory/a/b/c is an unsupported file type). We should check for that sort of error, and set a trailer [1] if there were any errors. Unfortunately, it's a bit awkward to set these headers in Go 1.4 and earlier [2], but [2] does have a workaround for Go 1.4 (it's not clear if the workaround is compatible with Go 1.3). [1]: http://tools.ietf.org/html/rfc2616#section-14.40 [2]: golang/go#7759 License: MIT Signed-off-by: W. Trevor King <[email protected]>
Make sure we neither hang nor silently tread named pipes as regular files. License: MIT Signed-off-by: W. Trevor King <[email protected]>
Make sure we neither hang nor silently tread named pipes as regular files. License: MIT Signed-off-by: W. Trevor King <[email protected]>
On Linux, open() will follow symlinks unless you use the O_NOFOLLOW flag (added in Linux 2.1.126). Then it will error out with ELOOP unless you've set O_PATH. Unfortunately, Go doesn't define O_PATH and has no plans to do so [1]. The other tricky bit here is extracting the errno [2] from the *PathError [3] returned by os.Open [4]. The new files.Errno tries to cast the error instance to an *os.PathError. If that succeeds, it looks at the *os.PathError's Err attribute and tries to cast that to an Errno. If that succeeds it returns the extracted errno. With this commit, we now have File object for the symlinks with the correct filename and mode. I've also created a string-based reader to pass along the link target, so the adder will have it available when it gains support for these types of files. [1]: golang/go#7830 [2]: https://golang.org/pkg/syscall/#Errno [3]: https://golang.org/pkg/os/#PathError [4]: https://golang.org/pkg/os/#Open License: MIT Signed-off-by: W. Trevor King <[email protected]>
| @@ -335,8 +340,16 @@ func appendStdinAsString(args []string, stdin *os.File) ([]string, *os.File, err | |||
| func appendFile(args []files.File, inputs []string, argDef *cmds.Argument, recursive bool) ([]files.File, []string, error) { | |||
| path := inputs[0] | |||
|
|
|||
| file, err := os.Open(path) | |||
| file, err := os.OpenFile(path, (os.O_RDONLY | syscall.O_NOFOLLOW | syscall.O_NONBLOCK), 0) | |||
There was a problem hiding this comment.
lets just call stat before we call open. It might introduce a slight race, but its better than breaking our cross platformability with importing syscall.
|
@wking update? whats needed here? |
|
ping |
|
closing, old PR cleanup time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Don't merge this yet, it's a work in progress.
Things like adding named-pipes don't work at the moment, because we
currently assume files are either regular files or directories, and
don't check mode flags for other options (symlinks, named-pipes,
etc.). This branch tries to shift us towards explicit mode handling
so we can gracefully die when we hit file types that don't yet have
IPFS analogs.