From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60629) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWlN3-00059r-G4 for qemu-devel@nongnu.org; Fri, 19 Feb 2016 08:45:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWlMy-00053a-PO for qemu-devel@nongnu.org; Fri, 19 Feb 2016 08:45:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56671) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWlMy-00053S-Hr for qemu-devel@nongnu.org; Fri, 19 Feb 2016 08:45:16 -0500 References: From: Thomas Huth Message-ID: <56C71C5B.1040805@redhat.com> Date: Fri, 19 Feb 2016 14:44:59 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCHv7 4/9] slirp: Factorizing tcpiphdr structure with an union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Samuel Thibault , qemu-devel@nongnu.org Cc: zhanghailiang , Li Zhijian , Stefan Hajnoczi , Jason Wang , Dave Gilbert , Vasiliy Tolstov , Huangpeng , Gonglei , Jan Kiszka , Guillaume Subiron On 14.02.2016 18:47, Samuel Thibault wrote: > From: Guillaume Subiron >=20 > This patch factorizes the tcpiphdr structure to put the IPv4 fields in > an union, for addition of version 6 in further patch. > Using some macros, retrocompatibility of the existing code is assured. >=20 > This patch also fixes the SLIRP_MSIZE and margin computation in various > functions, and makes them compatible with the new tcpiphdr structure, > whose size will be bigger than sizeof(struct tcphdr) + sizeof(struct ip= ) >=20 > Signed-off-by: Guillaume Subiron > Signed-off-by: Samuel Thibault > --- > slirp/if.h | 4 ++-- > slirp/mbuf.c | 3 ++- > slirp/slirp.c | 15 ++++++++------- > slirp/socket.c | 13 ++++++++++++- > slirp/tcp_input.c | 31 ++++++++++++++++++++----------- > slirp/tcp_output.c | 18 +++++++++++++----- > slirp/tcp_subr.c | 31 ++++++++++++++++++++++--------- > slirp/tcpip.h | 31 +++++++++++++++++++++++-------- > 8 files changed, 102 insertions(+), 44 deletions(-) >=20 > diff --git a/slirp/if.h b/slirp/if.h > index 3327023..c7a5c57 100644 > --- a/slirp/if.h > +++ b/slirp/if.h > @@ -17,7 +17,7 @@ > #define IF_MRU 1500 > #define IF_COMP IF_AUTOCOMP /* Flags for compression */ > =20 > -/* 2 for alignment, 14 for ethernet, 40 for TCP/IP */ > -#define IF_MAXLINKHDR (2 + 14 + 40) > +/* 2 for alignment, 14 for ethernet */ > +#define IF_MAXLINKHDR (2 + ETH_HLEN) > =20 > #endif > diff --git a/slirp/mbuf.c b/slirp/mbuf.c > index c959758..f081c69 100644 > --- a/slirp/mbuf.c > +++ b/slirp/mbuf.c > @@ -24,7 +24,8 @@ > * Find a nice value for msize > * XXX if_maxlinkhdr already in mtu Maybe you should now remove the XXX line, now that the size of the TCP/IP headers is counted via IF_MTU instead? > */ > -#define SLIRP_MSIZE (IF_MTU + IF_MAXLINKHDR + offsetof(struct mbuf, m_= dat) + 6) > +#define SLIRP_MSIZE\ > + (offsetof(struct mbuf, m_dat) + IF_MAXLINKHDR + TCPIPHDR_DELTA + I= F_MTU) > =20 > void > m_init(Slirp *slirp) ... > diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c > index 5f845da..26b0c8b 100644 > --- a/slirp/tcp_input.c > +++ b/slirp/tcp_input.c > @@ -256,11 +256,6 @@ tcp_input(struct mbuf *m, int iphlen, struct socke= t *inso) > } > slirp =3D m->slirp; > =20 > - /* > - * Get IP and TCP header together in first mbuf. > - * Note: IP leaves IP header in first mbuf. > - */ > - ti =3D mtod(m, struct tcpiphdr *); > if (iphlen > sizeof(struct ip )) { > ip_stripoptions(m, (struct mbuf *)0); > iphlen=3Dsizeof(struct ip ); > @@ -277,14 +272,28 @@ tcp_input(struct mbuf *m, int iphlen, struct sock= et *inso) > save_ip.ip_len+=3D iphlen; > =20 > /* > + * Get IP and TCP header together in first mbuf. > + * Note: IP leaves IP header in first mbuf. > + */ > + m->m_data -=3D sizeof(struct tcpiphdr) - (sizeof(struct ip) > + + sizeof(struct tcphdr)); > + m->m_len +=3D sizeof(struct tcpiphdr) - (sizeof(struct ip) > + + sizeof(struct tcphdr)); I'm somewhat having a hard time to understand the "+ sizeof(struct tcphdr))" here. In the tcp_output.c code, there is this: m->m_data +=3D sizeof(struct tcpiphdr) - sizeof(struct tcphdr) - sizeof(struct ip); So with my limited point of view, I'd rather expect this here in tcp_input.c: m->m_data -=3D sizeof(struct tcpiphdr) - (sizeof(struct ip) - sizeof(struct tcphdr)); i.e. "-" instead of "+" here ----------------^ Could you maybe elaborate a little bit on the above calculation? Or is it just a bug? > + ti =3D mtod(m, struct tcpiphdr *); > + > + /* > * Checksum extended TCP header and data. > */ > - tlen =3D ((struct ip *)ti)->ip_len; > - tcpiphdr2qlink(ti)->next =3D tcpiphdr2qlink(ti)->prev =3D NULL= ; > - memset(&ti->ti_i.ih_mbuf, 0 , sizeof(struct mbuf_ptr)); > - ti->ti_x1 =3D 0; > + tlen =3D ip->ip_len; > + tcpiphdr2qlink(ti)->next =3D tcpiphdr2qlink(ti)->prev =3D NULL; > + memset(&ti->ih_mbuf, 0 , sizeof(struct mbuf_ptr)); > + memset(&ti->ti, 0, sizeof(ti->ti)); > + ti->ti_x0 =3D 0; > + ti->ti_src =3D save_ip.ip_src; > + ti->ti_dst =3D save_ip.ip_dst; > + ti->ti_pr =3D save_ip.ip_p; > ti->ti_len =3D htons((uint16_t)tlen); > - len =3D sizeof(struct ip ) + tlen; > + len =3D ((sizeof(struct tcpiphdr) - sizeof(struct tcphdr)) + tlen); > if(cksum(m, len)) { > goto drop; > } > @@ -1475,7 +1484,7 @@ tcp_mss(struct tcpcb *tp, u_int offer) > DEBUG_ARG("tp =3D %p", tp); > DEBUG_ARG("offer =3D %d", offer); > =20 > - mss =3D min(IF_MTU, IF_MRU) - sizeof(struct tcpiphdr); > + mss =3D min(IF_MTU, IF_MRU) - sizeof(struct tcphdr) + sizeof(struct i= p); > if (offer) > mss =3D min(mss, offer); > mss =3D max(mss, 32); > diff --git a/slirp/tcp_output.c b/slirp/tcp_output.c > index 34e4d2e..7fc6a87 100644 > --- a/slirp/tcp_output.c > +++ b/slirp/tcp_output.c > @@ -448,15 +448,23 @@ send: > */ > m->m_len =3D hdrlen + len; /* XXX Needed? m_len should be correct */ > =20 > - { > + struct tcpiphdr tcpiph_save =3D *(mtod(m, struct tcpiphdr *)); > + m->m_data +=3D sizeof(struct tcpiphdr) - sizeof(struct tcphdr) > + - sizeof(struct ip); > + m->m_len -=3D sizeof(struct tcpiphdr) - sizeof(struct tcphdr) > + - sizeof(struct ip); > + struct ip *ip =3D mtod(m, struct ip *); > =20 > - ((struct ip *)ti)->ip_len =3D m->m_len; > + ip->ip_len =3D m->m_len; > + ip->ip_dst =3D tcpiph_save.ti_dst; > + ip->ip_src =3D tcpiph_save.ti_src; > + ip->ip_p =3D tcpiph_save.ti_pr; > =20 > - ((struct ip *)ti)->ip_ttl =3D IPDEFTTL; > - ((struct ip *)ti)->ip_tos =3D so->so_iptos; > + ip->ip_ttl =3D IPDEFTTL; > + ip->ip_tos =3D so->so_iptos; > =20 > error =3D ip_output(so, m); > - } > + > if (error) { > out: > return (error); Thomas