Patchwork [V2,1/2,SLIRP] Simple ARP table

login
register
mail settings
Submitter Fabien Chouteau
Date July 29, 2011, 4:25 p.m.
Message ID <1311956703-24788-1-git-send-email-chouteau@adacore.com>
Download mbox | patch
Permalink /patch/107422/
State New
Headers show

Comments

Fabien Chouteau - July 29, 2011, 4:25 p.m.
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 |   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
Jan Kiszka - July 29, 2011, 5:34 p.m.
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 <chouteau@adacore.com>
> ---
>  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.

> +{
> +    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.

> +        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(ArpTable *arptbl,
> +                      int       in_ip_addr,
> +                      uint8_t   out_ethaddr[ETH_ALEN])
> +{
> +    int i;
> +
> +    DEBUG_CALL("arp_table_search");
> +    DEBUG_ARG("ip = 0x%x", in_ip_addr);
> +
> +    for (i = 0; i < ARP_TABLE_SIZE; i++) {
> +        if (arptbl->table[i].ar_sip == in_ip_addr) {
> +            memcpy(out_ethaddr, arptbl->table[i].ar_sha,  ETH_ALEN);
> +            DEBUG_ARGS((dfd, " found hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n",
> +                        out_ethaddr[0], out_ethaddr[1], out_ethaddr[2],
> +                        out_ethaddr[3], out_ethaddr[4], out_ethaddr[5]));
> +            return 1;
> +        }
> +    }
> +
> +    return 0;
> +}
> diff --git a/slirp/bootp.c b/slirp/bootp.c
> index 1eb2ed1..07cbb80 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);
> @@ -164,8 +165,9 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
>      if (dhcp_msg_type != DHCPDISCOVER &&
>          dhcp_msg_type != DHCPREQUEST)
>          return;
> -    /* XXX: this is a hack to get the client mac address */
> -    memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6);
> +
> +    /* Get client's hardware address from bootp request */
> +    memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN);
> 
>      m = m_get(slirp);
>      if (!m) {
> @@ -178,25 +180,25 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
> 
>      if (dhcp_msg_type == DHCPDISCOVER) {
>          if (preq_addr.s_addr != htonl(0L)) {
> -            bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
> +            bc = request_addr(slirp, &preq_addr, client_ethaddr);
>              if (bc) {
>                  daddr.sin_addr = preq_addr;
>              }
>          }
>          if (!bc) {
>           new_addr:
> -            bc = get_new_addr(slirp, &daddr.sin_addr, slirp->client_ethaddr);
> +            bc = get_new_addr(slirp, &daddr.sin_addr, client_ethaddr);
>              if (!bc) {
>                  DPRINTF("no address left\n");
>                  return;
>              }
>          }
> -        memcpy(bc->macaddr, slirp->client_ethaddr, 6);
> +        memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>      } else if (preq_addr.s_addr != htonl(0L)) {
> -        bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
> +        bc = request_addr(slirp, &preq_addr, client_ethaddr);
>          if (bc) {
>              daddr.sin_addr = preq_addr;
> -            memcpy(bc->macaddr, slirp->client_ethaddr, 6);
> +            memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>          } else {
>              daddr.sin_addr.s_addr = 0;
>          }
> @@ -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.

Jan
Jan Kiszka - July 30, 2011, 9:09 a.m.
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 <chouteau@adacore.com>
>> ---
>>  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.
> 
>> +{
>> +    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.
> 
>> +        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(ArpTable *arptbl,
>> +                      int       in_ip_addr,
>> +                      uint8_t   out_ethaddr[ETH_ALEN])
>> +{
>> +    int i;
>> +
>> +    DEBUG_CALL("arp_table_search");
>> +    DEBUG_ARG("ip = 0x%x", in_ip_addr);
>> +
>> +    for (i = 0; i < ARP_TABLE_SIZE; i++) {
>> +        if (arptbl->table[i].ar_sip == in_ip_addr) {
>> +            memcpy(out_ethaddr, arptbl->table[i].ar_sha,  ETH_ALEN);
>> +            DEBUG_ARGS((dfd, " found hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n",
>> +                        out_ethaddr[0], out_ethaddr[1], out_ethaddr[2],
>> +                        out_ethaddr[3], out_ethaddr[4], out_ethaddr[5]));
>> +            return 1;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/slirp/bootp.c b/slirp/bootp.c
>> index 1eb2ed1..07cbb80 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);
>> @@ -164,8 +165,9 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
>>      if (dhcp_msg_type != DHCPDISCOVER &&
>>          dhcp_msg_type != DHCPREQUEST)
>>          return;
>> -    /* XXX: this is a hack to get the client mac address */
>> -    memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6);
>> +
>> +    /* Get client's hardware address from bootp request */
>> +    memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN);
>>
>>      m = m_get(slirp);
>>      if (!m) {
>> @@ -178,25 +180,25 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
>>
>>      if (dhcp_msg_type == DHCPDISCOVER) {
>>          if (preq_addr.s_addr != htonl(0L)) {
>> -            bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
>> +            bc = request_addr(slirp, &preq_addr, client_ethaddr);
>>              if (bc) {
>>                  daddr.sin_addr = preq_addr;
>>              }
>>          }
>>          if (!bc) {
>>           new_addr:
>> -            bc = get_new_addr(slirp, &daddr.sin_addr, slirp->client_ethaddr);
>> +            bc = get_new_addr(slirp, &daddr.sin_addr, client_ethaddr);
>>              if (!bc) {
>>                  DPRINTF("no address left\n");
>>                  return;
>>              }
>>          }
>> -        memcpy(bc->macaddr, slirp->client_ethaddr, 6);
>> +        memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>>      } else if (preq_addr.s_addr != htonl(0L)) {
>> -        bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
>> +        bc = request_addr(slirp, &preq_addr, client_ethaddr);
>>          if (bc) {
>>              daddr.sin_addr = preq_addr;
>> -            memcpy(bc->macaddr, slirp->client_ethaddr, 6);
>> +            memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>>          } else {
>>              daddr.sin_addr.s_addr = 0;
>>          }
>> @@ -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.
> 

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.

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.

Jan
Jan Kiszka - July 30, 2011, 9:19 a.m.
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 <chouteau@adacore.com>
>>> ---
>>>  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.
>>
>>> +{
>>> +    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.
>>
>>> +        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(ArpTable *arptbl,
>>> +                      int       in_ip_addr,
>>> +                      uint8_t   out_ethaddr[ETH_ALEN])
>>> +{
>>> +    int i;
>>> +
>>> +    DEBUG_CALL("arp_table_search");
>>> +    DEBUG_ARG("ip = 0x%x", in_ip_addr);
>>> +
>>> +    for (i = 0; i < ARP_TABLE_SIZE; i++) {
>>> +        if (arptbl->table[i].ar_sip == in_ip_addr) {
>>> +            memcpy(out_ethaddr, arptbl->table[i].ar_sha,  ETH_ALEN);
>>> +            DEBUG_ARGS((dfd, " found hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> +                        out_ethaddr[0], out_ethaddr[1], out_ethaddr[2],
>>> +                        out_ethaddr[3], out_ethaddr[4], out_ethaddr[5]));
>>> +            return 1;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> diff --git a/slirp/bootp.c b/slirp/bootp.c
>>> index 1eb2ed1..07cbb80 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);
>>> @@ -164,8 +165,9 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
>>>      if (dhcp_msg_type != DHCPDISCOVER &&
>>>          dhcp_msg_type != DHCPREQUEST)
>>>          return;
>>> -    /* XXX: this is a hack to get the client mac address */
>>> -    memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6);
>>> +
>>> +    /* Get client's hardware address from bootp request */
>>> +    memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN);
>>>
>>>      m = m_get(slirp);
>>>      if (!m) {
>>> @@ -178,25 +180,25 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
>>>
>>>      if (dhcp_msg_type == DHCPDISCOVER) {
>>>          if (preq_addr.s_addr != htonl(0L)) {
>>> -            bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
>>> +            bc = request_addr(slirp, &preq_addr, client_ethaddr);
>>>              if (bc) {
>>>                  daddr.sin_addr = preq_addr;
>>>              }
>>>          }
>>>          if (!bc) {
>>>           new_addr:
>>> -            bc = get_new_addr(slirp, &daddr.sin_addr, slirp->client_ethaddr);
>>> +            bc = get_new_addr(slirp, &daddr.sin_addr, client_ethaddr);
>>>              if (!bc) {
>>>                  DPRINTF("no address left\n");
>>>                  return;
>>>              }
>>>          }
>>> -        memcpy(bc->macaddr, slirp->client_ethaddr, 6);
>>> +        memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>>>      } else if (preq_addr.s_addr != htonl(0L)) {
>>> -        bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
>>> +        bc = request_addr(slirp, &preq_addr, client_ethaddr);
>>>          if (bc) {
>>>              daddr.sin_addr = preq_addr;
>>> -            memcpy(bc->macaddr, slirp->client_ethaddr, 6);
>>> +            memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>>>          } else {
>>>              daddr.sin_addr.s_addr = 0;
>>>          }
>>> @@ -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.
>>
> 
> 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.
> 

Jan
Fabien Chouteau - Aug. 1, 2011, 3:03 p.m.
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 <chouteau@adacore.com>
>>>> ---
>>>>  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.
Jan Kiszka - Aug. 1, 2011, 3:11 p.m.
On 2011-08-01 17:03, Fabien Chouteau wrote:
> 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 <chouteau@adacore.com>
>>>>> ---
>>>>>  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...

We should add a section on consistency - but I guess that will always
remain a subjective matter. :)

> 
>>>>
>>>>> +{
>>>>> +    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.

Actually, the whole 0.0.0.0/8 is source-only, ie. it should never show
up in the ARP table.

Jan
Fabien Chouteau - Aug. 1, 2011, 3:55 p.m.
On 01/08/2011 17:11, Jan Kiszka wrote:
> On 2011-08-01 17:03, Fabien Chouteau wrote:
>> 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 <chouteau@adacore.com>
>>>>>> ---
>>>>>>  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...
>
> We should add a section on consistency - but I guess that will always
> remain a subjective matter. :)

Indeed, I bet we can find in Qemu an example of every C 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.
>
> Actually, the whole 0.0.0.0/8 is source-only, ie. it should never show
> up in the ARP table.
>

Great, so I will just prevent insertion and search of 0.0.0.0/8 addresses.

Regards,

Patch

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])
+{
+    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++) {
+        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(ArpTable *arptbl,
+                      int       in_ip_addr,
+                      uint8_t   out_ethaddr[ETH_ALEN])
+{
+    int i;
+
+    DEBUG_CALL("arp_table_search");
+    DEBUG_ARG("ip = 0x%x", in_ip_addr);
+
+    for (i = 0; i < ARP_TABLE_SIZE; i++) {
+        if (arptbl->table[i].ar_sip == in_ip_addr) {
+            memcpy(out_ethaddr, arptbl->table[i].ar_sha,  ETH_ALEN);
+            DEBUG_ARGS((dfd, " found hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n",
+                        out_ethaddr[0], out_ethaddr[1], out_ethaddr[2],
+                        out_ethaddr[3], out_ethaddr[4], out_ethaddr[5]));
+            return 1;
+        }
+    }
+
+    return 0;
+}
diff --git a/slirp/bootp.c b/slirp/bootp.c
index 1eb2ed1..07cbb80 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);
@@ -164,8 +165,9 @@  static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
     if (dhcp_msg_type != DHCPDISCOVER &&
         dhcp_msg_type != DHCPREQUEST)
         return;
-    /* XXX: this is a hack to get the client mac address */
-    memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6);
+
+    /* Get client's hardware address from bootp request */
+    memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN);
 
     m = m_get(slirp);
     if (!m) {
@@ -178,25 +180,25 @@  static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
 
     if (dhcp_msg_type == DHCPDISCOVER) {
         if (preq_addr.s_addr != htonl(0L)) {
-            bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
+            bc = request_addr(slirp, &preq_addr, client_ethaddr);
             if (bc) {
                 daddr.sin_addr = preq_addr;
             }
         }
         if (!bc) {
          new_addr:
-            bc = get_new_addr(slirp, &daddr.sin_addr, slirp->client_ethaddr);
+            bc = get_new_addr(slirp, &daddr.sin_addr, client_ethaddr);
             if (!bc) {
                 DPRINTF("no address left\n");
                 return;
             }
         }
-        memcpy(bc->macaddr, slirp->client_ethaddr, 6);
+        memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
     } else if (preq_addr.s_addr != htonl(0L)) {
-        bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
+        bc = request_addr(slirp, &preq_addr, client_ethaddr);
         if (bc) {
             daddr.sin_addr = preq_addr;
-            memcpy(bc->macaddr, slirp->client_ethaddr, 6);
+            memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
         } else {
             daddr.sin_addr.s_addr = 0;
         }
@@ -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);
+    }
+
     saddr.sin_addr = slirp->vhost_addr;
     saddr.sin_port = htons(BOOTP_SERVER);
 
@@ -218,7 +225,7 @@  static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
     rbp->bp_xid = bp->bp_xid;
     rbp->bp_htype = 1;
     rbp->bp_hlen = 6;
-    memcpy(rbp->bp_hwaddr, bp->bp_hwaddr, 6);
+    memcpy(rbp->bp_hwaddr, bp->bp_hwaddr, ETH_ALEN);
 
     rbp->bp_yiaddr = daddr.sin_addr; /* Client IP address */
     rbp->bp_siaddr = saddr.sin_addr; /* Server IP address */
diff --git a/slirp/slirp.c b/slirp/slirp.c
index df787ea..ba76757 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -31,11 +31,11 @@ 
 struct in_addr loopback_addr;
 
 /* emulated hosts use the MAC addr 52:55:IP:IP:IP:IP */
-static const uint8_t special_ethaddr[6] = {
+static const uint8_t special_ethaddr[ETH_ALEN] = {
     0x52, 0x55, 0x00, 0x00, 0x00, 0x00
 };
 
-static const uint8_t zero_ethaddr[6] = { 0, 0, 0, 0, 0, 0 };
+static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
 
 /* XXX: suppress those select globals */
 fd_set *global_readfds, *global_writefds, *global_xfds;
@@ -599,42 +599,8 @@  void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds,
 	 global_xfds = NULL;
 }
 
-#define ETH_ALEN 6
-#define ETH_HLEN 14
-
-#define ETH_P_IP	0x0800		/* Internet Protocol packet	*/
-#define ETH_P_ARP	0x0806		/* Address Resolution packet	*/
-
-#define	ARPOP_REQUEST	1		/* ARP request			*/
-#define	ARPOP_REPLY	2		/* ARP reply			*/
-
-struct ethhdr
-{
-	unsigned char	h_dest[ETH_ALEN];	/* destination eth addr	*/
-	unsigned char	h_source[ETH_ALEN];	/* source ether addr	*/
-	unsigned short	h_proto;		/* packet type ID field	*/
-};
-
-struct arphdr
-{
-	unsigned short	ar_hrd;		/* format of hardware address	*/
-	unsigned short	ar_pro;		/* format of protocol address	*/
-	unsigned char	ar_hln;		/* length of hardware address	*/
-	unsigned char	ar_pln;		/* length of protocol address	*/
-	unsigned short	ar_op;		/* ARP opcode (command)		*/
-
-	 /*
-	  *	 Ethernet looks like this : This bit is variable sized however...
-	  */
-	unsigned char		ar_sha[ETH_ALEN];	/* sender hardware address	*/
-	uint32_t		ar_sip;			/* sender IP address		*/
-	unsigned char		ar_tha[ETH_ALEN];	/* target hardware address	*/
-	uint32_t		ar_tip	;		/* target IP address		*/
-} __attribute__((packed));
-
 static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
 {
-    struct ethhdr *eh = (struct ethhdr *)pkt;
     struct arphdr *ah = (struct arphdr *)(pkt + ETH_HLEN);
     uint8_t arp_reply[max(ETH_HLEN + sizeof(struct arphdr), 64)];
     struct ethhdr *reh = (struct ethhdr *)arp_reply;
@@ -645,6 +611,12 @@  static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
     ar_op = ntohs(ah->ar_op);
     switch(ar_op) {
     case ARPOP_REQUEST:
+        if (ah->ar_tip == ah->ar_sip) {
+            /* Gratuitous ARP */
+            arp_table_add(&slirp->arp_table, ah->ar_sip, ah->ar_sha);
+            return;
+        }
+
         if ((ah->ar_tip & slirp->vnetwork_mask.s_addr) ==
             slirp->vnetwork_addr.s_addr) {
             if (ah->ar_tip == slirp->vnameserver_addr.s_addr ||
@@ -657,8 +629,8 @@  static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
             return;
         arp_ok:
             memset(arp_reply, 0, sizeof(arp_reply));
-            /* XXX: make an ARP request to have the client address */
-            memcpy(slirp->client_ethaddr, eh->h_source, ETH_ALEN);
+
+            arp_table_add(&slirp->arp_table, ah->ar_sip, ah->ar_sha);
 
             /* ARP request for alias/dns mac address */
             memcpy(reh->h_dest, pkt + ETH_ALEN, ETH_ALEN);
@@ -679,11 +651,7 @@  static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
         }
         break;
     case ARPOP_REPLY:
-        /* reply to request of client mac address ? */
-        if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN) &&
-            ah->ar_sip == slirp->client_ipaddr.s_addr) {
-            memcpy(slirp->client_ethaddr, ah->ar_sha, ETH_ALEN);
-        }
+        arp_table_add(&slirp->arp_table, ah->ar_sip, ah->ar_sha);
         break;
     default:
         break;
@@ -729,15 +697,16 @@  void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
 {
     uint8_t buf[1600];
     struct ethhdr *eh = (struct ethhdr *)buf;
+    uint8_t ethaddr[ETH_ALEN];
+    const struct ip *iph = (const struct ip *)ip_data;
 
     if (ip_data_len + ETH_HLEN > sizeof(buf))
         return;
-    
-    if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN)) {
+
+    if (!arp_table_search(&slirp->arp_table, iph->ip_dst.s_addr, ethaddr)) {
         uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
         struct ethhdr *reh = (struct ethhdr *)arp_req;
         struct arphdr *rah = (struct arphdr *)(arp_req + ETH_HLEN);
-        const struct ip *iph = (const struct ip *)ip_data;
 
         /* If the client addr is not known, there is no point in
            sending the packet to it. Normally the sender should have
@@ -765,7 +734,7 @@  void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
         slirp->client_ipaddr = iph->ip_dst;
         slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
     } else {
-        memcpy(eh->h_dest, slirp->client_ethaddr, ETH_ALEN);
+        memcpy(eh->h_dest, ethaddr, ETH_ALEN);
         memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
         /* XXX: not correct */
         memcpy(&eh->h_source[2], &slirp->vhost_addr, 4);
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 16bb6ba..103b39d 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -170,6 +170,51 @@  int inet_aton(const char *cp, struct in_addr *ia);
 /* osdep.c */
 int qemu_socket(int domain, int type, int protocol);
 
+#define ETH_ALEN 6
+#define ETH_HLEN 14
+
+#define ETH_P_IP  0x0800        /* Internet Protocol packet  */
+#define ETH_P_ARP 0x0806        /* Address Resolution packet */
+
+#define ARPOP_REQUEST 1         /* ARP request */
+#define ARPOP_REPLY   2         /* ARP reply   */
+
+struct ethhdr {
+    unsigned char  h_dest[ETH_ALEN];   /* destination eth addr */
+    unsigned char  h_source[ETH_ALEN]; /* source ether addr    */
+    unsigned short h_proto;            /* packet type ID field */
+};
+
+struct arphdr {
+    unsigned short ar_hrd;      /* format of hardware address */
+    unsigned short ar_pro;      /* format of protocol address */
+    unsigned char  ar_hln;      /* length of hardware address */
+    unsigned char  ar_pln;      /* length of protocol address */
+    unsigned short ar_op;       /* ARP opcode (command)       */
+
+    /*
+     *  Ethernet looks like this : This bit is variable sized however...
+     */
+    unsigned char ar_sha[ETH_ALEN]; /* sender hardware address */
+    uint32_t      ar_sip;           /* sender IP address       */
+    unsigned char ar_tha[ETH_ALEN]; /* target hardware address */
+    uint32_t      ar_tip;           /* target IP address       */
+} __attribute__((packed));
+
+#define ARP_TABLE_SIZE 16
+
+typedef struct ArpTable {
+    struct arphdr table[ARP_TABLE_SIZE];
+    int next_victim;
+} ArpTable;
+
+void arp_table_add(ArpTable *arptbl,
+                   int       ip_addr,
+                   uint8_t   ethaddr[ETH_ALEN]);
+
+bool arp_table_search(ArpTable *arptbl,
+                      int       in_ip_addr,
+                      uint8_t out_ethaddr[ETH_ALEN]);
 
 struct Slirp {
     QTAILQ_ENTRY(Slirp) entry;
@@ -181,9 +226,6 @@  struct Slirp {
     struct in_addr vdhcp_startaddr;
     struct in_addr vnameserver_addr;
 
-    /* ARP cache for the guest IP addresses (XXX: allow many entries) */
-    uint8_t client_ethaddr[6];
-
     struct in_addr client_ipaddr;
     char client_hostname[33];
 
@@ -227,6 +269,8 @@  struct Slirp {
     char *tftp_prefix;
     struct tftp_session tftp_sessions[TFTP_SESSIONS_MAX];
 
+    ArpTable arp_table;
+
     void *opaque;
 };