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.

Sign up for GitHub

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

Jump to bottom

Added startup, shutdown methods to IpSocket and added startup as a re… #2261

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Added startup, shutdown methods to IpSocket and added startup as a re… #2261

merged 4 commits into from
Oct 24, 2023

Conversation

LeStarch
Copy link
Collaborator

…try step
Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

Briefly, fixes the TcpServer such that it will retry both the listen and accept steps not just the accept step.

More in-depth:

Sockets originally were implemented with open and close steps. However, some sockets (like the TcpServer) needed to have startup and showdown steps too. This was patched in directly to this socket, which meant that retries on generic sockets didn't handle this case.

This PR adds:

  1. startup and shutdown methods generically to all sockets (with the default being no-op)
  2. isStarted method to check status
  3. Retry start up in the read loop if not started.

@LeStarch LeStarch requested a review from thomas-bc August 28, 2023 22:46
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 28, 2023
View reviewed changes
Drv/Ip/IpSocket.cpp
@@ -98,23 +98,45 @@
return SOCK_SUCCESS;
}

bool IpSocket::isStarted() {

Check notice

Code scanning / CodeQL

Use of basic integral type

isStarted uses the basic integral type bool rather than a typedef with size and signedness.
Drv/Ip/IpSocket.hpp
@@ -179,6 +206,7 @@
U32 m_timeoutMicroseconds;
U16 m_port; //!< IP address port used
bool m_open; //!< Have we successfully opened
bool m_started; //!< Have we successfully started the socket

Check notice

Code scanning / CodeQL

Use of basic integral type

m_started uses the basic integral type bool rather than a typedef with size and signedness.
Drv/Ip/TcpServerSocket.cpp Fixed Show fixed Hide fixed
Drv/Ip/IpSocket.cpp
@@ -98,23 +98,45 @@
return SOCK_SUCCESS;
}

bool IpSocket::isStarted() {
bool is_started = false;

Check notice

Code scanning / CodeQL

Use of basic integral type

is_started uses the basic integral type bool rather than a typedef with size and signedness.
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 28, 2023
View reviewed changes
Drv/Ip/SocketReadTask.cpp
Comment on lines +75 to +76
if ((not self->getSocketHandler().isStarted()) and (not self->m_stop) and
((status = self->startup()) != SOCK_SUCCESS)) {

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression

This Boolean expression is not side-effect free.
Drv/Ip/TcpServerSocket.cpp Fixed Show resolved Hide resolved
Drv/Ip/SocketReadTask.cpp Fixed Show resolved Hide resolved
Drv/Ip/TcpServerSocket.cpp Fixed Show resolved Hide resolved
LeStarch
LeStarch commented Aug 28, 2023
View reviewed changes
Copy link
Collaborator Author

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Possible issues:

  1. Inconsistent use of this->getSocketHandler()->[[ action ]] and this->[[ action]] in SocketReadTask
  2. No check for isStarted in socket read thread loop

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 28, 2023
View reviewed changes
Drv/Ip/TcpServerSocket.cpp
}

SocketIpStatus TcpServerSocket::openProtocol(NATIVE_INT_TYPE& fd) {
NATIVE_INT_TYPE clientFd = -1;
NATIVE_INT_TYPE serverFd = -1;

Check notice

Code scanning / CodeQL

Use of basic integral type

serverFd uses the basic integral type int rather than a typedef with size and signedness.
@LeStarch
Copy link
Collaborator Author

This code works. Tested with:

nc -l 50000
fprime-gds --ip-client

Then killed nc. Waited for socket wait then the server socket reconnected!

@LeStarch LeStarch added the Update Instructions Needed Need to add instructions in the release notes for updates. label Aug 29, 2023
thomas-bc
thomas-bc approved these changes Aug 30, 2023
View reviewed changes
Copy link
Collaborator

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

Looks good to me!
Couple of notes:

  • We're going to want to remove the explicit calls to startup/shutdown in the fprime-tools cookiecutters - also in references/tutorials if we switched any to be TcpServers already?
  • Browsing through the code while reviewing this, I noticed that all references to Berkeley sockets in the codebase are misspelled (missing an "e" in Berkeley, e.g. here). This PR could be a good opportunity to fix that with a quick search/replace?

@thomas-bc thomas-bc mentioned this pull request Aug 31, 2023
Merged
@thomas-bc thomas-bc mentioned this pull request Oct 3, 2023
Merged
@thomas-bc
Copy link
Collaborator

REMINDER

Please merge nasa/fprime-tools#168 after this has been merged

@LeStarch LeStarch merged commit 5e0b388 into nasa:devel Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-bc thomas-bc thomas-bc approved these changes

Assignees
No one assigned
Labels
Update Instructions Needed Need to add instructions in the release notes for updates.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LeStarch @thomas-bc