-
Notifications
You must be signed in to change notification settings - Fork 30
Addition of network abstraction layer to separate socket implementation code from network communication logic #508
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
base: main
Are you sure you want to change the base?
Conversation
…create_clock_server, because there are no plans using other network stacks rather than UDP.
…e. && Change all read() write(), and shutdown() to use netdrv
…work driver is initialized, and send default values when not initialized.
| if (errno == ECONNRESET) { | ||
| lf_print_error("Socket connection to the RTI was closed by the RTI without" | ||
| " properly sending an EOF first. Considering this a soft error."); | ||
| // NOTE: If this happens, possibly a new RTI must be elected. | ||
| shutdown_socket(&_fed.socket_TCP_RTI, false); | ||
| return NULL; | ||
| } else { | ||
| lf_print_error("Socket connection to the RTI has been broken with error %d: %s." | ||
| " The RTI should close connections with an EOF first." | ||
| " Considering this a soft error.", | ||
| errno, strerror(errno)); | ||
| // NOTE: If this happens, possibly a new RTI must be elected. | ||
| shutdown_socket(&_fed.socket_TCP_RTI, false); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside read_from_netchan(), the errno will be printed when read_from_socket() fails.
edwardalee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't made it all the way though, but it's looking pretty good! One issue is that while I think "network abstraction" is a good term to use for the general architecture, I find the resulting names unnecessarily long. I suggest dropping the "abstraction" in most cases. E.g.:
fed->fed_net_abstraction -> fed->net
write_to_net_abstraction -> write_to_net
write_to_net_abstraction_fail_on_error -> write_to_net_fail_on_error
read_from_net_abstraction_fail_on_error -> read_from_net_fail_on_error
Hello Prof. Lee, thank you for your comment. I shortened most of the names. net_abstractions_for_inbound_p2p_connections -> net_for_inbound_p2p_connections |
|
|
||
| #include "util.h" | ||
| #include "socket_common.h" | ||
| #include "util.h" // LF_MUTEX_UNLOCK(), logging.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #include "util.h" // LF_MUTEX_UNLOCK(), logging.h | |
| #include <logging.h> | |
| #include "util.h" // LF_MUTEX_UNLOCK() |
| } | ||
|
|
||
| void send_physical_clock(unsigned char message_type, federate_info_t* fed, socket_type_t socket_type) { | ||
| void send_physical_clock(unsigned char message_type, federate_info_t* fed, bool use_UDP) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think socket_type == UDP is more readable than true. I recommend changing this back to using the socket_type_t enum unless there is a pressing reason to change this to bool.
| } | ||
|
|
||
| int handle_T1_clock_sync_message(unsigned char* buffer, int socket, instant_t t2) { | ||
| int handle_T1_clock_sync_message(unsigned char* buffer, void* socket_or_net, instant_t t2, bool use_udp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument name here is inconsistent with the one above: use_UDP vs. use_udp.
lf-lang/lingua-franca#2455
Summary
This PR adds a network interface layer to support interoperability in terms of network protocols, and security, using the
net_abstraction.The original code is very tied up with sockets, making it hard to add different network features.
However, I clarify that this PR does not remove all socket related functions out of the main flow from the RTI and federate, for two reasons.
MSG_TYPE_ADDRESS_ADVERTISEMENTandMSG_TYPE_ADDRESS_QUERYuses the port and IP address in the protocol itself.We can completely take these socket-related stuff using
#ifdef COMM_TYPE_TCP; however, I highlight that this PR is more concentrated on supporting end-to-end pluggable and interchangeable network security based on TCP. Thus, in this PR, I have not added the#ifdefguards, and have not changed the protocol forMSG_TYPE_ADDRESS_ADVERTISEMENTandMSG_TYPE_ADDRESS_QUERY.Features
Plugin API for
networkThis PR creates a separate-compiled library on the
networkinstead of a part of the core runtime.I followed the prior work on
low_level_platform.handplatform.h.All source files related to network is moved under
network/impl/src, and all headers are undernetwork/api. Also there are separateCMakelists.txtfor each.Add
COMM_TYPEtarget property.The
comm-typekeyword is available in theCtarget as follows.Currently only supports
TCP, and plan to supportSSTfor security.Refactoring on clock-synchronization.
There is no other reason to do clock synchronization besides UDP. So, I left all clock-sync functions to directly use UDP sockets, and refactored these functions.
rti_remote.csend_physical_clock()andhandle_physical_clock_sync_message(): Uses boolean flaguse_UDPwhen UDP socket used.clock-sync.chandle_T4_clock_sync_message(): Uses boolean flaguse_UDPwhen UDP socket used.handle_T1_clock_sync_message(): Usevoid* socket_or_net_abstractionas parameter to support both socket and network abstraction.handle_address_ad()andhandle_address_query()There are no changes in the protocol.
As explained in the summary,
MSG_TYPE_ADDRESS_ADVERTISEMENTandMSG_TYPE_ADDRESS_QUERYuses port numbers and IP addresses in the protocol itself. To explain further, for physical connections or decentralized coordination, thefederateAhas to know the peerfederateB's port number and IP address to directly connect to it. This is done byfederateAsending aMSG_TYPE_ADDRESS_ADVERTISEMENTto theRTIit's port, and theRTIsends aMSG_TYPE_ADDRESS_QUERY_REPLYmessage to the peerfederateB, including the port number and IP address. Thus, the port number and IP address itself cannot be encapsulated under the network abstraction layer, as it is included in the protocol.Therefore, there are some
get()andset()calls to the network interface.FedA- >RTI:MSG_TYPE_ADDRESS_ADVERTISEMENT:FedAcallsget_my_port()to send its own port to theRTI.FedB->RTI:MSG_TYPE_ADDRESS_QUERY: No changes.RTI->FedB:MSG_TYPE_ADDRESS_QUERY_REPLY:RTIcallsget_server_port()andget_ip_addr()to encode it to the message to send toFedB.FedB: Sets the received port and IP address byset_server_port()andset_server_host_name()to directly connect toFedA.Minor logic change: Move
getpeername()logic toaccept_net_abstraction()The RTI should know the connected federate(FedA)'s IP address, to pass the IP address to the other federate(FedB), as explained above.
Before, this was done in
rti_remote.c'sreceive_and_check_fed_id_message(). I moved this tolf_socket_support.c'saccept_net_abstraction(), because it looks more appropriate to set the connected peer's information inside this function.One inefficiency that happens is that in decentralized coordination, the server federate does not need to save the connected peer federate's IP address. However, I think this will barely affect the performance.
Add default UDP port number as 15061.
The UDP port was usually set to the
RTI's port + 1, inrti_remote.c'screate_server()function call. However, I did not want to expose the port number in thecreate_server()interface, so I took the parameterportout fromcreate_server().Minor changes
start_rti_server()function does not get theportas a parameter. Theportwill be saved inrti_remote_t;suser_specified_port, with a default value when not set up.