On 23 April 2016 at 11:58, Jean-Christophe Dubois <j...@tribudubois.net> wrote: > This patch adds: > * based on Eth, UDP, TCP struct present in eth.h instead of hardcoded > indexes. > * based on various macros present in eth.h. > * allow to account for optional VLAN header.
This is doing several things: (1) changing style to use structs and macros rather than raw array accesses (which shouldn't affect functionality) (2) adding new functionality I think they would be better in separate patches. This function is used by multiple network devices, including the important ones xen_nic and virtio-net -- is it definitely OK to have it change behaviour for those devices? > Signed-off-by: Jean-Christophe Dubois <j...@tribudubois.net> > --- > net/checksum.c | 83 > ++++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 57 insertions(+), 26 deletions(-) > > diff --git a/net/checksum.c b/net/checksum.c > index d0fa424..fd25209 100644 > --- a/net/checksum.c > +++ b/net/checksum.c > @@ -18,9 +18,7 @@ > #include "qemu/osdep.h" > #include "qemu-common.h" > #include "net/checksum.h" > - > -#define PROTO_TCP 6 > -#define PROTO_UDP 17 > +#include "net/eth.h" > > uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq) > { > @@ -57,40 +55,73 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t > proto, > > void net_checksum_calculate(uint8_t *data, int length) > { > - int hlen, plen, proto, csum_offset; > - uint16_t csum; > + int plen; > + struct ip_header *ip; > + > + /* Ensure we have at least a Eth header */ > + if (length < sizeof(struct eth_header)) { > + return; > + } > > - /* Ensure data has complete L2 & L3 headers. */ > - if (length < 14 + 20) { > + /* Now check we have an IP header (with an optonnal VLAN header */ "optional", also missing ")". > + if (length < eth_get_l2_hdr_length(data) + sizeof(struct ip_header)) { > return; > } > > - if ((data[14] & 0xf0) != 0x40) > + ip = PKT_GET_IP_HDR(data); Are we definitely guaranteed that the input data buffer is aligned? It seems unlikely, and if it isn't then a lot of this code (both in macros like PKT_GET_IP_HDR, and open coded stuff below) is going to go wrong if it tries to just access 16 bit struct fields and they're not aligned and we're on a host that requires strict alignment. > + > + if (IP_HEADER_VERSION(ip) != IP_HEADER_VERSION_4) { > return; /* not IPv4 */ > - hlen = (data[14] & 0x0f) * 4; > - plen = (data[16] << 8 | data[17]) - hlen; > - proto = data[23]; > + } > + > + /* Last, check that we have enough data for the IP frame */ > + if (length < eth_get_l2_hdr_length(data) + be16_to_cpu(ip->ip_len)) { > + return; > + } > + > + plen = be16_to_cpu(ip->ip_len) - IP_HDR_GET_LEN(ip); > + > + switch (ip->ip_p) { > + case IP_PROTO_TCP: > + { > + uint16_t csum; > + tcp_header *tcp = (tcp_header *)(ip + 1); > + > + if (plen < sizeof(tcp_header)) { > + return; > + } I think this indent style is indenting to much. switch statements usually look like: switch (whatever) { case FOO: { code here; } case BAR: { more code; } default: whatever; } > > - switch (proto) { > - case PROTO_TCP: > - csum_offset = 16; > + tcp->th_sum = 0; > + > + csum = net_checksum_tcpudp(plen, ip->ip_p, > + (uint8_t *)&ip->ip_src, > + (uint8_t *)tcp); > + > + tcp->th_sum = cpu_to_be16(csum); > + } > break; > - case PROTO_UDP: > - csum_offset = 6; > + case IP_PROTO_UDP: > + { > + uint16_t csum; > + udp_header *udp = (udp_header *)(ip + 1); > + > + if (plen < sizeof(udp_header)) { > + return; > + } > + > + udp->uh_sum = 0; > + > + csum = net_checksum_tcpudp(plen, ip->ip_p, > + (uint8_t *)&ip->ip_src, > + (uint8_t *)udp); > + > + udp->uh_sum = cpu_to_be16(csum); > + } > break; > default: > + /* Can't handle any other protocol */ > return; > } > - > - if (plen < csum_offset + 2 || 14 + hlen + plen > length) { > - return; > - } > - > - data[14+hlen+csum_offset] = 0; > - data[14+hlen+csum_offset+1] = 0; > - csum = net_checksum_tcpudp(plen, proto, data+14+12, data+14+hlen); > - data[14+hlen+csum_offset] = csum >> 8; > - data[14+hlen+csum_offset+1] = csum & 0xff; > } > > uint32_t > -- > 2.7.4 thanks -- PMM