Skip to content

Remove host_print_writer from the arguments to UninitializedSandbox::new #487

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

Merged
merged 1 commit into from
May 14, 2025

Conversation

jprendes
Copy link
Contributor

@jprendes jprendes commented May 13, 2025

With #480 merged, registering a host function with a name that is already in use, just replaces the previous registration.
This means that we can pre-emptively register the default HostPrint method, and replace it afterwards.

This PR removes host_print_writer from the arguments to UninitializedSandbox::new.
The default host print is pre-emptively registered, and a new HostPrint can be registered like any other host function.
Alternatively, the method register_print can be used, which uses the correct name and (compile-time-)checks the function signature.

This partly addresses the following comment (i.e., it still doesn't implement a builder pattern, but does allow adding extra syscalls to the writer method)

// TODO: These only here to accommodate some writer functions.
// We should modify the `UninitializedSandbox` to follow the builder pattern we use in
// hyperlight-wasm to allow the user to specify what syscalls they need specifically.

@jprendes jprendes added kind/refactor For PRs that restructure or remove code without adding new functionality. area/API Related to the API or public interface labels May 13, 2025
@jprendes jprendes changed the title Remove the host writer from the arguments to UninitializedSandbox::new Remove host_print_writer from the arguments to UninitializedSandbox::new May 13, 2025
@jprendes jprendes marked this pull request as ready for review May 13, 2025 16:27
@jprendes
Copy link
Contributor Author

@danbugs @ludfjig PTAL :-)

Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Could you update the example in the root README as well?

danbugs
danbugs previously approved these changes May 14, 2025
Copy link
Contributor

@danbugs danbugs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Other than the README, you might want to check doctests! IIrc, we have 1 that might create an uninit sandbox.

Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
@jprendes
Copy link
Contributor Author

@danbugs @ludfjig PTAL :-)
Basically rebased and fixed doctest.

@jprendes jprendes merged commit 2b208b2 into hyperlight-dev:main May 14, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API Related to the API or public interface kind/refactor For PRs that restructure or remove code without adding new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants