search

mirage / mirage-qubes

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

Rexec client and server #39

Closed reynir closed 4 years ago

reynir commented 4 years ago

This PR adds functionality to the qrexec implementation such that receiving and sending qrexec rpc calls concurrently is possible.

I will need to review this myself again, but it seems to work provided https://github.com/mirage/ocaml-vchan/pull/132 is merged.

reynir commented 4 years ago

Added @yomimono as co-author since a lot of the code was based on theirs (thanks!!)

yomimono commented 4 years ago

please add @linse also :)

reynir commented 4 years ago

Done! :-)

reynir commented 4 years ago

I had a discussion with @cfcs regarding RExec.Client_flow.writef. It differs from RExec.Flow.writef in that it doesn't add a newline. We discussed renaming it RExec.Client_flow.writefmt and adding a RExec.Flow.writefmt with equivalent semantics.

cfcs commented 4 years ago

leaving a note here about concatenating printf format6 values to avoid first evaluating, then string concatenating (for the \n thing)

# open CamlinternalFormatBasics ;;
# let f (Format (fmt, n)) =
  let joined = Format (concat_fmt fmt (Char_literal ('z', End_of_format)), "") in
  Printf.sprintf joined
in f "a %s " "b";;
- : string = "a b z"

So @hannesm suggested this instead:

let f fmt = Printf.sprintf (fmt ^^ "z") in f "a %s " "b";;
- : string = "a b z"
reynir commented 4 years ago

The CI fails at OCaml 4.04.0. I use Hashtbl.find_opt which is from OCaml 4.05.0. Mirage requires OCaml >= 4.05.0, so I think the CI should be updated to reflect this. :-)

hannesm commented 4 years ago

from what I understand (please correct me if I'm wrong)

thanks for reading, maybe we can converge to get this merged (and a mirage-qubes release)!?

reynir commented 4 years ago

@hannesm sorry, I have been travelling with little opportunity for computer time. Yes, that is correct. I have adapted some of the suggestions made by @cfcs, and https://github.com/mirage/mirage-qubes/pull/44 ups the OCaml compiler version. What is still outstanding is:

I don't find the latter an issue personally, but I'm open for adding begin .. end. I am personally not interested in adding read_line, so I think it would be better as a separate PR. Regarding killing the flow I've made a PR to my own PR with an implementation that I'm considering https://github.com/reynir/mirage-qubes/pull/1.

I would love to see this merged and released soon.

cfcs commented 4 years ago

@reynir I'm happy with your kill flow implementation. I haven't tested it, but I think it looks reasonable, and I think we should get this merged. My comments aside, all of which I consider addressed, are we ready to merge?

hannesm commented 4 years ago

a rebase onto master would be great (than CI should pass AFAICT)!

cfcs commented 4 years ago

Looks like CI is happy :)

hannesm commented 4 years ago

for me, the remaining question is about kill flow implementation -- reynir/mirage-qubes#1 (on unexpected message?, and EOF?) -- which is not part of this PR!?

reynir commented 4 years ago

I was considering doing kill flow in a separate PR. I noticed the existing server implementation did not close the flow either, but rereading the code now I noticed that it raises an exception (by Lwt.fail) on unexpected message types. It seems to me that a misbehaving client can then take down the whole unikernel?!

hannesm commented 4 years ago

I agree with @reynir that the kill behaviour can be improved in a future PR (there are other components, such as qubesdb, which lead to Lwt.fail if they receive unexpected messages). Merging after review and ok from @cfcs to include into the next release. Thanks for your patience and submitting this PR!