-
Notifications
You must be signed in to change notification settings - Fork 350
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
initial qnx vlan support #1931
initial qnx vlan support #1931
Conversation
5db253b
to
b67ab2d
Compare
Signed-off-by: Marcel Jordense <[email protected]>
31c0530
to
b9a6397
Compare
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.
Thanks @MarcelJordense, I have only done a quick review, but it generally looks fine. I have a few minor comments, perhaps you could take a look?
I am curious to try it on macOS, but I haven't had a chance yet.
src/core/ddsi/src/ddsi_raweth.c
Outdated
#if defined(__FreeBSD__) || defined(__APPLE__) | ||
#include <net/ethernet.h> | ||
#elif defined(__QNXNTO__) | ||
#define DDSI_BPF_IS_CONING_DEV (1) |
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 suspect a typo here, that is meant to be DDSI_BPF_IS_CLONING_DEV
src/core/ddsi/src/ddsi_raweth.c
Outdated
return DDS_RETCODE_ERROR; | ||
} | ||
|
||
#if defined(DDSI_BPF_IS_CONING_DEV) |
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 my suspicion on this above is correct, then this should also be DDSI_BPF_IS_CLONING_DEV
.
src/core/ddsi/src/ddsi_raweth.c
Outdated
ddsrt_close (sock); | ||
DDS_CWARNING (&fact->gv->logconfig, "ddsi_raweth_create_conn %s port %u could not set directiion ... retcode = %d\n", mcast ? "multicast" : "unicast", port, r); | ||
} | ||
#elif defined(__QNXNTO__) || defined(__APPLLE__) |
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.
#elif defined(__QNXNTO__) || defined(__APPLLE__) | |
#elif defined(__QNXNTO__) || defined(__APPLE__) |
src/core/ddsi/src/ddsi_raweth.c
Outdated
port = (uint32_t)(ntohs (eth_hdr->proto)); | ||
if (vtag) | ||
{ | ||
port += ((uint32_t)(vtag->tag & 0xfff) << 20) + ((uint32_t)(vtag->tag & 0xf000) << 16); |
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 it makes sense not to duplicate this. I have no problem with the magic numbers and the bit fiddling, but it is a fiddly bit of code and it is nice to have only one copy to check.
Setting the source locator is also the same, I'd also de-dup that.
src/core/ddsi/src/ddsi_raweth.c
Outdated
ushort etype = (ushort)(port & 0xFFFF); | ||
struct bpf_insn insns[] = { | ||
BPF_STMT(BPF_LD+BPF_H+BPF_ABS, 12), | ||
BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, etype, 3, 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.
BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, etype, 3, 0), | |
BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, etype, 3, 0), |
uc->bptr = uc->buffer; | ||
} | ||
|
||
if (uc->bptr < uc->buffer + uc->avail) |
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 all the bptr
manipulations/checks: these structures come straight from the OS kernel, so I suppose we can trust them. I do think it would be good to point out in a comment that this is the reason there is so little checking, to help any future reader.
src/core/ddsi/src/ddsi_raweth.c
Outdated
memcpy(vhdr.e.dmac, dst->address + 10, 6); | ||
memcpy(vhdr.e.smac, uc->m_base.m_base.gv->interfaces[0].loc.address + 10, 6); | ||
|
||
if (vtag) { |
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.
Constructing the ethernet header also looks like a good candidate for de-duping.
src/core/ddsi/src/ddsi_raweth.c
Outdated
return DDS_RETCODE_ERROR; | ||
} | ||
|
||
buflen = gv->config.socket_rcvbuf_size.max.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.
buflen = gv->config.socket_rcvbuf_size.max.value; | |
buflen = gv->config.socket_rcvbuf_size.max.value; |
src/core/ddsi/src/ddsi_raweth.c
Outdated
ddsrt_close (sock); | ||
DDS_CWARNING (&fact->gv->logconfig, "ddsi_raweth_create_conn %s port %u could not set directiion ... retcode = %d\n", mcast ? "multicast" : "unicast", port, r); |
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.
ddsrt_close (sock); | |
DDS_CWARNING (&fact->gv->logconfig, "ddsi_raweth_create_conn %s port %u could not set directiion ... retcode = %d\n", mcast ? "multicast" : "unicast", port, r); | |
ddsrt_close (sock); | |
DDS_CWARNING (&fact->gv->logconfig, "ddsi_raweth_create_conn %s port %u could not set direction ... retcode = %d\n", mcast ? "multicast" : "unicast", port, r); |
src/core/ddsi/src/ddsi_raweth.c
Outdated
ddsrt_close (sock); | ||
DDS_CWARNING (&fact->gv->logconfig, "ddsi_raweth_create_conn %s port %u could not set directiion ... retcode = %d\n", mcast ? "multicast" : "unicast", port, r); |
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.
ddsrt_close (sock); | |
DDS_CWARNING (&fact->gv->logconfig, "ddsi_raweth_create_conn %s port %u could not set directiion ... retcode = %d\n", mcast ? "multicast" : "unicast", port, r); | |
ddsrt_close (sock); | |
DDS_CWARNING (&fact->gv->logconfig, "ddsi_raweth_create_conn %s port %u could not set direction ... retcode = %d\n", mcast ? "multicast" : "unicast", port, r); |
Signed-off-by: Marcel Jordense <[email protected]>
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.
Thanks @MarcelJordense
src/core/ddsi/src/ddsi_raweth.c
Outdated
@@ -121,7 +151,7 @@ static ssize_t ddsi_raweth_conn_read (struct ddsi_tran_conn * conn, unsigned cha | |||
struct tpacket_auxdata *pauxd; | |||
struct cmsghdr *cptr; | |||
uint16_t vtag = 0; | |||
uint32_t port; | |||
// uint32_t port; |
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.
// uint32_t port; |
src/core/ddsi/src/ddsi_raweth.c
Outdated
@@ -200,7 +223,7 @@ static ssize_t ddsi_raweth_conn_write (struct ddsi_tran_conn * conn, const ddsi_ | |||
struct msghdr msg; | |||
struct sockaddr_ll dstaddr; | |||
struct ddsi_vlan_header vhdr; | |||
uint16_t vtag; | |||
// uint16_t vtag; |
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.
// uint16_t vtag; |
src/core/ddsi/src/ddsi_raweth.c
Outdated
static void set_locator(ddsi_locator_t *srcloc, const uint8_t * addr, uint16_t port, uint16_t vtag) | ||
{ | ||
srcloc->kind = DDSI_LOCATOR_KIND_RAWETH; | ||
srcloc->port = (uint32_t)(port + ((vtag & 0xfff) << 20) + ((vtag & 0xf000) << 4)); |
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'd prefer it if port
and vtag
were cast to a uint32_t
before doing the arithmetic, so that all of it only involves unsigned 32-bit integers. I think the C99 section 6.5.7 "Bitwise shift operators" even says that if we are not careful we will get ourselves into trouble:
The integer promotions are performed on each of the operands. The type of the result is that of the promoted left operand.
The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros.
If E1 has a signed type and nonnegative value, and E1 × 2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.
So if vtag & 0xfff
≥ 0x800, so say 0x800 = 2^11, then (vtag & 0xfff) << 20
= 2^11 × 2^20 = 2^31, and 2^31 isn't representable in a signed 32 bit int. That would make it undefined behaviour.
It is quite possible that this hasn't been introduced by the refactoring and that I just happened to spot it now. Even if that's the case, I think it makes sense to fix it now.
srcloc->port = (uint32_t)(port + ((vtag & 0xfff) << 20) + ((vtag & 0xf000) << 4)); | |
srcloc->port = (uint32_t)port + (((uint32_t)vtag & 0xfff) << 20) + (((uint32_t)vtag & 0xf000) << 4)); |
src/core/ddsi/src/ddsi_raweth.c
Outdated
@@ -307,6 +331,7 @@ static dds_return_t ddsi_raweth_create_conn (struct ddsi_tran_conn **conn_out, s | |||
memset(&addr, 0, sizeof(addr)); | |||
addr.sll_family = AF_PACKET; | |||
addr.sll_protocol = htons(ETH_P_ALL); | |||
|
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.
memset(srcloc->address, 0, 10); | ||
memcpy(srcloc->address + 10, src.sll_addr, 6); | ||
} | ||
set_locator(srcloc, src.sll_addr, ntohs (src.sll_protocol), vtag); |
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 it really the case that vtag
a.k.a. pauxd->tp_vlan_tci
is already in native byte order? I'm asking only because it seems somewhat against the pattern, but it is not impossible.
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 this case the vlan tag is already in host order
src/core/ddsi/src/ddsi_raweth.c
Outdated
{ | ||
memcpy(buf, ptr, (size_t)ret); | ||
if (srcloc) | ||
set_locator(srcloc, eth_hdr->smac, ntohs (eth_hdr->proto), (vtag ? vtag->tag : 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.
Like in the Linux version, on consideration I am somewhat surprised that the tag would be in native byte order already. Perhaps you can double-check?
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.
In this case the vtag is of course in network order. I will correct this.
Signed-off-by: Marcel Jordense <[email protected]>
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.
Thanks @MarcelJordense !
While I don't like merging when there are CI failures, these are definitely unrelated and so it makes no sense to not merge this PR now.
Include the option to use raw ethernet transport and on QNX