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

Support for using multiple protocols #175

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

balajiv113
Copy link
Contributor

This support enables usage of multiple different protocols at the same time.

fixes #86

Compatibility
The code should be backward compatible. Just that now Accept methods like AcceptQemu, AcceptBess takes care of setting the protocol themself.

Even if the protocol mentioned in configuration is wrong, these Accept methods takes priority and sets proper one.
Note: The configuration still has Protocol and this is used for /connect mux.

Usecase
This will enable us to create a user-space network with multiple different hypervisors (eg: QEMU & VZ in macOS)

Signed-off-by: Balaji Vijayakumar <kuttibalaji.v6@gmail.com>
@gbraad
Copy link
Member

gbraad commented Feb 15, 2023

Does the API for use change? From the code I see that it internally handles this... right?

@balajiv113
Copy link
Contributor Author

Yes exactly no external API change its just the internal handling to support multiple protocol.

The behaviour of protocol used in configuration alone will change now (now its used only in /connect mux)

@gbraad
Copy link
Member

gbraad commented Feb 15, 2023

@cfergeau WDYT?

size := sProtocol.Buf()
sProtocol.Write(size, pkt.Size())
return e.txBuf(pkt, size)
return e.txPkt(pkt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, this would have been slightly more straightforward to review if there was a preliminary commit doing these tx() changes while keeping e.protocol (a Move stream socket handling in a txBuf() helper)
txPkt also does not seem very useful in a separate method as tx is a one liner, and the only user of this method.
These are just minor commets, this looks good, and worked fine with CRC, so ACK from me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure will keep this in mind for the next one 👍

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 15, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: balajiv113, cfergeau

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cfergeau cfergeau merged commit 898842f into containers:main Feb 15, 2023
@balajiv113 balajiv113 deleted the multiple-conn branch February 15, 2023 18:04
@cfergeau
Copy link
Contributor

Note: The configuration still has Protocol and this is used for /connect mux.

Just had more thoughts about this after digging into the code some more. I think only gvforwarder is making use of the /connect mux, and gvforwarder is using the HyperKit protocol. I suspect

_ = n.networkSwitch.Accept(context.Background(), conn, n.configuration.Protocol)
could be changed to hardcode types.HyperKitProtocol instead, and the Protocol field in types.Configuration would then become unused.

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.

Refactor code to handle multiple protocols at the same time
3 participants