-
Notifications
You must be signed in to change notification settings - Fork 654
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
Fix network interface identification for VLAN in IPv6 #553
base: master
Are you sure you want to change the base?
Fix network interface identification for VLAN in IPv6 #553
Conversation
…n case an IPv6 address with interface ID is provided
Hi @nachtfuchs, Also, please remove unnecessary debug logs. cc: @goncaloalmeida |
…ch compromised the build
…ts adapted to vsomeip coding style
Thanks for the quick feedback. I made the changes in the browser instead of actually cloning the branch. During the process, I introduced a bug that compromised the build. I am not going to do changes in the browser in future pull-requests. |
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.
please don't include TODOs in our implementation
@@ -41,6 +41,8 @@ | |||
#include "../../security/include/policy_manager_impl.hpp" | |||
#include "../../security/include/security.hpp" | |||
|
|||
#include <ifaddrs.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.
We cannot accept a PR that breaks our windows build, this is linux only.
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.
added if defined(__linux__) .... #endif
to address it
@@ -1380,6 +1382,64 @@ void configuration_impl::load_trace_filter_match( | |||
} | |||
} | |||
|
|||
int configuration_impl::get_nic(const std::string& _local_address) |
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.
int configuration_impl::get_nic(const std::string& _local_address) {
searched_address.sin6_port = htons( 0xdead ); | ||
inet_pton( AF_INET6, _local_address.c_str(), &searched_address.sin6_addr ); // actually copying | ||
|
||
if (getifaddrs(&ifaddr) == -1) // "-1" is an error value |
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.
if (getifaddrs(&ifaddr) == -1) { // "-1" is an error value
|
||
if (getifaddrs(&ifaddr) == -1) // "-1" is an error value | ||
{ | ||
VSOMEIP_ERROR << __func__ << |
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.
Not sure is VSOMEIP_ERROR is the best approach, maybe WARNING
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 prefer error, because the communication does not work. As a user, I would not expect a warning when something does not work.
Edit: Now that I think twice about it, it is potato potato.
} | ||
|
||
/* Walk through linked list of network interfaces*/ | ||
for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) |
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.
for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) {
|
||
/* For an AF_INET6 interface address, compare the address of the NIC | ||
with the searched address */ | ||
if (family == AF_INET6) |
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.
same as the others before
if (family == AF_INET6) | ||
{ | ||
p_addrIP6 = reinterpret_cast<sockaddr_in6*>(ifa->ifa_addr); | ||
if (::bcmp(p_addrIP6->sin6_addr.s6_addr, searched_address.sin6_addr.s6_addr, 16) == 0) |
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.
... you get the point
|
||
// PRECONDITION: netlink_connector_ has troubles to create a correct object, | ||
// if network interface is part of unicast-address-string. So strip it! | ||
char delimiter {'%'}; // "%" is a symbol for the device, e.g. "beef::1%7", |
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.
Is 7 a hardcoded value for the Network Interface you want to use?
If yes, we will need another solution not using an hardcoded value
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 feature is the "delimiter", not the value "7". The example string is for instrumental purposes so that other developers know, why the delimiter was chosen the way it is.
if (getifaddrs(&ifaddr) == -1) // "-1" is an error value | ||
{ | ||
VSOMEIP_ERROR << __func__ << | ||
strerror(errno) << ::std::endl; |
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.
You need to give a space after the func or you will have the two things together.
} | ||
if (nic.size() > 1) | ||
{ | ||
VSOMEIP_ERROR << __func__ << " found multiple network interfaces " << |
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.
Maybe start the log message with :
<< func << ": Found....
And again now sure if using ERROR is the best approach
Finished fixing the review findings from 10th November. |
Attention: pull-request only tested with Linux.
The Problem
VLAN in IPv6 did not work with vsomeip 3.3.8 for communication partners that use a different SOME/IP stack than vsomeip. The problem is that the wrong network interface is used to send the message. The network interface is chosen based on the unicast-IPv6-address in the json file. The effect is that there is no 802.1Q field in the sent Ethernet frame during service discovery. Consequently, the communication partner does not talk on the same "channel".
Providing a network interface to the unicast address in the json-file shows no effect, i.e. {"unicast" : "beef::1%7"}.
This problem is NOT observed with IPv4.
The solution
So I chose to identify the network interface address during the configuration part when the user defined "unicast" address is fixed. The idea of the solution is to go through all network interfaces on the system and find the "unicast" address that is assigned to one of the network interfaces. Its ID is used to extend the given IPv6 unicast-address and this way create the correct socket for communication using the boost library.
The test environment
I tested this with a Xenomai 3.2.2 operating system on the one end, and a embedded ECU on the other end. The ECU runs a different SOME/IP-stack than my Xenomai machine. Xenomai is based on a Linux Kernel.
Alternative solutions
An alternative could be to to change "udp_server_endpoint_impl.cpp" and assign the correct outbound interface in "boost::asio::ip::multicast::outbound_interface" for IPv6. I did not look into it, because I thought that the IPv6 address with network interface ID might be used somewhere else during the setup of the communication.