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

Standardize socket addresses to network format #1808

Merged
merged 8 commits into from
Dec 18, 2024
Merged

Conversation

huitema
Copy link
Collaborator

@huitema huitema commented Dec 14, 2024

The previous socket code in sockloop.c and the various address setting/getting APIs used a mix of "host" and "network" format depending on the calls. This was somewhat confusing. This PR standardize the use of network or host order for the "port" values with two guidelines:

  • the high level API, described in picoquic.h, always pass the addresses in host format. For example, is the port number is 443, the value of sin_port will be 443, regardless of whether the local host is using big endian or little endian format.
  • the translation between host order and network order will be performed by the low level code, such as for example sockloop.c.

The PR also clarifies the rules for selecting the outgoing socket. For outgoing packets, the outgoing socket will be chosen based on the Address Family of the destination address (addr_to) and the port number of the source address (addr_from), with the following rules:

  • the code in sockloop.c will always pick a socket bound to the same address family as the destination address.
  • if the port number is set and there is a socket bound to that port number, that socket will be selected.
  • if the port number is set but there is no match, the packet will be dropped.
  • if the port number is left to zero, or if the source address is not specified at all. the first socket will be used -- unless the parameter prefer_extra_socket is set in the call to sockloop, in which case the last socket will be selected.

@huitema
Copy link
Collaborator Author

huitema commented Dec 15, 2024

@victorstewart this PR changes the way the Picoquic stack interprets the port numbers in addresses passed through the "incoming packet" and "prepare packet" API. I asked you to review, because I am concerned it might affect your implementation.

@victorstewart
Copy link
Collaborator

victorstewart commented Dec 15, 2024

i'm not sure why it makes sense to standardize around host byte order instead of network byte order, but whatever you want to do i've identified the places in my code that would need conversions.

@huitema
Copy link
Collaborator Author

huitema commented Dec 15, 2024

@victorstewart the problem are the user level API that let users set or get the IP addresses. I suppose we could set all of them in network order, but I have observed that this creates confusion with some users. Let's wait for other reviews before merging this.

@victorstewart
Copy link
Collaborator

victorstewart commented Dec 15, 2024

i would see the argument for host byte order if the interfaces were like setDestination(const char *address, uint16_t port), but the fact that they're struct sockaddr that choice makes less sense to me. usage of that structure already assumes a user knows what he or she is doing; those structures were intended to be used with network byte order-- for example the address in the struct is stored in network byte order.

@huitema
Copy link
Collaborator Author

huitema commented Dec 16, 2024

@victorstewart I asked for a review because I wanted feedback, so thank you for providing it. Yes, standardizing on network order would also be consistent. It will make the socket code a bit simpler -- no need to "reorder" the addresses exchanged with the stack through the API picoquic_incoming_packet_ex and picoquic_prepare_next_packet_ex.

The caveat is for the application interfaces -- the server address parameter in picoquic_create_cnx, the local address parameter in picoquic_set_local_addr, the path local and destination address in picoquic_probe_new_path_ex. The application programmer will have to ensure that the port number in these structures is set in network order.

I am also waiting a review from @suhasHere. If he agrees with your feedback, I will update the PR to standardize on network order.

@huitema
Copy link
Collaborator Author

huitema commented Dec 17, 2024

I did a "PR on the PR" to standardize on network order. Interestingly, I did not need to change the code in the picoquicdemo application at all -- I just needed to update the code in the function picoquic_get_server_address to set the port number in network order in the returned sockaddr.

@huitema huitema changed the title Standardize socket addresses to host format Standardize socket addresses to network format Dec 18, 2024
@huitema huitema merged commit 9c31c31 into master Dec 18, 2024
11 checks passed
@huitema huitema deleted the check-ntohs-htons branch December 18, 2024 20:10
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