From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35008) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qn5hM-00039G-Rd for qemu-devel@nongnu.org; Sat, 30 Jul 2011 05:19:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qn5hL-0002d9-AL for qemu-devel@nongnu.org; Sat, 30 Jul 2011 05:19:08 -0400 Received: from fmmailgate02.web.de ([217.72.192.227]:45932) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qn5hK-0002d5-Pw for qemu-devel@nongnu.org; Sat, 30 Jul 2011 05:19:07 -0400 Message-ID: <4E33CC89.502@web.de> Date: Sat, 30 Jul 2011 11:19:05 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1311956703-24788-1-git-send-email-chouteau@adacore.com> <4E32EF27.2060406@siemens.com> <4E33CA59.7070607@web.de> In-Reply-To: <4E33CA59.7070607@web.de> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig31F539C5FD552227C63CFF45" Sender: jan.kiszka@web.de Subject: Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau , "qemu-devel@nongnu.org" This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig31F539C5FD552227C63CFF45 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2011-07-30 11:09, Jan Kiszka wrote: > On 2011-07-29 19:34, Jan Kiszka wrote: >> On 2011-07-29 18:25, Fabien Chouteau wrote: >>> This patch adds a simple ARP table in Slirp and also adds handling of= >>> gratuitous ARP requests. >>> >>> Signed-off-by: Fabien Chouteau >>> --- >>> Makefile.objs | 2 +- >>> slirp/arp_table.c | 50 ++++++++++++++++++++++++++++++++++++++++++ >>> slirp/bootp.c | 23 ++++++++++++------ >>> slirp/slirp.c | 63 +++++++++++++------------------------------= --------- >>> slirp/slirp.h | 50 +++++++++++++++++++++++++++++++++++++++-- >>> 5 files changed, 129 insertions(+), 59 deletions(-) >>> create mode 100644 slirp/arp_table.c >>> >>> diff --git a/Makefile.objs b/Makefile.objs >>> index 6991a9f..0c10557 100644 >>> --- a/Makefile.objs >>> +++ b/Makefile.objs >>> @@ -151,7 +151,7 @@ common-obj-y +=3D qemu-timer.o qemu-timer-common.= o >>> >>> slirp-obj-y =3D cksum.o if.o ip_icmp.o ip_input.o ip_output.o >>> slirp-obj-y +=3D slirp.o mbuf.o misc.o sbuf.o socket.o tcp_input.o t= cp_output.o >>> -slirp-obj-y +=3D tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o >>> +slirp-obj-y +=3D tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o arp_tab= le.o >>> common-obj-$(CONFIG_SLIRP) +=3D $(addprefix slirp/, $(slirp-obj-y)) >>> >>> # xen backend driver support >>> diff --git a/slirp/arp_table.c b/slirp/arp_table.c >>> new file mode 100644 >>> index 0000000..5d7404b >>> --- /dev/null >>> +++ b/slirp/arp_table.c >>> @@ -0,0 +1,50 @@ >>> +#include "slirp.h" >>> + >>> +void arp_table_add(ArpTable *arptbl, >>> + int ip_addr, >>> + uint8_t ethaddr[ETH_ALEN]) >> >> I still prefer the condensed formatting, but that's a minor nit. >> >>> +{ >>> + int i; >>> + >>> + DEBUG_CALL("arp_table_add"); >>> + DEBUG_ARG("ip =3D 0x%x", ip_addr); >>> + DEBUG_ARGS((dfd, " hw addr =3D %02x:%02x:%02x:%02x:%02x:%02x\n",= >>> + ethaddr[0], ethaddr[1], ethaddr[2], >>> + ethaddr[3], ethaddr[4], ethaddr[5])); >>> + >>> + /* Search for an entry */ >>> + for (i =3D 0; i < ARP_TABLE_SIZE && arptbl->table[i].ar_sip !=3D= 0; i++) { >> >> Missed that on round #1: Why treating 0.0.0.0 special? If ip_addr can = be >> zero, the current logic will append every "update" of that address as = a >> new entry. >> >>> + if (arptbl->table[i].ar_sip =3D=3D ip_addr) { >>> + /* Update the entry */ >>> + memcpy(arptbl->table[i].ar_sha, ethaddr, ETH_ALEN); >>> + return; >>> + } >>> + } >>> + >>> + /* No entry found, create a new one */ >>> + arptbl->table[arptbl->next_victim].ar_sip =3D ip_addr; >>> + memcpy(arptbl->table[arptbl->next_victim].ar_sha, ethaddr, ETH_= ALEN); >>> + arptbl->next_victim =3D (arptbl->next_victim + 1) % ARP_TABLE_SI= ZE; >>> +} >>> + >>> +bool arp_table_search(ArpTable *arptbl, >>> + int in_ip_addr, >>> + uint8_t out_ethaddr[ETH_ALEN]) >>> +{ >>> + int i; >>> + >>> + DEBUG_CALL("arp_table_search"); >>> + DEBUG_ARG("ip =3D 0x%x", in_ip_addr); >>> + >>> + for (i =3D 0; i < ARP_TABLE_SIZE; i++) { >>> + if (arptbl->table[i].ar_sip =3D=3D in_ip_addr) { >>> + memcpy(out_ethaddr, arptbl->table[i].ar_sha, ETH_ALEN);= >>> + DEBUG_ARGS((dfd, " found hw addr =3D %02x:%02x:%02x:%02x= :%02x:%02x\n", >>> + out_ethaddr[0], out_ethaddr[1], out_ethaddr[= 2], >>> + out_ethaddr[3], out_ethaddr[4], out_ethaddr[= 5])); >>> + return 1; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> diff --git a/slirp/bootp.c b/slirp/bootp.c >>> index 1eb2ed1..07cbb80 100644 >>> --- a/slirp/bootp.c >>> +++ b/slirp/bootp.c >>> @@ -149,6 +149,7 @@ static void bootp_reply(Slirp *slirp, const struc= t bootp_t *bp) >>> struct in_addr preq_addr; >>> int dhcp_msg_type, val; >>> uint8_t *q; >>> + uint8_t client_ethaddr[ETH_ALEN]; >>> >>> /* extract exact DHCP msg type */ >>> dhcp_decode(bp, &dhcp_msg_type, &preq_addr); >>> @@ -164,8 +165,9 @@ static void bootp_reply(Slirp *slirp, const struc= t bootp_t *bp) >>> if (dhcp_msg_type !=3D DHCPDISCOVER && >>> dhcp_msg_type !=3D DHCPREQUEST) >>> return; >>> - /* XXX: this is a hack to get the client mac address */ >>> - memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6); >>> + >>> + /* Get client's hardware address from bootp request */ >>> + memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN); >>> >>> m =3D m_get(slirp); >>> if (!m) { >>> @@ -178,25 +180,25 @@ static void bootp_reply(Slirp *slirp, const str= uct bootp_t *bp) >>> >>> if (dhcp_msg_type =3D=3D DHCPDISCOVER) { >>> if (preq_addr.s_addr !=3D htonl(0L)) { >>> - bc =3D request_addr(slirp, &preq_addr, slirp->client_eth= addr); >>> + bc =3D request_addr(slirp, &preq_addr, client_ethaddr); >>> if (bc) { >>> daddr.sin_addr =3D preq_addr; >>> } >>> } >>> if (!bc) { >>> new_addr: >>> - bc =3D get_new_addr(slirp, &daddr.sin_addr, slirp->clien= t_ethaddr); >>> + bc =3D get_new_addr(slirp, &daddr.sin_addr, client_ethad= dr); >>> if (!bc) { >>> DPRINTF("no address left\n"); >>> return; >>> } >>> } >>> - memcpy(bc->macaddr, slirp->client_ethaddr, 6); >>> + memcpy(bc->macaddr, client_ethaddr, ETH_ALEN); >>> } else if (preq_addr.s_addr !=3D htonl(0L)) { >>> - bc =3D request_addr(slirp, &preq_addr, slirp->client_ethaddr= ); >>> + bc =3D request_addr(slirp, &preq_addr, client_ethaddr); >>> if (bc) { >>> daddr.sin_addr =3D preq_addr; >>> - memcpy(bc->macaddr, slirp->client_ethaddr, 6); >>> + memcpy(bc->macaddr, client_ethaddr, ETH_ALEN); >>> } else { >>> daddr.sin_addr.s_addr =3D 0; >>> } >>> @@ -209,6 +211,11 @@ static void bootp_reply(Slirp *slirp, const stru= ct bootp_t *bp) >>> } >>> } >>> >>> + if (daddr.sin_addr.s_addr !=3D 0) { >>> + /* Update ARP table for this IP address */ >>> + arp_table_add(&slirp->arp_table, daddr.sin_addr.s_addr, clie= nt_ethaddr); >> >> Here you prevent that arp_table_add is called with a zero IP, but not = in >> arp_input below. Likely harmless, but at least inconsistent. >> >=20 > Unfortunately, this patch also has a more sever issues: it breaks DHCP = e.g. >=20 > Slirp's bootp sends out all replies, also acks and offers, as > broadcasts. Normal servers already use the clients IP address as > destination here. Obviously, using a broadcast address on return is valid as well. So this is no slirp bug, it's purely an ARP table lookup issue introduced by this patch. >=20 > Even if bootp is fixed, you still lack logic to deal with special > addresses in your arp table lookup. At least broadcasts need to be > handled, I think other multicasts aren't supported by slirp anyway. >=20 Jan --------------enig31F539C5FD552227C63CFF45 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk4zzIkACgkQitSsb3rl5xRh5wCcCkCykyBVZe890Gq6oNL4mgTE XnwAoKXCxH2Y6Ux48DyyBEEfHx/rHboy =5hQ6 -----END PGP SIGNATURE----- --------------enig31F539C5FD552227C63CFF45--