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

Add Socket::Address.from without addrlen #15060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mamantoha
Copy link
Contributor

I'm not sure why this was implemented this way. The addlen parameter is not necessary when creating an address from a LibC::Sockaddr pointer, as it can be obtained directly within the method.

src/socket/address.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member

addrlen is used by the system API for generalization purposes. And yeah it's completely unnecessary for structs with fixed sizes.
So I think we could probably update the entire Address API to deprecate the addrlen parameters. Needs further investigation though.

Looking a bit into this I noticed some weird and unsafe behaviour. We're passing around stack pointers for LibC::SockAddr` outside of their scope. This needs a bit more inspection as well.

Considering this, I'd like to hold this PR for a while until we can look a bit better at the details of what else we want to change.

@ysbaddaden
Copy link
Contributor

We're passing around stack pointers for LibC::SockAddr` outside of their scope.

We never capture the pointer, though.

@straight-shoota
Copy link
Member

I think I had seen somewhere a method which returned a pointer to a stack-allocated LibC::Sockaddr*. But I can't find it anymore, so I probably got confused (the socket address code is quite confusing sometimes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants