[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table |
Date: |
Fri, 29 Jul 2011 19:34:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
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 <address@hidden>
> ---
> 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