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

[Jan Kiszka]

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

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

> +    ArpTable *arptbl = &slirp->arp_table;
> +    int i;
> +
> +    DEBUG_CALL("arp_table_search");
> +    DEBUG_ARG("ip = 0x%x", in_ip_addr);
> +
> +    /* If address in 0.0.0.0/8 */
> +    if ((in_ip_addr & ~(0xf << 28)) == 0) {

Should rather be an assert() as it means the caller is about to send a
frame to that source-only address.

Jan

signature.asc
Description: OpenPGP digital signature