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

feat(common): Improve native IO API #553

Open
clabby opened this issue Sep 23, 2024 · 3 comments
Open

feat(common): Improve native IO API #553

clabby opened this issue Sep 23, 2024 · 3 comments
Assignees
Labels
A-common Area: kona-common crate K-feature Kind: feature

Comments

@clabby
Copy link
Collaborator

clabby commented Sep 23, 2024

Overview

Right now, the IO API for the native platform in kona-common is inherently unsafe. Though, because it is behind the BasicKernelInterface trait, the API is not actually marked as unsafe lexically.

The reason the API is currently unsafe is because of our use of raw file descriptors. The FileDescriptor enum for the FPVM target is constant, and it is also Clone + Copy. However, on the native platform, file descriptors cannot be safely cloned or copied without checking for potential issues with creating a second handle to the fd.

This has resulted in a hacky workaround, where we hold onto the raw file descriptor, expect the host process to keep them alive for long enough, and construct + forget the file for read and write ops. While this works, the API is very unclear, and this invisible requirement of having the host keep the file descriptors alive is a bit annoying.

Instead, we should treat the FileDescriptor enum as an owned type. Cloning or copying it should be a falliable operation, i.e. OwnedFd::try_clone.

This will help improve the API to expose a truly safe native IO API, and prevent us from having to leak the Files in NativeIO.

@malik672
Copy link

@clabby this should work if a try_clone() is implemented right ?

@clabby
Copy link
Collaborator Author

clabby commented Nov 13, 2024

@clabby this should work if a try_clone() is implemented right ?

We could definitely go that route, however it will be a fairly large API change. Right now, we assume the FileDescriptor enum is Copy, and the API is built up around that.

Ideally, we can move towards a TryCloneable type, though, and pass references through the preimage oracle / low-level kona-common IO API.

@malik672
Copy link

@clabby this should work if a try_clone() is implemented right ?

We could definitely go that route, however it will be a fairly large API change. Right now, we assume the FileDescriptor enum is Copy, and the API is built up around that.

Ideally, we can move towards a TryCloneable type, though, and pass references through the preimage oracle / low-level kona-common IO API.

Both approaches can work, and we can simply call the try_clone()inside the I/O functions when we want to duplicate, since we will only be duplicating when needed, something like: fd.try_clone()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area: kona-common crate K-feature Kind: feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants