Make the utility maybe_open_stdio cross platform #240
Conversation
|
Needs a rebase. |
Signed-off-by: James Sturtevant <[email protected]>
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
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.
| 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.
There was a problem hiding this comment.
It is, will clean it up in #238
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 |
| @@ -13,6 +8,11 @@ use containerd_shim_wasm::sandbox::instance_utils::maybe_open_stdio; | |||
| 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.
The removes an unused function in wasmtime and also updates
maybe_open_stdioto cross-platform helper (part of #49 and #238)