search

mirage / mirage-qubes

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

RExec.connect: handle protocol version negotiation #57

Closed reynir closed 3 years ago

reynir commented 3 years ago

Continuation of #56.

Adds debug message the other place where protocol version is negotiated. In the case of version > 2 it would work without this patch. It fails if the other end wants to use version 1 for good measure.

hannesm commented 3 years ago

The "fail if other ends wants version 1" looks fine to me; but I'm not a big fan of the conditionally print a debug message -- and would either use a warning there, or keep the old code (an info message printing the remote version).

talex5 commented 3 years ago

It's not really a warning - there's nothing wrong with the other end offering a newer version.

How about an info message that always shows the version offered and the version chosen?

reynir commented 3 years ago

The old code was wrong in its log message, though, in the case of the other end wanting to use protocol version 3. Then it would log us using protocol version 3 even though we send protocol version 2 thereby forcing the protocol version to 2.

reynir commented 3 years ago

The new commit unconditionally logs the protocol version negotiation in both places.

hannesm commented 3 years ago

that looks good - though I'd condense the two Log in connect into a single on the Info level -- and also use the Info level in with_flow

reynir commented 3 years ago

Yes, I'll fix it up. The log message still says "newer" even though that's not necessarily true.