Ipv4_packet improvements #1915
Ipv4_packet improvements #1915
Conversation
| unsigned _header_length : 4; | ||
| unsigned _version : 4; |
There was a problem hiding this comment.
why do you change the order of version and IHL header fields here? The standard says that the version field is followed by the header length field, isn't it?
There was a problem hiding this comment.
I think this was a byte-order issue. Apparently, bitfields are affected by the endianess so both fields where shuffled in the memory layout. I tested this with wireshark.
There was a problem hiding this comment.
A strong sign to replace the bit fields with our register/bit set framework.
There was a problem hiding this comment.
Ok, the problem is that although it is one byte (version and IHL) and byte-order is correct here, within bitfields the bit-order is not correctly reproduced here. As an interims fix the re-order is a good solution. In general, I agree to chelmuth that we should get rid of bitfields here and use the register framework (which did not exist yet when those headers got written).
Apart from that, it would be nice when the header length return value would be expressed in bytes instead of 32bit words.
|
I'd like to merge these improvements, but am uncertain about the state of the header-length return value discussion... @skalk? @ValiValpas? |
|
@ValiValpas @chelmuth as I already wrote, the re-ordering of the bitfields is good as an interims solution before we replace them with the register framework. On the other hand I'd like to replace the header length return value to not return 32-bit word count, but byte count. Because although the IPv4 standard says that this field in the header returns 32-bit fields, I assume most programmers suppose to get byte count when having a size_t return value, and otherwise have to turn the value into byte count anyway. |
Supports stronger typing of raw accesses and const correctness. Issue #1915
… message amendment) Replace size_t by uint8_t in accessors for the IPv4 header fields 'version' and 'header_length' - uint8_t is the smallest integral type for 4 bit of information. Note, as the _internet header length_ field is defined to reflect the number of 32-bit words the header occupies, we also stick to the specification with our accessor. Issue #1915
Also adds header-checksum calculation function. Fixes #1915
…ror in net-stat) Issue #1915
|
@chelmuth I couldn't test this in my original setup because the nic_drv for linux is segfaulting. I was able to run the essential part on hw_zynq though and had a look at the packets with wireshark. Looks perfekt! |
|
Thanks. Regarding nic_drv segfault I assume you're using a recent Linux distro. We had some issues with Ubuntu 15.10 / 16.04 recently and will address these in the next weeks. |
|
Yes, I'm using Archlinux:
|
Supports stronger typing of raw accesses and const correctness. Issue #1915
Replace size_t by uint8_t in accessors for the IPv4 header fields 'version' and 'header_length' - uint8_t is the smallest integral type for 4 bit of information. Note, as the _internet header length_ field is defined to reflect the number of 32-bit words the header occupies, we also stick to the specification with our accessor. Issue #1915
Adding write accessors to the Ipv4 header fields, including checksum generation. Also fixing the header_length field.