Skip to content

netsocket: add get_remote_peer() method to InternetSocket #8565

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

Closed

Conversation

unsignedint
Copy link

@unsignedint unsignedint commented Oct 29, 2018

Description

With the TCP API changes in 5.10 it seems like there is now no easy way to access the address information of the remote peer, because it is not exposed in the protected _remote_peer member. This PR adds a get method to expose it as a const pointer.

The end goal is to replicate previous behaviour from 5.9:

TCPServer srv;
TCPSocket client_sock;
SocketAddress client_addr;
...
srv.accept(&client_sock, &client_addr);
printf("Client connected: %s", client_addr.get_ip_address());

And now we can write:

client_sock = sock.accept();
client_addr = client_sock->getpeername();

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

I have labeled this a "fix" but it can be considered an enhancement, too, I guess.

@0xc0170 0xc0170 requested a review from a team October 29, 2018 10:05
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 29, 2018

Thank you for the contribution, we will review and provide feedback (someone from the @ARMmbed/mbed-os-ipcore team)

@SeppoTakalo
Copy link
Contributor

I feel like it should be getpeername() as in POSIX standard.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpeername.html

All other functions inherit the name and behaviour from POSIX as well.

@unsignedint unsignedint changed the title netsocket: add get_remote_peer() method to TCPSocket netsocket: add get_remote_peer() method to InternetSocket Nov 5, 2018
@unsignedint
Copy link
Author

@SeppoTakalo I have renamed to getpeername() and moved up one level to the InternetSocket class, presumably its could also be useful for UDP connections.

@unsignedint
Copy link
Author

Also, sorry, I'm still waiting on signed CLA >_<

@kjbracey
Copy link
Contributor

kjbracey commented Nov 6, 2018

Discussed this with @SeppoTakalo - it's probably best to incorporate the API all the way up in the base Socket interface - it is conceptually required for its base accept.

This is related to #5922 - we concluded there that getsockname was too hard on too many platforms, but getpeername would be a good idea. And accept has since made it necessary.

If it goes all the way up to Socket then TLSSocketWrapper can also implement it by passing through.

*
* @return pointer to the remote_peer SocketAddress
*/
const SocketAddress* getpeername();
Copy link
Contributor

Choose a reason for hiding this comment

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

For lifetime reasons and general POSIX compatibility - aligning with the way we've mapped other functions - this should be prototyped as virtual nsapi_error_t getpeername(SocketAddress *address), assuming implementing a pure virtual in Socket as suggested.

(Other calls already omit the POSIX length parameter for addresses, on the assumption SocketAddress has space for all possible address types...)

@SeppoTakalo
Copy link
Contributor

Park this change.

We as a IP networking team will do this change so that it will become official part of Mbed OS Socket API.

@SeppoTakalo
Copy link
Contributor

API change submitted in #8651

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 6, 2018

API change submitted in #8651

@unsignedint Please review 8651.

We will close this PR as 8651 extends it. Thank you for your first contribution

@0xc0170 0xc0170 closed this Nov 6, 2018
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.

4 participants