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

RExec.connect: handle protocol version negotiation #57

Merged
merged 2 commits into from Nov 25, 2020

Conversation

reynir
Copy link
Member

@reynir reynir commented Nov 25, 2020

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
Copy link
Member

hannesm commented Nov 25, 2020

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
Copy link
Collaborator

talex5 commented Nov 25, 2020

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
Copy link
Member Author

reynir commented Nov 25, 2020

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
Copy link
Member Author

reynir commented Nov 25, 2020

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

@hannesm
Copy link
Member

hannesm commented Nov 25, 2020

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
Copy link
Member Author

reynir commented Nov 25, 2020

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

lib/rExec.ml Outdated Show resolved Hide resolved
@talex5 talex5 merged commit c992ebc into mirage:master Nov 25, 2020
hannesm added a commit to hannesm/opam-repository that referenced this pull request Dec 2, 2020
CHANGES:

- RExec: being future compatible, and negotiate protocol version 2 (mirage/mirage-qubes#56 mirage/mirage-qubes#57 @reynir)
- Remove Utils module (mirage/mirage-qubes#58 @hannesm)
- RExec: close connection with peer on unknown messages, instead of raising an exception (mirage/mirage-qubes#61 @reynir)
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

3 participants