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

[Fabien Chouteau <chouteau@adacore.com>]

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 <chouteau@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