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

network: move socket type from address to socket #11486

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

florincoras
Copy link
Member

Signed-off-by: Florin Coras [email protected]

Risk Level: Low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a

Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

I know this change follows the bsd socket API, but I think the code is clearer having the type attached to the address. I think it's clearer that 127.0.0.1:80 cannot point to both TCP and UDP; a different address is needed for the two.

@florincoras
Copy link
Member Author

Ultimately this is a cosmetic change, so I guess it's not worth wasting too much time on it. However, I'm not sure I follow the above example. We are already storing the socket type in the socket, it's just that we're using a type defined in address.

Also, if one wants to open two tcp connections to 127.0.0.1:80, two addresses are needed but the above distinction does not hold anymore.

Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't look closely enough before I commented.

Yes, given that this is just moving the type definition, I agree it makes sense.

@ggreenway ggreenway merged commit 6679d57 into envoyproxy:master Jun 8, 2020
@florincoras
Copy link
Member Author

No worries! Thanks, @ggreenway !

@florincoras florincoras deleted the sock_type branch June 11, 2020 17:35
songhu pushed a commit to songhu/envoy that referenced this pull request Jun 25, 2020
yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jul 24, 2020
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.

2 participants