Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table

[Fabien Chouteau]

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 <address@hidden>
>>>> ---
>>>>  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