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 support for getpeername on Windows #201

Merged
merged 3 commits into from Jul 25, 2018

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Contributor

commented Jul 25, 2018

The implementation of this method closely follows getsockname. Adding
the method enables use-cases such as https://github.com/ScR4tCh/timebox

This change was created together with @Smarker.

Add support for getpeername on Windows
The implementation of this method closely follows getsockname. Adding
the method enables use-cases such as https://github.com/ScR4tCh/timebox

This change was created together with @Smarker.
@rgov
Copy link
Member

left a comment

Thanks for this contribution! It looks rather straightforward so I think it can be merged in pretty quickly, but a few review points to consider.

int sockfd = -1;
SOCKADDR_BTH sa = { 0 };
int sa_len = sizeof(sa);
char buf[100] = { 0 };

This comment has been minimized.

Copy link
@rgov

rgov Jul 25, 2018

Member

This buffer length doesn't seem appropriate; at most it can be 2*6 hex digits + 5 colons + 1 null = 18 bytes right?

This comment has been minimized.

Copy link
@c-w

c-w Jul 25, 2018

Author Contributor

This is code is copied from the getsockname implementation:

pybluez/msbt/_msbt.c

Lines 324 to 329 in f3b3cfb

msbt_getsockname(PyObject *self, PyObject *args)
{
int sockfd = -1;
SOCKADDR_BTH sa = { 0 };
int sa_len = sizeof(sa);
char buf[100] = { 0 };

The getsockname and getpeername functions have the same interface in the winsock2.h documentation, so would you like me to also update the getsockname code?

This comment has been minimized.

Copy link
@rgov

rgov Jul 25, 2018

Member

Yeah, please do. 18 is the right buffer size to pass to ba2str.

This comment has been minimized.

Copy link
@c-w

c-w Jul 25, 2018

Author Contributor

Done in 7c86236.

ba2str( sa.btAddr, buf, buf_len );
return Py_BuildValue( "si", buf, sa.port );
};
PyDoc_STRVAR(msbt_getpeername_doc, "TODO");

This comment has been minimized.

Copy link
@rgov

rgov Jul 25, 2018

Member

I know that many other routines aren't documented, but if this one could be, it would be great. Just a quick note about what it does and that it might raise a certain kind of exception if it fails.

This comment has been minimized.

Copy link
@c-w

c-w Jul 25, 2018

Author Contributor

Done in 6fcc5f6.

c-w added some commits Jul 25, 2018

Reduce getpeername and getsockname buffer sizes
The buffer size can be at most:
2*6 hex digits
+ 5 colons
+ 1 null
= 18 bytes.
@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

Thanks for the super quick review, @rgov. I believe that I've addressed your comments. Could you please take another look?

@rgov

rgov approved these changes Jul 25, 2018

@rgov rgov merged commit 23d3471 into pybluez:master Jul 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgov

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

Thank you for this contribution. If there are others you have in mind, the project could really use the attention, particularly on Windows where we receive a lot of complaints about installing it in the first place.

@c-w c-w deleted the c-w:windows-getpeername branch Jul 25, 2018

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

Thanks for the merge! I don't often work with Bluetooth. I was just playing with timebox earlier today and found some issues with the library's Windows support. I'll submit additional PRs if I find more features that are not working during my experiments.

@karulis

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2018

Also thanks for contribution:)
Now we need also Linux implementation for it to be really cross platform.
http://man7.org/linux/man-pages/man2/getpeername.2.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.