search

mirage / mirage-qubes

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

RExec: Close connection with peer on unknown msgs #61

Closed reynir closed 3 years ago

reynir commented 3 years ago

Do not raise an exception. Instead, `Eof is returned. This does not change the interface, but I'm thinking it's perhaps better returning `Error - or maybe it's not interesting to know which end closed the connection?

Fixes #45

hannesm commented 3 years ago

This greatly improves the semantics of QRExec: instead of raising an exception, now an error is logged, and the connection is closed. Returning an error could be done at some point, the benefit would be that API clients would be able to handle such an Error different from Eof -- I'm not sure whether this is worth it, and clearly this can be done when needed.

LGTM - imho once this is merged a new release would make sense (what about #60 - is that PR ready & tested)? //what do you think @talex5?

reynir commented 3 years ago

Yes, I agree we should wait until there's a need.

In RExec.listen we just log (as INFO) unexpected messages and ignore them. I'd like to log it at WARN level at least. In that function we're talking with dom0 which is presumably more trusted to be well-behaved. I am wondering if we should instead close the connection or raise an error. I'm not sure whether dom0 will reopen the connection to domU if the agent closes the connection.

About #60, I haven't tested it yet. Basically, it allows for up to 16× larger data chunks in qrexec at the cost of more complexity. I think it's not a deal breaker if it doesn't make it in next release, though the performance gains would be nice.

hannesm commented 3 years ago

Thanks @reynir. I went ahead, merged this PR and released 0.9.1 (see https://github.com/ocaml/opam-repository/pull/17748) without #60 -- this should pave the road for Qubes 4.1 users, and once #60 is ready and tested we can merge and cut a new release.