From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48562) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a7O4d-0001Am-PS for qemu-devel@nongnu.org; Fri, 11 Dec 2015 08:49:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a7O4Z-0002x7-9k for qemu-devel@nongnu.org; Fri, 11 Dec 2015 08:49:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47371) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a7O4Z-0002x3-2E for qemu-devel@nongnu.org; Fri, 11 Dec 2015 08:49:23 -0500 References: <20151211001505.GV2905@var.home> <1449792930-27293-1-git-send-email-samuel.thibault@ens-lyon.org> <1449792930-27293-2-git-send-email-samuel.thibault@ens-lyon.org> <566AD1E4.1050403@redhat.com> <566AD30E.2060002@redhat.com> From: Thomas Huth Message-ID: <566AD45C.20000@redhat.com> Date: Fri, 11 Dec 2015 14:49:16 +0100 MIME-Version: 1.0 In-Reply-To: <566AD30E.2060002@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff 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 11/12/15 14:43, Thomas Huth wrote: > On 11/12/15 14:38, Thomas Huth wrote: >> On 11/12/15 01:15, Samuel Thibault wrote: >>> From: Guillaume Subiron >>> >>> Basically, this patch replaces "arp" by "resolution" every time "arp" >>> means "mac resolution" and not specifically ARP. >>> >>> Some indentation problems are solved in functions that will be modifi= ed >>> in the next patches (ip_input=E2=80=A6). >>> >>> In if_encap, a switch is added to prepare for the IPv6 case. Some cod= e >>> is factorized. >>> >>> Signed-off-by: Guillaume Subiron >>> Signed-off-by: Samuel Thibault >>> --- >>> slirp/mbuf.c | 2 +- >>> slirp/mbuf.h | 2 +- >>> slirp/slirp.c | 24 ++++++++++++++++++++---- >>> 3 files changed, 22 insertions(+), 6 deletions(-) >>> >>> diff --git a/slirp/mbuf.c b/slirp/mbuf.c >>> index 795fc29..bc942b6 100644 >>> --- a/slirp/mbuf.c >>> +++ b/slirp/mbuf.c >>> @@ -91,7 +91,7 @@ m_get(Slirp *slirp) >>> m->m_len =3D 0; >>> m->m_nextpkt =3D NULL; >>> m->m_prevpkt =3D NULL; >>> - m->arp_requested =3D false; >>> + m->resolution_requested =3D false; >>> m->expiration_date =3D (uint64_t)-1; >>> end_error: >>> DEBUG_ARG("m =3D %p", m); >>> diff --git a/slirp/mbuf.h b/slirp/mbuf.h >>> index b144f1c..38fedf4 100644 >>> --- a/slirp/mbuf.h >>> +++ b/slirp/mbuf.h >>> @@ -79,7 +79,7 @@ struct mbuf { >>> int m_len; /* Amount of data in this mbuf */ >>> =20 >>> Slirp *slirp; >>> - bool arp_requested; >>> + bool resolution_requested; >>> uint64_t expiration_date; >>> /* start of dynamic buffer area, must be last element */ >>> union { >>> diff --git a/slirp/slirp.c b/slirp/slirp.c >>> index 35f819a..380ddc3 100644 >>> --- a/slirp/slirp.c >>> +++ b/slirp/slirp.c >>> @@ -776,6 +776,8 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >>> return 1; >>> } >>> =20 >>> + switch (iph->ip_v) { >>> + case IPVERSION: >>> if (iph->ip_dst.s_addr =3D=3D 0) { >>> /* 0.0.0.0 can not be a destination address, something went = wrong, >>> * avoid making it worse */ >> >> That indentation looks now broken - shouldn't the if-statement now be >> indented with four more spaces now? >> >>> @@ -786,7 +788,7 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >>> struct ethhdr *reh =3D (struct ethhdr *)arp_req; >>> struct arphdr *rah =3D (struct arphdr *)(arp_req + ETH_HLEN)= ; >>> =20 >>> - if (!ifm->arp_requested) { >>> + if (!ifm->resolution_requested) { >>> /* If the client addr is not known, send an ARP request = */ >>> memset(reh->h_dest, 0xff, ETH_ALEN); >>> memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4); >>> @@ -812,22 +814,36 @@ int if_encap(Slirp *slirp, struct mbuf *ifm) >>> rah->ar_tip =3D iph->ip_dst.s_addr; >>> slirp->client_ipaddr =3D iph->ip_dst; >>> slirp_output(slirp->opaque, arp_req, sizeof(arp_req)); >>> - ifm->arp_requested =3D true; >>> + ifm->resolution_requested =3D true; >>> =20 >>> /* Expire request and drop outgoing packet after 1 secon= d */ >>> ifm->expiration_date =3D qemu_clock_get_ns(QEMU_CLOCK_RE= ALTIME) + 1000000000ULL; >>> } >>> return 0; >>> } else { >>> - memcpy(eh->h_dest, ethaddr, ETH_ALEN); >>> memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4); >>> /* XXX: not correct */ >>> memcpy(&eh->h_source[2], &slirp->vhost_addr, 4); >>> eh->h_proto =3D htons(ETH_P_IP); >>> + break; >>> + } >>> + >>> + default: >>> + /* Do not assert while we don't manage IP6VERSION */ >>> + /* assert(0); */ >>> + break; >>> + } >>> + >>> + memcpy(eh->h_dest, ethaddr, ETH_ALEN); >>> + DEBUG_ARGS((dfd, " src =3D %02x:%02x:%02x:%02x:%02x:%02x\n", >>> + eh->h_source[0], eh->h_source[1], eh->h_source[2= ], >>> + eh->h_source[3], eh->h_source[4], eh->h_source[5= ])); >>> + DEBUG_ARGS((dfd, " dst =3D %02x:%02x:%02x:%02x:%02x:%02x\n", >>> + eh->h_dest[0], eh->h_dest[1], eh->h_dest[2], >>> + eh->h_dest[3], eh->h_dest[4], eh->h_dest[5])); >>> memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len)= ; >>> slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN); >>> return 1; >>> - } >>> } >> >> The lines after the switch statement should also only be indented with= 4 >> spaces, not with 8. >=20 > Ah, well, never mind, I did not see the comment in the patch descriptio= n > that you fix it up in the next patch. >=20 > Anyway, it's IMHO a somewhat strange way to structure a patch ... maybe > it would be nicer to do it in one go instead (after splitting off the > arp_requested renaming)? Or maybe even move the IPv4-only code into a separate function instead? ... otherwise if_encap() will be very huge once the IPv6 code has been added, I think. Thomas