Skip to content

Remove INET_ADDRSTRLEN limit for symbolic hostnames and optimize socket connections.#578

Merged
edwardalee merged 10 commits intomainfrom
ipv4-fix
Apr 15, 2026
Merged

Remove INET_ADDRSTRLEN limit for symbolic hostnames and optimize socket connections.#578
edwardalee merged 10 commits intomainfrom
ipv4-fix

Conversation

@Jakio815
Copy link
Copy Markdown
Collaborator

@Jakio815 Jakio815 commented Apr 13, 2026

Summary

  • Remove server_hostname from socket_priv_t, which was actually not necessary to save in the struct.
  • Fix bug setting pointers of net_abs in stack to NULL after shutdown_net().
  • Support connect_to_socket() use in_addr instead of char* hostname when available.

1. Remove server_hostname from socket_priv_t

There was a bug found in #576, which happened after merging #508, not allowing symbolic hostnames longer than 16 bytes (INET_ADDRSTRLEN) from being connected.
This happened because there was a memcpy() to save the host_name in char server_hostname[INET_ADDRSTRLEN];.

While investigating this, the code only used this for printing debug messages on the hostname, and it was never used again.
I removed the server_hostname field from socket_priv_t.

2. Bug Fix: Remove unnecessary NULL setting to pointers.

After shutdown_net(net_abs), I set all net_abs to NULL, however some were just pointers from the stack, so had no effect. Removed all of these. Thanks for reporting this @edwardalee.

3. Support connect_to_socket() use in_addr instead of char* hostname when available.

When a federate tries to connect another federate, the RTI sends the target federate's IP address in a network format (uint32_t), not the presentation format (192.168.0.1 or localhost).
However, connect_to_socket() only supported the presentation format as the input parameter.
So there was some inefficiency where federate.c does inet_ntop() to convert the recieved network format into the presentation format, and inside connect_to_socket, does getaddrinfo to convert that presentation format backward into the network format (struct sockaddr_in) to make the actual system call connect().

So, by updating connect_to_socket() to accept an optional struct in_addr*, this PR removes this redundant Network -> Presentation -> Network conversion, which removes unnecessary DNS resolution overhead.

Why we couldn't remove getaddrinfo() entirely?

The redundancy above was from lf_connect_to_federate, however, connect_to_net is used once again in lf_connect_to_rti.

However, when used in lf_connect_to_rti, this always uses the char* hostname, because the RTI's address is provided strictly as a human-readable string (e.g., "localhost", "192.168.0.1" or "rti-server.domain.com"), performing DNS resolution via getaddrinfo() is mandatory to discover its actual network routing address. Therefore, connect_to_socket() was updated to selectively bypass DNS only when a raw IP is available.

This suggestion was also made by @edwardalee, thanks for your insight!

… resolution if the IP address (`struct in_addr`) is already provided.
@Jakio815 Jakio815 changed the title Ipv4 fix Remove INET_ADDRSTRLEN limit for symbolic hostnames and optimize socket connections. Apr 13, 2026
@Jakio815 Jakio815 marked this pull request as ready for review April 13, 2026 14:07
@edwardalee edwardalee requested a review from Copilot April 13, 2026 14:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the socket-based network abstraction used by the federated runtime to (1) avoid truncating symbolic hostnames, and (2) optimize certain federate-to-federate connections by using an already-available IPv4 address directly instead of round-tripping through presentation format + DNS resolution.

Changes:

  • Removed persisted server_hostname storage from socket_priv_t (fixes hostname truncation caused by INET_ADDRSTRLEN-sized buffers).
  • Extended socket connection parameters to optionally pass a raw struct in_addr* and bypass getaddrinfo() when available.
  • Cleaned up ineffective “set local pointer to NULL after shutdown” code paths.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
network/impl/src/socket_common.c Removes debug hostname storage; adds connect_to_socket() fast-path using struct in_addr.
network/impl/src/lf_socket_support.c Stops copying hostname into a fixed-size buffer; passes hostname/IP through to connect_to_socket().
network/api/socket_common.h Adds optional server_ip_addr to connection params; removes server_hostname from socket_priv_t; updates API docs/signature.
core/federated/RTI/rti_remote.h Updates comments to reflect removal of server_hostname tracking.
core/federated/RTI/rti_remote.c Removes hostname-based debug log and ineffective NULL assignments after shutdown.
core/federated/federate.c Uses server_ip_addr when connecting to federates; avoids terminating accept loop on transient accept failure.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread network/impl/src/socket_common.c
Comment thread network/impl/src/socket_common.c Outdated
Comment thread core/federated/federate.c
Comment thread network/api/socket_common.h
Copy link
Copy Markdown
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

LGTM! I tested and am able to start a multi-machine federation now.

@Jakio815 Jakio815 enabled auto-merge April 15, 2026 17:10
@Jakio815 Jakio815 added this pull request to the merge queue Apr 15, 2026
@edwardalee edwardalee removed this pull request from the merge queue due to a manual request Apr 15, 2026
@edwardalee edwardalee merged commit fe69a79 into main Apr 15, 2026
30 checks passed
@Jakio815 Jakio815 deleted the ipv4-fix branch April 16, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants