Skip to content
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

unix: add support for libc::readv and libc::writev #4048

Open
mstoeckl opened this issue Nov 22, 2024 · 8 comments
Open

unix: add support for libc::readv and libc::writev #4048

mstoeckl opened this issue Nov 22, 2024 · 8 comments
Labels
A-shims Area: This affects the external function shims A-unix Area: affects our shared Unix target support C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available

Comments

@mstoeckl
Copy link

mstoeckl commented Nov 22, 2024

readv and writev are vectored versions of read/write which are included in POSIX. (See e.g. man 3p readv, writev .) They could be (and sometimes have been) implemented using a loop and many read or write calls, so their semantics are not unusual.

I was advised to make this issue by the following error message:

test test ... error: unsupported operation: can't call foreign function `readv` on OS `linux`
   --> src/test.rs:125:19
    |
125 |         let ret = libc::readv(src_fd.as_raw_fd(), iovs.as_ptr(), iovs.len() as _);
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't call foreign function `readv` on OS `linux`
    |
    = help: if this is a basic API commonly used on this target, please report an issue with Miri
    = help: however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases

A workaround for this error: under cfg(miri), replace libc::readv/libc::writev with functions that implement them using libc::read/libc::write directly.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement A-shims Area: This affects the external function shims A-unix Area: affects our shared Unix target support labels Nov 22, 2024
@shamb0
Copy link
Contributor

shamb0 commented Dec 3, 2024

Hi @RalfJung, I am interested to work on this issue, Could you please assign this task to me.

@RalfJung
Copy link
Member

RalfJung commented Dec 3, 2024

@shamb0 you can do that yourself. :) Just write @rustbot claim.

@shamb0
Copy link
Contributor

shamb0 commented Dec 3, 2024

@shamb0 you can do that yourself. :) Just write @rustbot claim.

Thanks @RalfJung

@shamb0
Copy link
Contributor

shamb0 commented Dec 3, 2024

@rustbot claim

@RalfJung
Copy link
Member

They could be (and sometimes have been) implemented using a loop and many read or write calls,

That does not seem right. The docs say that the entire readv call is atomic in terms of the file contents it observes. So implementing them as a loop will not behave correctly.

@mstoeckl
Copy link
Author

That does not seem right.

Good point, I was wrong there. Double checking man 2 readv, I see that a temporary buffer was used instead:

In the case of readv(), the wrapper function allocated temporary buffer large enough for all of the items specified by iov, that buffer in a call to read(2), copied data from the buffer to the specified by the iov_base fields of the elements of iov, and then the buffer. The wrapper function for writev() performed the analogous using a temporary buffer and a call to write(2).

@RalfJung
Copy link
Member

RalfJung commented Jan 20, 2025

@rustbot release-assignment
(based on #4110 (comment))

@RalfJung
Copy link
Member

What we learned from the first attempt here is that some more preparation is needed: read (and write, but please just do read in the first PR so iteration is easier) in trait FileDescription need to be adjusted to support customizing what happens after the operation is complete. Specifically, the dest: &MPlaceTy<'tcx> argument should be replaced by a finish: DynMachineCallback<Result<u64, IoError>>. The existing read/write shims should then implement finish as "write to dest, maybe update errno".

This will require changes to all existing FileDescription instances, but should otherwise not require any new logic.

@RalfJung RalfJung added the E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available label Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shims Area: This affects the external function shims A-unix Area: affects our shared Unix target support C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants