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

Remove key args and both #1442

Closed
wants to merge 15 commits into from
Closed

Conversation

samoht
Copy link
Member

@samoht samoht commented Jun 23, 2023

To test with: mirage/mirage-skeleton#364

Based on top of #1437 - experiment with removing "both" keys (just keep runtime and configure-time keys). Also remove the ~key parameter when registering unikernel to force people to move these into the unikenerls.

Should address most of what was requested in #1422 - but it's a bit more radical than #1437 so opening a separate PR to discuss.

@samoht samoht force-pushed the remove-key-args-and-both branch 2 times, most recently from 818dc84 to 97646b8 Compare June 26, 2023 10:07
@hannesm
Copy link
Member

hannesm commented Jun 26, 2023

To me, this looks like the right direction on how to make unikernel development easier to understand -- the keys are now defined in unikernel.ml, and can be accessed there directly. Should the return value from register (of a key) be a lazy 'a instead of unit -> 'a?

This also allows in unikernels that use other types than the base ones to specify converters for them (i.e. Ipaddr / Domain_name / X509.Key_type.t / ...). That's just great, I've been looking at that for a decade ;)

IIUC, Key_gen is still used, mainly for these keys for the network stack etc. (which are barely used in the unikernel, but the network stack uses it internally to configure itself).

Did you try to port a more complex unikernel, such as mirage-www or unipi, to this branch? That'd likely be a good test on how this pans out.

@samoht
Copy link
Member Author

samoht commented Jun 30, 2023

This has been merged as #1451, #1450, #1449, #1448, #1447 and #1437 (the main one).

@samoht samoht closed this Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants