From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qn5YT-0001VW-NA for qemu-devel@nongnu.org; Sat, 30 Jul 2011 05:09:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qn5YP-0000pI-GJ for qemu-devel@nongnu.org; Sat, 30 Jul 2011 05:09:57 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]:53441) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qn5YO-0000ox-Vd for qemu-devel@nongnu.org; Sat, 30 Jul 2011 05:09:53 -0400 Message-ID: <4E33CA59.7070607@web.de> Date: Sat, 30 Jul 2011 11:09:45 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1311956703-24788-1-git-send-email-chouteau@adacore.com> <4E32EF27.2060406@siemens.com> In-Reply-To: <4E32EF27.2060406@siemens.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig954C18299FB803D80C2DD08F" 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: , Cc: "qemu-devel@nongnu.org" , Fabien Chouteau This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig954C18299FB803D80C2DD08F Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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 tc= p_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_tabl= e.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]) >=20 > I still prefer the condensed formatting, but that's a minor nit. >=20 >> +{ >> + 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++) { >=20 > Missed that on round #1: Why treating 0.0.0.0 special? If ip_addr can b= e > zero, the current logic will append every "update" of that address as a= > new entry. >=20 >> + 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_A= LEN); >> + arptbl->next_victim =3D (arptbl->next_victim + 1) % ARP_TABLE_SIZ= E; >> +} >> + >> +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 struct= 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 struct= 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 stru= ct 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_etha= ddr); >> + 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->client= _ethaddr); >> + bc =3D get_new_addr(slirp, &daddr.sin_addr, client_ethadd= r); >> 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 struc= t 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, clien= t_ethaddr); >=20 > Here you prevent that arp_table_add is called with a zero IP, but not i= n > arp_input below. Likely harmless, but at least inconsistent. >=20 Unfortunately, this patch also has a more sever issues: it breaks DHCP e.= g. Slirp's bootp sends out all replies, also acks and offers, as broadcasts. Normal servers already use the clients IP address as destination here. 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. Jan --------------enig954C18299FB803D80C2DD08F 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/ iEYEARECAAYFAk4zyl0ACgkQitSsb3rl5xTxUgCgpqgE2xyZTXjvqLoO/jGbxV/i lF8AnjCHIN3sBfZKoiAdbzZ7zKGQRpYj =3JhB -----END PGP SIGNATURE----- --------------enig954C18299FB803D80C2DD08F--