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

[Fabien Chouteau <chouteau@adacore.com>]

On 01/08/2011 22:16, Jan Kiszka wrote:
> On 2011-08-01 18:18, 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 <chouteau@adacore.com>
>> ---
>>  Makefile.objs     |    2 +-
>>  slirp/arp_table.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  slirp/bootp.c     |   21 +++++++++------
>>  slirp/slirp.c     |   63 +++++++++++----------------------------------
>>  slirp/slirp.h     |   47 ++++++++++++++++++++++++++++++++--
>>  5 files changed, 146 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..c7034ee
>> --- /dev/null
>> +++ b/slirp/arp_table.c
>> @@ -0,0 +1,72 @@
>> +#include "slirp.h"
>> +
>> +void arp_table_add(Slirp *slirp, int ip_addr, uint8_t ethaddr[ETH_ALEN])
>> +{
>> +    const in_addr_t broadcast_addr =
>> +        ~slirp->vnetwork_mask.s_addr | slirp->vnetwork_addr.s_addr;
> 
> That's only part of the picture. 255.255.255.255 is a valid broadcast
> address as well.

Of course, how can I have missed this...

>> +    ArpTable *arptbl = &slirp->arp_table;
>> +    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]));
>> +
>> +    if ((ip_addr & ~(0xf << 28)) == 0 ||
>> +        ip_addr == broadcast_addr) {
>> +        /* Do not register 0.0.0.0/8 or broadcast addresses */
>> +        return;
>> +    }
>> +
>> +    /* Search for an entry */
>> +    for (i = 0; i < ARP_TABLE_SIZE && arptbl->table[i].ar_sip != 0; i++) {
> 
> Forgot to remove the test for ar_sip != 0.
> 
>> +        if (arptbl->table[i].ar_sip == 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 = ip_addr;
>> +    memcpy(arptbl->table[arptbl->next_victim].ar_sha,  ethaddr, ETH_ALEN);
>> +    arptbl->next_victim = (arptbl->next_victim + 1) % ARP_TABLE_SIZE;
>> +}
>> +
>> +bool arp_table_search(Slirp *slirp, int in_ip_addr,
>> +                      uint8_t out_ethaddr[ETH_ALEN])
>> +{
>> +    const in_addr_t broadcast_addr =
>> +        ~slirp->vnetwork_mask.s_addr | slirp->vnetwork_addr.s_addr;
> 
> Same as above. That means DCHP is still broken. Please include that
> scenario in your tests before sending the next round.

OK I will test with DHCP.

Thanks again,

-- 
Fabien Chouteau