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

Fix 14611 - socket.localAddress fails on Unix sockets with longer path (> 13 characters) #3300

Merged
merged 4 commits into from May 22, 2015

Conversation

dcarp
Copy link
Contributor

@dcarp dcarp commented May 20, 2015

@CyberShadow
Copy link
Member

The previous implementation supported paths of any size. What is the reason for restricting the path length to the one defined in system headers? I don't think the length is important as far as the ABI is concerned, and your change may result in regressions.

On reviewing the documentation, it looks like this change is correct.

@CyberShadow
Copy link
Member

Don't forget to add the test case from the bug report as a new unit test.

@dcarp
Copy link
Contributor Author

dcarp commented May 22, 2015

assert(listener.localAddress.toString == name);

... in the existing unittest will trigger the bug, thus no additional test case.

@CyberShadow
Copy link
Member

OK, thanks. LGTM.

@schveiguy schveiguy changed the title Fix 14611 Fix 14611 - socket.localAddress fails on Unix sockets with longer path (> 13 characters) May 22, 2015

this() pure nothrow @nogc
{
sun.sun_family = AF_UNIX;
sun.sun_path[].uninitializedFill('?');
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense. sun_path is a byte[], uninitializedFill is only needed for structs with copy ctors, and for cases where the data is uninitialized (in this case, it will be initialized).

I don't think we need a specialized function here, just:

sun.sun_path[] = '?';

And remove the import for uninitializedFill

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! I didn't know that you can fill an array like this.

@schveiguy
Copy link
Member

I updated the title. Please in the future include the bug title in the PR title so it's easy to know what the PR is about. Thanks.

LGTM except for the uninitializedFill thing

@schveiguy
Copy link
Member

Auto-merge toggled on

schveiguy added a commit that referenced this pull request May 22, 2015
Fix 14611 - socket.localAddress fails on Unix sockets with longer path (> 13 characters)
@schveiguy schveiguy merged commit b8bc7c1 into dlang:master May 22, 2015
@dcarp dcarp deleted the Fix_14611 branch May 22, 2015 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants