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

Added TCP LinkLayer to socktap #191

Merged
merged 5 commits into from
Aug 9, 2023
Merged

Added TCP LinkLayer to socktap #191

merged 5 commits into from
Aug 9, 2023

Conversation

fysch
Copy link
Contributor

@fysch fysch commented Jun 7, 2023

I've added TCP to socktap.
To accept incoming connections, use --tcp-accept 127.0.0.1:12345.
To connect to another service, use --tcp-connect 127.0.0.1:12345.

Copy link
Owner

@riebl riebl left a comment

Choose a reason for hiding this comment

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

I like the addition of a TCP link layer, this is cool stuff!
Please have a look at my comments which I would like to sort out before merging. Thanks!

tools/socktap/link_layer.cpp Outdated Show resolved Hide resolved
tools/socktap/link_layer.cpp Outdated Show resolved Hide resolved
std::string tcp_ip;
unsigned short tcp_port, tcp_ip_len;

if (vm.count("tcp-connect")) {
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure if vm.count is necessary: An empty vector of string would be just fine.

tools/socktap/tcp_link.hpp Outdated Show resolved Hide resolved

private:
std::list<TcpSocket*> sockets_;
std::map<boost::asio::ip::tcp::endpoint, boost::asio::ip::tcp::acceptor*> acceptors_;
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, storing acceptor objects instead of pointers simplifies memory management.

tools/socktap/tcp_link.cpp Outdated Show resolved Hide resolved
tools/socktap/tcp_link.cpp Outdated Show resolved Hide resolved
@riebl
Copy link
Owner

riebl commented Jul 30, 2023

Nice! Let me give it a final try, I will merge it then.

@fysch
Copy link
Contributor Author

fysch commented Jul 30, 2023

I've just encountered some issues with it today.
I will try to resolve them in the next days.
So, don't worry about testing right now.

@fysch
Copy link
Contributor Author

fysch commented Jul 31, 2023

@riebl I have resolved the issues.

Copy link
Owner

@riebl riebl left a comment

Choose a reason for hiding this comment

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

I have just noticed that there is no frame delimiter in the TCP stream. Your implementation cannot cope with situations where the IP stack transmits/receives data in chunks of different sizes than your socket calls pass/expect.

tools/socktap/tcp_link.cpp Outdated Show resolved Hide resolved
tools/socktap/tcp_link.cpp Show resolved Hide resolved
@riebl
Copy link
Owner

riebl commented Aug 9, 2023

Thanks a lot for your patience while contributing your TCP link layer!

@riebl riebl merged commit 6696d4f into riebl:master Aug 9, 2023
5 checks passed
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