-
Notifications
You must be signed in to change notification settings - Fork 73
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
eio_posix: fix pread at end-of-file #581
Conversation
Reported by Simon Grondin.
@@ -71,7 +71,11 @@ let of_fd fd = object (_ : <Eio_unix.Net.stream_socket; Eio.File.rw>) | |||
method read_methods = [] | |||
method copy src = copy src fd | |||
|
|||
method pread ~file_offset bufs = Low_level.preadv ~file_offset fd (Array.of_list bufs) | |||
method pread ~file_offset bufs = | |||
let got = Low_level.preadv ~file_offset fd (Array.of_list bufs) in |
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.
The array/list mismatch in the low-level/high-level interface leads to more allocations, but I can't see how to get rid of that easily given what the underlying syscall expects.
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 know I wouldn't mind the high-level interface exposing array. That way I can choose whether to convert my list to an array or adapt my algorithm to use array.
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.
LGTM
CHANGES: New features / API changes: - Extend `Eio.Condition` API (@talex5 ocaml-multicore/eio#563). - `loop_no_mutex` is a simpler and more efficient way to way for a condition. - `register_immediate` allows integration with other IO libraries. - Expose `Eio.Stdenv.backend_id` (@bord-o ocaml-multicore/eio#560, reviewed by @talex5). Useful in tests to report which backend is being used. - Remove deprecated features (@talex5 ocaml-multicore/eio#552, reviewed by @avsm). These were all already marked as deprecated in v0.10 and are now gone completely: - `Fiber.fork_sub` - `Eio_unix.{FD,Ipaddr,socketpair,getnameinfo}` - `Eio_linux.{FD,get_fd,get_fd_opt}` - `Eio_posix.Low_level.Fd` - Allow calling `close` more than once (@talex5 ocaml-multicore/eio#547, requested by @anmonteiro, reviewed by @patricoferris, @avsm). - Add `close` to socket type (@talex5 ocaml-multicore/eio#549). Simplifies the type signatures a bit by avoiding having to mention this everywhere. Bug fixes: - Fix handling of empty path strings (@talex5 ocaml-multicore/eio#569, reported by @SGrondin). Using "" instead of "." in some places resulted in an error. - eio_posix: fix update to watched FDs on cancel (@talex5 ocaml-multicore/eio#574, reported and reviewed by @quernd). Cancelling the last watcher of an FD didn't remove it from the set passed to `poll`, which could result in constant wake-ups. - eio_posix: fix `pread` at end-of-file (@talex5 ocaml-multicore/eio#581, reported by @SGrondin). It tried to return 0 instead of `End_of_file`, triggering an assertion. - eio_posix: don't reap non-Eio child processes (@talex5 ocaml-multicore/eio#562). This allows spawning processes with e.g. the stdlib or Lwt (but see ocaml-multicore/lwt_eio#19 for Lwt support). - Preserve backtraces across `Domain_manager.run` (@talex5 ocaml-multicore/eio#571). See ocaml/ocaml#12362. - Correct the backend selection for Cygwin (@dra27 ocaml-multicore/eio#557). Use `eio_posix`, not `eio_windows` in this case. Other changes: - Simplify dune files with dune 3.9's `build_if` (@talex5 ocaml-multicore/eio#582). - Remove `Waiters` from `Eio_core` (@talex5 ocaml-multicore/eio#567). `Eio.Switch` no longer uses this so it can finally be removed. - Use `Fmt.Dump.signal` to format signals (@talex5, @MisterDA ocaml-multicore/eio#543). Documentation: - Add some notes about thread-safety in the documentation (@talex5 ocaml-multicore/eio#568).
CHANGES: New features / API changes: - Extend `Eio.Condition` API (@talex5 ocaml-multicore/eio#563). - `loop_no_mutex` is a simpler and more efficient way to way for a condition. - `register_immediate` allows integration with other IO libraries. - Expose `Eio.Stdenv.backend_id` (@bord-o ocaml-multicore/eio#560, reviewed by @talex5). Useful in tests to report which backend is being used. - Remove deprecated features (@talex5 ocaml-multicore/eio#552, reviewed by @avsm). These were all already marked as deprecated in v0.10 and are now gone completely: - `Fiber.fork_sub` - `Eio_unix.{FD,Ipaddr,socketpair,getnameinfo}` - `Eio_linux.{FD,get_fd,get_fd_opt}` - `Eio_posix.Low_level.Fd` - Allow calling `close` more than once (@talex5 ocaml-multicore/eio#547, requested by @anmonteiro, reviewed by @patricoferris, @avsm). - Add `close` to socket type (@talex5 ocaml-multicore/eio#549). Simplifies the type signatures a bit by avoiding having to mention this everywhere. Bug fixes: - Fix handling of empty path strings (@talex5 ocaml-multicore/eio#569, reported by @SGrondin). Using "" instead of "." in some places resulted in an error. - eio_posix: fix update to watched FDs on cancel (@talex5 ocaml-multicore/eio#574, reported and reviewed by @quernd). Cancelling the last watcher of an FD didn't remove it from the set passed to `poll`, which could result in constant wake-ups. - eio_posix: fix `pread` at end-of-file (@talex5 ocaml-multicore/eio#581, reported by @SGrondin). It tried to return 0 instead of `End_of_file`, triggering an assertion. - eio_posix: don't reap non-Eio child processes (@talex5 ocaml-multicore/eio#562). This allows spawning processes with e.g. the stdlib or Lwt (but see ocaml-multicore/lwt_eio#19 for Lwt support). - Preserve backtraces across `Domain_manager.run` (@talex5 ocaml-multicore/eio#571). See ocaml/ocaml#12362. - Correct the backend selection for Cygwin (@dra27 ocaml-multicore/eio#557). Use `eio_posix`, not `eio_windows` in this case. Other changes: - Simplify dune files with dune 3.9's `build_if` (@talex5 ocaml-multicore/eio#582). - Remove `Waiters` from `Eio_core` (@talex5 ocaml-multicore/eio#567). `Eio.Switch` no longer uses this so it can finally be removed. - Use `Fmt.Dump.signal` to format signals (@talex5, @MisterDA ocaml-multicore/eio#543). Documentation: - Add some notes about thread-safety in the documentation (@talex5 ocaml-multicore/eio#568).
CHANGES: New features / API changes: - Extend `Eio.Condition` API (@talex5 ocaml-multicore/eio#563). - `loop_no_mutex` is a simpler and more efficient way to way for a condition. - `register_immediate` allows integration with other IO libraries. - Expose `Eio.Stdenv.backend_id` (@bord-o ocaml-multicore/eio#560, reviewed by @talex5). Useful in tests to report which backend is being used. - Remove deprecated features (@talex5 ocaml-multicore/eio#552, reviewed by @avsm). These were all already marked as deprecated in v0.10 and are now gone completely: - `Fiber.fork_sub` - `Eio_unix.{FD,Ipaddr,socketpair,getnameinfo}` - `Eio_linux.{FD,get_fd,get_fd_opt}` - `Eio_posix.Low_level.Fd` - Allow calling `close` more than once (@talex5 ocaml-multicore/eio#547, requested by @anmonteiro, reviewed by @patricoferris, @avsm). - Add `close` to socket type (@talex5 ocaml-multicore/eio#549). Simplifies the type signatures a bit by avoiding having to mention this everywhere. Bug fixes: - Fix handling of empty path strings (@talex5 ocaml-multicore/eio#569, reported by @SGrondin). Using "" instead of "." in some places resulted in an error. - eio_posix: fix update to watched FDs on cancel (@talex5 ocaml-multicore/eio#574, reported and reviewed by @quernd). Cancelling the last watcher of an FD didn't remove it from the set passed to `poll`, which could result in constant wake-ups. - eio_posix: fix `pread` at end-of-file (@talex5 ocaml-multicore/eio#581, reported by @SGrondin). It tried to return 0 instead of `End_of_file`, triggering an assertion. - eio_posix: don't reap non-Eio child processes (@talex5 ocaml-multicore/eio#562). This allows spawning processes with e.g. the stdlib or Lwt (but see ocaml-multicore/lwt_eio#19 for Lwt support). - Preserve backtraces across `Domain_manager.run` (@talex5 ocaml-multicore/eio#571). See ocaml/ocaml#12362. - Correct the backend selection for Cygwin (@dra27 ocaml-multicore/eio#557). Use `eio_posix`, not `eio_windows` in this case. Other changes: - Simplify dune files with dune 3.9's `build_if` (@talex5 ocaml-multicore/eio#582). - Remove `Waiters` from `Eio_core` (@talex5 ocaml-multicore/eio#567). `Eio.Switch` no longer uses this so it can finally be removed. - Use `Fmt.Dump.signal` to format signals (@talex5, @MisterDA ocaml-multicore/eio#543). Documentation: - Add some notes about thread-safety in the documentation (@talex5 ocaml-multicore/eio#568).
Reported by @SGrondin.
Fixes #579.