On 27/07/2011 12:49, Jan Kiszka wrote: > On 2011-07-26 18:21, 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 <chout...@adacore.com> >> --- >> Makefile.objs | 2 +- >> slirp/arp_table.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++ >> slirp/bootp.c | 15 ++++++----- >> slirp/slirp.c | 68 >> ++++++++++++++++------------------------------------ >> slirp/slirp.h | 51 +++++++++++++++++++++++++++++++++++++-- >> 5 files changed, 139 insertions(+), 58 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 += qemu-timer.o qemu-timer-common.o >> >> slirp-obj-y = cksum.o if.o ip_icmp.o ip_input.o ip_output.o >> slirp-obj-y += slirp.o mbuf.o misc.o sbuf.o socket.o tcp_input.o >> tcp_output.o >> -slirp-obj-y += tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o >> +slirp-obj-y += tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o arp_table.o >> common-obj-$(CONFIG_SLIRP) += $(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..e3251ed >> --- /dev/null >> +++ b/slirp/arp_table.c >> @@ -0,0 +1,61 @@ >> +#include "slirp.h" >> + >> +void arp_table_flush(ArpTable *arptbl) >> +{ >> + int i; >> + >> + assert(arptbl != NULL); >> + >> + for (i = 0; i < ARP_TABLE_SIZE; i++) { >> + arptbl->table[i].ar_sip = 0; >> + } >> + arptbl->next_victim = 0; >> +} > > arp_table_flush is only used for initialization, but the memory it > touches is already zero-initialized (on alloc in slirp_init). So you can > save this service.
OK, it was just in case slirp can be re-initialized. >> +{ >> + int i; >> + >> + DEBUG_CALL("arp_table_add"); >> + DEBUG_ARG("ip = 0x%x", ahdr->ar_sip); >> + DEBUG_ARGS((dfd, " hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n", >> + ahdr->ar_sha[0], ahdr->ar_sha[1], ahdr->ar_sha[2], >> + ahdr->ar_sha[3], ahdr->ar_sha[4], ahdr->ar_sha[5])); >> + >> + /* Search for an entry */ >> + for (i = 0; i < ARP_TABLE_SIZE && arptbl->table[i].ar_sip != 0; i++) { >> + if (arptbl->table[i].ar_sip == ahdr->ar_sip) { >> + /* Update the entry */ >> + memcpy(arptbl->table[i].ar_sha, ahdr->ar_sha, ETH_ALEN); >> + return; >> + } >> + } >> + >> + /* No entry found, create a new one */ >> + arptbl->table[arptbl->next_victim].ar_sip = ahdr->ar_sip; >> + memcpy(arptbl->table[arptbl->next_victim].ar_sha, ahdr->ar_sha, >> ETH_ALEN); >> + arptbl->next_victim = (arptbl->next_victim + 1) % ARP_TABLE_SIZE; > > Pragmatic aging. But I guess that's OK, we shouldn't need anything > smarter here. Yes, simple round-robin. >> diff --git a/slirp/bootp.c b/slirp/bootp.c >> index 1eb2ed1..ccca93b 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); >> @@ -165,7 +166,7 @@ static void bootp_reply(Slirp *slirp, const struct >> bootp_t *bp) >> dhcp_msg_type != DHCPREQUEST) >> return; >> /* XXX: this is a hack to get the client mac address */ >> - memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6); >> + memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN); > > Makes me wonder if the comment above still applies. Actually I don't think it is a hack at all. Makes me think I should update the ARP table here, with the IP address assigned to this client. Thanks for your review, I will fix the others style problems. Regards, -- Fabien Chouteau