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

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux