mirage / mirage-qubes

Mirage support for writing QubesOS AppVM unikernels
BSD 2-Clause "Simplified" License
62 stars 11 forks source link

Add common code for Qubes unikernels #71

Closed palainp closed 1 month ago

palainp commented 1 month ago

This PR is a starting point for #70 . I was unsure how to add functions that can be called directly like Mirage_qubes.shutdown so I stuck with Mirage_qubes.Misc.shutdown.

I also left Mirage_qubes.Misc.check_memory () as it works fine with qubes-mirage-firewall (and some quick tests with qubes-miragevpn). Perhaps it would be better to add the free memory ratio as parameters if the different unikernels need a different tuning ratio? I dislike hard coded values :(, but as @hannesm said, this should be less critical with the cstruct deleting process underway :)

If merged, it should be easy to update the various Qubes unikernels :)

dinosaure commented 1 month ago

LGTM from a quick look :+1:.

hannesm commented 1 month ago

maybe using an optional argument is a good way to allow specialization by the client while making it not cumbersome. what do you think?

palainp commented 1 month ago

Thank you for your reviews. I've added the optional argument (using it twice, I used to have two different values with qubes-mirage-firewall, but I always found it unintuitive, and hard to understand).

hannesm commented 1 month ago

the only thing I'm wondering is about floating point numbers... since I dislike them, and would prefer integer numbers -- but I'm not sure whether there's a good/easy path forward in respect to percentiles...

palainp commented 1 month ago

the only thing I'm wondering is about floating point numbers... since I dislike them, and would prefer integer numbers -- but I'm not sure whether there's a good/easy path forward in respect to percentiles...

Maybe we don't want to go, precision-wise, beyond to a percent of total memory? I'm perfectly fine with integers :)

hannesm commented 1 month ago

back, I've a minor suggestion above. otherwise good to be merged.

hannesm commented 1 month ago

thanks, squashed and merged :)