-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move File and IO::FileDescriptor's platform-specific parts to Crystal::System #5333
Merged
+577
−323
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
require "./unix/file" |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
require "./unix/file_descriptor" |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,230 @@ | ||
require "c/sys/file" | ||
|
||
# :nodoc: | ||
module Crystal::System::File | ||
def self.open(filename, mode, perm) | ||
oflag = open_flag(mode) | LibC::O_CLOEXEC | ||
|
||
fd = LibC.open(filename.check_no_null_byte, oflag, perm) | ||
if fd < 0 | ||
raise Errno.new("Error opening file '#{filename}' with mode '#{mode}'") | ||
end | ||
fd | ||
end | ||
|
||
private def self.open_flag(mode) | ||
if mode.size == 0 | ||
raise "Invalid access mode #{mode}" | ||
end | ||
|
||
m = 0 | ||
o = 0 | ||
case mode[0] | ||
when 'r' | ||
m = LibC::O_RDONLY | ||
when 'w' | ||
m = LibC::O_WRONLY | ||
o = LibC::O_CREAT | LibC::O_TRUNC | ||
when 'a' | ||
m = LibC::O_WRONLY | ||
o = LibC::O_CREAT | LibC::O_APPEND | ||
else | ||
raise "Invalid access mode #{mode}" | ||
end | ||
|
||
case mode.size | ||
when 1 | ||
# Nothing | ||
when 2 | ||
case mode[1] | ||
when '+' | ||
m = LibC::O_RDWR | ||
when 'b' | ||
# Nothing | ||
else | ||
raise "Invalid access mode #{mode}" | ||
end | ||
else | ||
raise "Invalid access mode #{mode}" | ||
end | ||
|
||
oflag = m | o | ||
end | ||
|
||
def self.mktemp(name, extension) | ||
tmpdir = tempdir + ::File::SEPARATOR | ||
path = "#{tmpdir}#{name}.XXXXXX#{extension}" | ||
|
||
if extension | ||
fd = LibC.mkstemps(path, extension.bytesize) | ||
else | ||
fd = LibC.mkstemp(path) | ||
end | ||
|
||
raise Errno.new("mkstemp") if fd == -1 | ||
{fd, path} | ||
end | ||
|
||
def self.tempdir | ||
tmpdir = ENV["TMPDIR"]? || "/tmp" | ||
tmpdir.rchop(::File::SEPARATOR) | ||
end | ||
|
||
def self.stat(path) | ||
if LibC.stat(path.check_no_null_byte, out stat) != 0 | ||
raise Errno.new("Unable to get stat for '#{path}'") | ||
end | ||
::File::Stat.new(stat) | ||
end | ||
|
||
def self.lstat(path) | ||
if LibC.lstat(path.check_no_null_byte, out stat) != 0 | ||
raise Errno.new("Unable to get lstat for '#{path}'") | ||
end | ||
::File::Stat.new(stat) | ||
end | ||
|
||
def self.empty?(path) | ||
begin | ||
stat(path).size == 0 | ||
rescue Errno | ||
raise Errno.new("Error determining size of '#{path}'") | ||
end | ||
end | ||
|
||
def self.exists?(path) | ||
accessible?(path, LibC::F_OK) | ||
end | ||
|
||
def self.readable?(path) : Bool | ||
accessible?(path, LibC::R_OK) | ||
end | ||
|
||
def self.writable?(path) : Bool | ||
accessible?(path, LibC::W_OK) | ||
end | ||
|
||
def self.executable?(path) : Bool | ||
accessible?(path, LibC::X_OK) | ||
end | ||
|
||
private def self.accessible?(path, flag) | ||
LibC.access(path.check_no_null_byte, flag) == 0 | ||
end | ||
|
||
def self.file?(path) : Bool | ||
if LibC.stat(path.check_no_null_byte, out stat) != 0 | ||
if Errno.value == Errno::ENOENT | ||
return false | ||
else | ||
raise Errno.new("stat") | ||
end | ||
end | ||
::File::Stat.new(stat).file? | ||
end | ||
|
||
def self.chown(path, uid : Int, gid : Int, follow_symlinks) | ||
ret = if !follow_symlinks && symlink?(path) | ||
LibC.lchown(path, uid, gid) | ||
else | ||
LibC.chown(path, uid, gid) | ||
end | ||
raise Errno.new("Error changing owner of '#{path}'") if ret == -1 | ||
end | ||
|
||
def self.chmod(path, mode : Int) | ||
if LibC.chmod(path, mode) == -1 | ||
raise Errno.new("Error changing permissions of '#{path}'") | ||
end | ||
end | ||
|
||
def self.delete(path) | ||
err = LibC.unlink(path.check_no_null_byte) | ||
if err == -1 | ||
raise Errno.new("Error deleting file '#{path}'") | ||
end | ||
end | ||
|
||
def self.real_path(path) | ||
real_path_ptr = LibC.realpath(path, nil) | ||
raise Errno.new("Error resolving real path of #{path}") unless real_path_ptr | ||
String.new(real_path_ptr).tap { LibC.free(real_path_ptr.as(Void*)) } | ||
end | ||
|
||
def self.link(old_path, new_path) | ||
ret = LibC.link(old_path.check_no_null_byte, new_path.check_no_null_byte) | ||
raise Errno.new("Error creating link from #{old_path} to #{new_path}") if ret != 0 | ||
ret | ||
end | ||
|
||
def self.symlink(old_path, new_path) | ||
ret = LibC.symlink(old_path.check_no_null_byte, new_path.check_no_null_byte) | ||
raise Errno.new("Error creating symlink from #{old_path} to #{new_path}") if ret != 0 | ||
ret | ||
end | ||
|
||
def self.symlink?(path) | ||
if LibC.lstat(path.check_no_null_byte, out stat) != 0 | ||
if Errno.value == Errno::ENOENT | ||
return false | ||
else | ||
raise Errno.new("stat") | ||
end | ||
end | ||
(stat.st_mode & LibC::S_IFMT) == LibC::S_IFLNK | ||
end | ||
|
||
def self.rename(old_filename, new_filename) | ||
code = LibC.rename(old_filename.check_no_null_byte, new_filename.check_no_null_byte) | ||
if code != 0 | ||
raise Errno.new("Error renaming file '#{old_filename}' to '#{new_filename}'") | ||
end | ||
end | ||
|
||
def self.utime(atime : ::Time, mtime : ::Time, filename : String) : Nil | ||
timevals = uninitialized LibC::Timeval[2] | ||
timevals[0] = to_timeval(atime) | ||
timevals[1] = to_timeval(mtime) | ||
ret = LibC.utimes(filename, timevals) | ||
if ret != 0 | ||
raise Errno.new("Error setting time to file '#{filename}'") | ||
end | ||
end | ||
|
||
private def self.to_timeval(time : ::Time) | ||
t = uninitialized LibC::Timeval | ||
t.tv_sec = typeof(t.tv_sec).new(time.to_local.epoch) | ||
t.tv_usec = typeof(t.tv_usec).new(0) | ||
t | ||
end | ||
|
||
private def system_truncate(size) : Nil | ||
flush | ||
code = LibC.ftruncate(fd, size) | ||
if code != 0 | ||
raise Errno.new("Error truncating file '#{path}'") | ||
end | ||
end | ||
|
||
private def system_flock_shared(blocking) | ||
flock LibC::FlockOp::SH, blocking | ||
end | ||
|
||
private def system_flock_exclusive(blocking) | ||
flock LibC::FlockOp::EX, blocking | ||
end | ||
|
||
private def system_flock_unlock | ||
flock LibC::FlockOp::UN | ||
end | ||
|
||
private def flock(op : LibC::FlockOp, blocking : Bool = true) | ||
op |= LibC::FlockOp::NB unless blocking | ||
|
||
if LibC.flock(@fd, op) != 0 | ||
raise Errno.new("flock") | ||
end | ||
|
||
nil | ||
end | ||
end |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,141 @@ | ||
require "c/fcntl" | ||
|
||
# :nodoc: | ||
module Crystal::System::FileDescriptor | ||
include IO::Syscall | ||
|
||
@fd : Int32 | ||
|
||
@read_event : Event::Event? | ||
@write_event : Event::Event? | ||
|
||
private def unbuffered_read(slice : Bytes) | ||
read_syscall_helper(slice, "Error reading file") do | ||
# `to_i32` is acceptable because `Slice#size` is a Int32 | ||
LibC.read(@fd, slice, slice.size).to_i32 | ||
end | ||
end | ||
|
||
private def unbuffered_write(slice : Bytes) | ||
write_syscall_helper(slice, "Error writing file") do |slice| | ||
LibC.write(@fd, slice, slice.size).tap do |return_code| | ||
if return_code == -1 && Errno.value == Errno::EBADF | ||
raise IO::Error.new "File not open for writing" | ||
end | ||
end | ||
end | ||
end | ||
|
||
private def system_blocking? | ||
fcntl(LibC::F_GETFL) & LibC::O_NONBLOCK == 0 | ||
end | ||
|
||
private def system_blocking=(value) | ||
current_flags = fcntl(LibC::F_GETFL) | ||
new_flags = current_flags | ||
if value | ||
new_flags &= ~LibC::O_NONBLOCK | ||
else | ||
new_flags |= LibC::O_NONBLOCK | ||
end | ||
fcntl(LibC::F_SETFL, new_flags) unless new_flags == current_flags | ||
end | ||
|
||
private def system_close_on_exec? | ||
flags = fcntl(LibC::F_GETFD) | ||
(flags & LibC::FD_CLOEXEC) == LibC::FD_CLOEXEC | ||
end | ||
|
||
private def system_close_on_exec=(arg : Bool) | ||
fcntl(LibC::F_SETFD, arg ? LibC::FD_CLOEXEC : 0) | ||
arg | ||
end | ||
|
||
def self.fcntl(fd, cmd, arg = 0) | ||
r = LibC.fcntl(fd, cmd, arg) | ||
raise Errno.new("fcntl() failed") if r == -1 | ||
r | ||
end | ||
|
||
private def system_stat | ||
if LibC.fstat(@fd, out stat) != 0 | ||
raise Errno.new("Unable to get stat") | ||
end | ||
::File::Stat.new(stat) | ||
end | ||
|
||
private def system_seek(offset, whence : IO::Seek) : Nil | ||
seek_value = LibC.lseek(@fd, offset, whence) | ||
|
||
if seek_value == -1 | ||
raise Errno.new "Unable to seek" | ||
end | ||
end | ||
|
||
private def system_pos | ||
pos = LibC.lseek(@fd, 0, IO::Seek::Current) | ||
raise Errno.new "Unable to tell" if pos == -1 | ||
pos | ||
end | ||
|
||
private def system_tty? | ||
LibC.isatty(fd) == 1 | ||
end | ||
|
||
private def system_reopen(other : IO::FileDescriptor) | ||
{% if LibC.methods.includes? "dup3".id %} | ||
# dup doesn't copy the CLOEXEC flag, so copy it manually using dup3 | ||
flags = other.close_on_exec? ? LibC::O_CLOEXEC : 0 | ||
if LibC.dup3(other.fd, self.fd, flags) == -1 | ||
raise Errno.new("Could not reopen file descriptor") | ||
end | ||
{% else %} | ||
# dup doesn't copy the CLOEXEC flag, copy it manually to the new | ||
if LibC.dup2(other.fd, self.fd) == -1 | ||
raise Errno.new("Could not reopen file descriptor") | ||
end | ||
|
||
if other.close_on_exec? | ||
self.close_on_exec = true | ||
end | ||
{% end %} | ||
|
||
# We are now pointing to a new file descriptor, we need to re-register | ||
# events with libevent and enqueue readers and writers again. | ||
@read_event.try &.free | ||
@read_event = nil | ||
|
||
@write_event.try &.free | ||
@write_event = nil | ||
|
||
reschedule_waiting | ||
end | ||
|
||
private def add_read_event(timeout = @read_timeout) : Nil | ||
event = @read_event ||= Scheduler.create_fd_read_event(self) | ||
event.add timeout | ||
end | ||
|
||
private def add_write_event(timeout = @write_timeout) : Nil | ||
event = @write_event ||= Scheduler.create_fd_write_event(self) | ||
event.add timeout | ||
end | ||
|
||
private def system_close | ||
if LibC.close(@fd) != 0 | ||
case Errno.value | ||
when Errno::EINTR, Errno::EINPROGRESS | ||
# ignore | ||
else | ||
raise Errno.new("Error closing file") | ||
end | ||
end | ||
ensure | ||
@read_event.try &.free | ||
@read_event = nil | ||
@write_event.try &.free | ||
@write_event = nil | ||
|
||
reschedule_waiting | ||
end | ||
end |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer
system_tell
oversystem_pos
.I wish we'd drop
IO#pos
andIO#pos=
and keptIO#seek
andIO#tell
instead (less duplication, more explicit naming).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just need 1 call:
seek
which always returns the current position. I suggested thatIO
implementpos
pos=
andtell
in the base class to @asterite and simply provide a raisingIO#tell
by default.For now, this is how
IO
works. It's not great, but it is how it is until I refactor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally i'd rather keep
pos
,pos=
andseek
.tell
is obscure and weird naming to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's "seek and tell". They're well known associated terms, whatever the language. E.g. C, Haskell, Python, Ruby or Rust, all have both.
I don't like the
pos
naming, it's an abbreviation and inexplicit (inherited from Ruby). At leastposition
would be a more explicit name but I still prefertell
(broadly used). Using a setter (pos=
) to perform an action, that is move the cursor in a file, sounds awful to me, in addition to be an exact duplicate forseek
, too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say "seek" is not an unknown term to me, although "tell" is. I agree with you on
pos
vsposition
, but not that setters shouldn't have performance side-effects.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, the terms tell and seek come from working with files.
IO
is a general class, and to me it doesm't seem logical to transfer this meaning from files to any other stream. But I may be mistaken.Another thought:
pos
andpos=
is a concept easy to understand. One reads the current stream position, the other sets it (position
andposition=
are a bit longer but also fine). Withseek
andtell
it is more complicated, if your not familiar with these terms. You have to remember two names instead of one and can't simply asseses their meaning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also for dropping
#seek/#tell
and using#position/#position=
(Not#pos
). The latter is really clear in intention. Also, it lets you seek around in a file using normal code:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
seek
should stay,io.position = io.size - 5
involves 2 syscalls whereasio.seek(-5, IO::Seek::End)
requires only one. Similar forio.position += 5
. That being said, I think that usingio.position
is a much nicer interface which is more than adequate for most people. Perhaps some caching for size and position would mean that we can obviateseek
entirely.Regardless of that, we should make a new issue containing all the IO refactor suggestions from this thread. Unless anyone else volunteers to make that issue now, i'll get it done once this PR is merged.