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

Remove bind-to-device support where unavailable #773

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove bind-to-device support where unavailable #773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62645cc
Remove bind-to-device support where unavailable
EriComic Oct 3, 2024
07bda51
Merge branch 'master' into io-sock_SO_BINDTODEVICE
EriComic Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions implementation/endpoints/src/tcp_client_endpoint_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,15 @@ void tcp_client_endpoint_impl::connect() {
// If specified, bind to device
std::string its_device(configuration_->get_device());
if (its_device != "") {
#if defined(SO_BINDTODEVICE)
if (setsockopt(socket_->native_handle(),
SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), static_cast<socklen_t>(its_device.size())) == -1) {
VSOMEIP_WARNING << "TCP Client: Could not bind to device \"" << its_device << "\"";
}
#else
VSOMEIP_WARNING << "TCP Client: Could not bind to device \"" << its_device << "\""
Copy link
Contributor

@goncaloalmeida goncaloalmeida Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

All the changes seem ok, but why not use the normal logging? VSOMEIP_WARNING << "tcp_client_endpoint::connect:.." or VSOMEIP_WARNING <<"tcp_client_endpoint::" << func ""..

cc @duartenfonseca

Copy link
Author

Choose a reason for hiding this comment

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

I see your point and I had a few ideas on how to make this better as well, but I decided against it for the following reasons:

  1. The warning log that was already present in the code uses the same formatting I have currently, I believe it makes sense to adhere to what's already there. I believe best practice is to keep logging style/formatting changes in separate commits in order to maintain a "prettier git log".
  2. Having function signatures in logs is very useful for developers working on the library but it's too noisy for the user using the library. Your proposed log would make more sense for a DEBUG or TRACE level log. Perhaps in addition to the currently proposed WARNING log.

TL;DR I think your suggestion is good but only useful for the developer and not-so-much the user. If you would like the addition of this log as a DEBUG or TRACE, then please add it, but I believe our proposed WARNING should be there as it is.

<< " SO_BINDTODEVICE socket option not supported";
#endif
}
#endif

Expand Down
5 changes: 5 additions & 0 deletions implementation/endpoints/src/tcp_server_endpoint_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,15 @@ void tcp_server_endpoint_impl::init(const endpoint_type& _local,
// If specified, bind to device
std::string its_device(configuration_->get_device());
if (its_device != "") {
#if defined(SO_BINDTODEVICE)
if (setsockopt(acceptor_.native_handle(), SOL_SOCKET, SO_BINDTODEVICE,
its_device.c_str(), static_cast<socklen_t>(its_device.size())) == -1) {
VSOMEIP_WARNING << "TCP Server: Could not bind to device \"" << its_device << "\"";
}
#else
VSOMEIP_WARNING << "TCP Server: Could not bind to device \"" << its_device << "\""
<< " SO_BINDTODEVICE socket option not supported";
#endif
}
#endif

Expand Down
5 changes: 5 additions & 0 deletions implementation/endpoints/src/udp_client_endpoint_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,15 @@ void udp_client_endpoint_impl::connect() {
// If specified, bind to device
std::string its_device(configuration_->get_device());
if (its_device != "") {
#if defined(SO_BINDTODEVICE)
if (setsockopt(socket_->native_handle(),
SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), socklen_t(its_device.size())) == -1) {
VSOMEIP_WARNING << "UDP Client: Could not bind to device \"" << its_device << "\"";
}
#else
VSOMEIP_WARNING << "UDP Client: Could not bind to device \"" << its_device << "\""
<< " SO_BINDTODEVICE socket option not supported";
#endif
}
#endif

Expand Down
5 changes: 5 additions & 0 deletions implementation/endpoints/src/udp_server_endpoint_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,15 @@ void udp_server_endpoint_impl::init(const endpoint_type& _local,
// If specified, bind to device
std::string its_device(configuration_->get_device());
if (its_device != "") {
#if defined(SO_BINDTODEVICE)
if (setsockopt(unicast_socket_->native_handle(), SOL_SOCKET, SO_BINDTODEVICE,
its_device.c_str(), static_cast<socklen_t>(its_device.size())) == -1) {
VSOMEIP_WARNING << "UDP Server: Could not bind to device \"" << its_device << "\"";
}
#else
VSOMEIP_WARNING << "UDP Server: Could not bind to device \"" << its_device << "\""
<< " SO_BINDTODEVICE socket option not supported";
#endif
}
#endif

Expand Down
Loading