Make the utility maybe_open_stdio cross platform#240
Conversation
|
Needs a rebase. |
Signed-off-by: James Sturtevant <jstur@microsoft.com>
jsturtevant
force-pushed
the
use-cross-platfrom-file
branch
from
c8649cb to
e9a95b8
Compare
|
/hold going to try #226 (comment) |
jsturtevant
force-pushed
the
use-cross-platfrom-file
branch
from
1e7ffe7 to
b1b2a7f
Compare
jprendes
left a comment
jprendes
left a comment
There was a problem hiding this comment.
I'm not a big fan of try_clone-ing the file, since that opens yet another fd for the same file.
But it works, and would unblock you to continue working on windows support.
I am happy for this to be merged as is.
@cpuguy83: I agree with you in #226 (comment) and think that would also improve the file handling here.
| fs::{self, File, OpenOptions}, | ||
| io::ErrorKind, | ||
| os::fd::{IntoRawFd, RawFd}, | ||
| os::fd::AsRawFd, |
There was a problem hiding this comment.
question: Does this build on Windows? I thought AsRawFd was only available on unix.
I wasn't too happy with this either but wasn't sure how else to handle the ownership here (open to suggestions). Initially, reading the docs I didn't get that it will create a second FD since it says it would use "same underlying file handle" but I see it uses fcntl which will give a new FD. Will open an issue for #226 (comment) to follow up. |
|
Feel free to drop b1b2a7f from the PR as that would be part of the new issue :-) |
jsturtevant
force-pushed
the
use-cross-platfrom-file
branch
from
b1b2a7f to
e9a95b8
Compare
done, put it over here for future reference https://github.com/containerd/runwasi/compare/main...jsturtevant:runwasi:use-cross-platfrom-file-backup?expand=1 |
| use containerd_shim_wasm::sandbox::{EngineGetter, InstanceConfig}; | ||
| use nix::unistd::close; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::fs::File; |
There was a problem hiding this comment.
Why are these imports moved to here? Did you use a formatter to do it?
There was a problem hiding this comment.
It think so, I didn't notice but I did run make fix which runs fmt and clippy and looks like it re-ordered them alphabetically
There was a problem hiding this comment.
umm rustfmt and clippy won't re-order imports alphabetically IIRC.
5 participants
The removes an unused function in wasmtime and also updates
maybe_open_stdioto cross-platform helper (part of #49 and #238)