From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:39702) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qnu2F-0000ee-IB for qemu-devel@nongnu.org; Mon, 01 Aug 2011 11:04:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qnu2E-00079O-1o for qemu-devel@nongnu.org; Mon, 01 Aug 2011 11:04:03 -0400 Received: from mel.act-europe.fr ([194.98.77.210]:35215) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qnu2D-00079D-LS for qemu-devel@nongnu.org; Mon, 01 Aug 2011 11:04:02 -0400 Message-ID: <4E36C055.2070407@adacore.com> Date: Mon, 01 Aug 2011 17:03:49 +0200 From: Fabien Chouteau MIME-Version: 1.0 References: <1311956703-24788-1-git-send-email-chouteau@adacore.com> <4E32EF27.2060406@siemens.com> <4E33CA59.7070607@web.de> <4E33CC89.502@web.de> In-Reply-To: <4E33CC89.502@web.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Jan Kiszka Cc: "qemu-devel@nongnu.org" On 30/07/2011 11:19, Jan Kiszka wrote: > 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 += 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..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. OK I'll change it to keep consistent style. Unfortunately, there's nothing on this subject in the CODING_STYLE... >>> >>>> +{ >>>> + int i; >>>> + >>>> + DEBUG_CALL("arp_table_add"); >>>> + DEBUG_ARG("ip = 0x%x", ip_addr); >>>> + DEBUG_ARGS((dfd, " hw addr = %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 = 0; i < ARP_TABLE_SIZE && arptbl->table[i].ar_sip != 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. Isn't 0.0.0.0 a reserved address? I think it's safe to use it here. >>>> diff --git a/slirp/bootp.c b/slirp/bootp.c >>>> index 1eb2ed1..07cbb80 100644 >>>> --- a/slirp/bootp.c >>>> +++ b/slirp/bootp.c >>>> @@ -209,6 +211,11 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp) >>>> } >>>> } >>>> >>>> + if (daddr.sin_addr.s_addr != 0) { >>>> + /* Update ARP table for this IP address */ >>>> + arp_table_add(&slirp->arp_table, daddr.sin_addr.s_addr, client_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. I will handle this case directly in arp_table_add to get a consistent behavior. >> >> 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. > > 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. > >> >> 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. >> > That's right, I will add broadcast handling in the next version. Thanks for your review. -- Fabien Chouteau