From: Jan Kiszka <jan.kiszka@siemens.com>
To: Fabien Chouteau <chouteau@adacore.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table
Date: Wed, 27 Jul 2011 12:49:12 +0200 [thread overview]
Message-ID: <4E2FED28.4030506@siemens.com> (raw)
In-Reply-To: <1311697292-9845-1-git-send-email-chouteau@adacore.com>
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.
> +
> +void arp_table_add(ArpTable *arptbl,
> + struct arphdr *ahdr)
Let's stick with
void arp_table_add(ArpTable *arptbl, struct arphdr *ahdr)
formatting. That's more consistent with other parts of QEMU.
> +{
> + 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.
> +}
> +
> +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..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.
>
> m = m_get(slirp);
> if (!m) {
> @@ -178,25 +179,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;
> }
> @@ -218,7 +219,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..29966e6 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;
> @@ -237,6 +237,8 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
>
> QTAILQ_INSERT_TAIL(&slirp_instances, slirp, entry);
>
> + arp_table_flush(&slirp->arp_table);
> +
> return slirp;
> }
>
> @@ -599,42 +601,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 +613,13 @@ 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:
> +
Please drop this blank line.
> + if (ah->ar_tip == ah->ar_sip) {
> + /* Gratuitous ARP */
> + arp_table_add(&slirp->arp_table, ah);
> + 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 +632,7 @@ 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);
>
> /* ARP request for alias/dns mac address */
> memcpy(reh->h_dest, pkt + ETH_ALEN, ETH_ALEN);
> @@ -677,13 +651,11 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> rah->ar_tip = ah->ar_sip;
> slirp_output(slirp->opaque, arp_reply, sizeof(arp_reply));
> }
> +
> +
Spurious empty lines.
> 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);
> break;
> default:
> break;
> @@ -729,15 +701,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
> @@ -764,8 +737,9 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
> rah->ar_tip = iph->ip_dst.s_addr;
> slirp->client_ipaddr = iph->ip_dst;
> slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
> +
...and another one.
> } 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..13d7471 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -170,6 +170,52 @@ 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_flush(ArpTable *arptbl);
> +
> +void arp_table_add(ArpTable *arptbl,
> + struct arphdr *ahdr);
> +
> +bool arp_table_search(ArpTable *arptbl,
> + int in_ip_addr,
> + uint8_t out_ethaddr[ETH_ALEN]);
>
> struct Slirp {
> QTAILQ_ENTRY(Slirp) entry;
> @@ -181,9 +227,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 +270,8 @@ struct Slirp {
> char *tftp_prefix;
> struct tftp_session tftp_sessions[TFTP_SESSIONS_MAX];
>
> + ArpTable arp_table;
> +
> void *opaque;
> };
>
> --
> 1.7.4.1
>
Looks good otherwise.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
next prev parent reply other threads:[~2011-07-27 10:49 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-26 16:21 [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table Fabien Chouteau
2011-07-26 16:21 ` [Qemu-devel] [PATCH 2/2] [SLIRP] Delayed IP packets Fabien Chouteau
2011-07-27 9:30 ` Jan Kiszka
2011-07-27 10:14 ` Fabien Chouteau
2011-07-27 10:32 ` Jan Kiszka
2011-07-27 10:49 ` Jan Kiszka [this message]
2011-07-27 12:55 ` [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table Fabien Chouteau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4E2FED28.4030506@siemens.com \
--to=jan.kiszka@siemens.com \
--cc=chouteau@adacore.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.