* [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table
@ 2011-07-26 16:21 Fabien Chouteau
2011-07-26 16:21 ` [Qemu-devel] [PATCH 2/2] [SLIRP] Delayed IP packets Fabien Chouteau
2011-07-27 10:49 ` [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table Jan Kiszka
0 siblings, 2 replies; 7+ messages in thread
From: Fabien Chouteau @ 2011-07-26 16:21 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka
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;
+}
+
+void arp_table_add(ArpTable *arptbl,
+ struct arphdr *ahdr)
+{
+ 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;
+}
+
+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);
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:
+
+ 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));
}
+
+
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));
+
} 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH 2/2] [SLIRP] Delayed IP packets
2011-07-26 16:21 [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table Fabien Chouteau
@ 2011-07-26 16:21 ` Fabien Chouteau
2011-07-27 9:30 ` Jan Kiszka
2011-07-27 10:49 ` [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table Jan Kiszka
1 sibling, 1 reply; 7+ messages in thread
From: Fabien Chouteau @ 2011-07-26 16:21 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka
In the current implementation, if Slirp tries to send an IP packet to a client
with an unknown hardware address, the packet is simply dropped and an ARP
request is sent (if_encap in slirp/slirp.c).
This patch adds a list of delayed IP packets to handle such cases. If the
hardware address is unknown, Slirp inserts the packet in delayed list and sends
an ARP request. Each time the ARP table is updated Slirp retries to send the
packet.
Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
slirp/slirp.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
slirp/slirp.h | 15 +++++++++++++
2 files changed, 77 insertions(+), 3 deletions(-)
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 29966e6..fa4e822 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -195,6 +195,7 @@ static void slirp_init_once(void)
static void slirp_state_save(QEMUFile *f, void *opaque);
static int slirp_state_load(QEMUFile *f, void *opaque, int version_id);
+static void slirp_flush_delayed_ip_packet(Slirp *slirp);
Slirp *slirp_init(int restricted, struct in_addr vnetwork,
struct in_addr vnetmask, struct in_addr vhost,
@@ -239,6 +240,8 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
arp_table_flush(&slirp->arp_table);
+ slirp->delayed = NULL;
+
return slirp;
}
@@ -248,6 +251,7 @@ void slirp_cleanup(Slirp *slirp)
unregister_savevm(NULL, "slirp", slirp);
+ slirp_flush_delayed_ip_packet(slirp);
qemu_free(slirp->tftp_prefix);
qemu_free(slirp->bootp_filename);
qemu_free(slirp);
@@ -601,6 +605,49 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds,
global_xfds = NULL;
}
+static void slirp_delay_ip_packet(Slirp *slirp,
+ const uint8_t *ip_data,
+ int ip_data_len)
+{
+ DelayedIpPacket *new = qemu_malloc(sizeof(*new));
+
+ new->next = slirp->delayed;
+ slirp->delayed = new;
+ new->len = ip_data_len;
+ new->data = qemu_malloc(ip_data_len);
+ memcpy(new->data, ip_data, ip_data_len);
+}
+
+static void slirp_send_delayed_ip_packet(Slirp *slirp)
+{
+ DelayedIpPacket *list = slirp->delayed;
+ DelayedIpPacket *pkt;
+
+ slirp->delayed = NULL;
+ while (list != NULL) {
+ pkt = list;
+ list = list->next;
+
+ if_encap(slirp, pkt->data, pkt->len);
+ qemu_free(pkt->data);
+ qemu_free(pkt);
+ }
+}
+
+static void slirp_flush_delayed_ip_packet(Slirp *slirp)
+{
+ DelayedIpPacket *list = slirp->delayed;
+ DelayedIpPacket *pkt;
+
+ slirp->delayed = NULL;
+ while (list != NULL) {
+ pkt = list;
+ list = list->next;
+ qemu_free(pkt->data);
+ qemu_free(pkt);
+ }
+}
+
static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
{
struct arphdr *ah = (struct arphdr *)(pkt + ETH_HLEN);
@@ -609,6 +656,7 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
struct arphdr *rah = (struct arphdr *)(arp_reply + ETH_HLEN);
int ar_op;
struct ex_list *ex_ptr;
+ bool arptbl_updated = false;
ar_op = ntohs(ah->ar_op);
switch(ar_op) {
@@ -617,6 +665,7 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
if (ah->ar_tip == ah->ar_sip) {
/* Gratuitous ARP */
arp_table_add(&slirp->arp_table, ah);
+ arptbl_updated = true;
return;
}
@@ -633,6 +682,7 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
arp_ok:
memset(arp_reply, 0, sizeof(arp_reply));
arp_table_add(&slirp->arp_table, ah);
+ arptbl_updated = true;
/* ARP request for alias/dns mac address */
memcpy(reh->h_dest, pkt + ETH_ALEN, ETH_ALEN);
@@ -656,10 +706,16 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
break;
case ARPOP_REPLY:
arp_table_add(&slirp->arp_table, ah);
+ arptbl_updated = true;
break;
default:
break;
}
+
+ if (arptbl_updated) {
+ /* Try to send the delayed IP packets */
+ slirp_send_delayed_ip_packet(slirp);
+ }
}
void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
@@ -714,9 +770,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
/* If the client addr is not known, there is no point in
sending the packet to it. Normally the sender should have
- done an ARP request to get its MAC address. Here we do it
- in place of sending the packet and we hope that the sender
- will retry sending its packet. */
+ done an ARP request to get its MAC address. */
memset(reh->h_dest, 0xff, ETH_ALEN);
memcpy(reh->h_source, special_ethaddr, ETH_ALEN - 4);
memcpy(&reh->h_source[2], &slirp->vhost_addr, 4);
@@ -738,6 +792,11 @@ 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));
+ /* Put the packet in delayed list, and try to send it when we receive
+ * the arp reply.
+ */
+ slirp_delay_ip_packet(slirp, ip_data, ip_data_len);
+
} else {
memcpy(eh->h_dest, ethaddr, ETH_ALEN);
memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 13d7471..2884285 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -217,6 +217,15 @@ bool arp_table_search(ArpTable *arptbl,
int in_ip_addr,
uint8_t out_ethaddr[ETH_ALEN]);
+struct DelayedIpPacket;
+typedef struct DelayedIpPacket DelayedIpPacket;
+
+struct DelayedIpPacket {
+ uint8_t *data;
+ int len;
+ DelayedIpPacket *next;
+};
+
struct Slirp {
QTAILQ_ENTRY(Slirp) entry;
@@ -272,6 +281,12 @@ struct Slirp {
ArpTable arp_table;
+ /* List of delayed IP packets.
+ * These packets are delayed because the target hw address is unknown. Slirp
+ * will try to send them each time the arp table is updated.
+ */
+ DelayedIpPacket *delayed;
+
void *opaque;
};
--
1.7.4.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 2/2] [SLIRP] Delayed IP packets
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
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kiszka @ 2011-07-27 9:30 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: qemu-devel@nongnu.org
On 2011-07-26 18:21, Fabien Chouteau wrote:
> In the current implementation, if Slirp tries to send an IP packet to a client
> with an unknown hardware address, the packet is simply dropped and an ARP
> request is sent (if_encap in slirp/slirp.c).
>
> This patch adds a list of delayed IP packets to handle such cases. If the
> hardware address is unknown, Slirp inserts the packet in delayed list and sends
> an ARP request. Each time the ARP table is updated Slirp retries to send the
> packet.
Haven't looked at details yet, just two general thoughts so far:
We already have queues for outgoing packets, why can't we reuse that
infrastructure? That would also avoid additional memory allocations.
Delayed packets should be requeued at the end and only one attempt to
send them should be performed per queue flush.
I think we need some timeout for the delayed packets. If invalid or
non-reactive clients are addressed, we will otherwise pile them up until
qemu terminates, right?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] [SLIRP] Delayed IP packets
2011-07-27 9:30 ` Jan Kiszka
@ 2011-07-27 10:14 ` Fabien Chouteau
2011-07-27 10:32 ` Jan Kiszka
0 siblings, 1 reply; 7+ messages in thread
From: Fabien Chouteau @ 2011-07-27 10:14 UTC (permalink / raw)
To: qemu-devel
On 27/07/2011 11:30, Jan Kiszka wrote:
> On 2011-07-26 18:21, Fabien Chouteau wrote:
>> In the current implementation, if Slirp tries to send an IP packet to a client
>> with an unknown hardware address, the packet is simply dropped and an ARP
>> request is sent (if_encap in slirp/slirp.c).
>>
>> This patch adds a list of delayed IP packets to handle such cases. If the
>> hardware address is unknown, Slirp inserts the packet in delayed list and sends
>> an ARP request. Each time the ARP table is updated Slirp retries to send the
>> packet.
>
> Haven't looked at details yet, just two general thoughts so far:
>
> We already have queues for outgoing packets, why can't we reuse that
> infrastructure? That would also avoid additional memory allocations.
> Delayed packets should be requeued at the end and only one attempt to
> send them should be performed per queue flush.
Sure, I didn't know about these queues. Where are they implemented?
>
> I think we need some timeout for the delayed packets. If invalid or
> non-reactive clients are addressed, we will otherwise pile them up until
> qemu terminates, right?
Yes that's a good idea.
Regards,
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] [SLIRP] Delayed IP packets
2011-07-27 10:14 ` Fabien Chouteau
@ 2011-07-27 10:32 ` Jan Kiszka
0 siblings, 0 replies; 7+ messages in thread
From: Jan Kiszka @ 2011-07-27 10:32 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: qemu-devel
On 2011-07-27 12:14, Fabien Chouteau wrote:
> On 27/07/2011 11:30, Jan Kiszka wrote:
>> On 2011-07-26 18:21, Fabien Chouteau wrote:
>>> In the current implementation, if Slirp tries to send an IP packet to a client
>>> with an unknown hardware address, the packet is simply dropped and an ARP
>>> request is sent (if_encap in slirp/slirp.c).
>>>
>>> This patch adds a list of delayed IP packets to handle such cases. If the
>>> hardware address is unknown, Slirp inserts the packet in delayed list and sends
>>> an ARP request. Each time the ARP table is updated Slirp retries to send the
>>> packet.
>>
>> Haven't looked at details yet, just two general thoughts so far:
>>
>> We already have queues for outgoing packets, why can't we reuse that
>> infrastructure? That would also avoid additional memory allocations.
>> Delayed packets should be requeued at the end and only one attempt to
>> send them should be performed per queue flush.
>
> Sure, I didn't know about these queues. Where are they implemented?
Check e.g. what happens in and is documented for if_start().
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table
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 10:49 ` Jan Kiszka
2011-07-27 12:55 ` Fabien Chouteau
1 sibling, 1 reply; 7+ messages in thread
From: Jan Kiszka @ 2011-07-27 10:49 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: qemu-devel@nongnu.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table
2011-07-27 10:49 ` [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table Jan Kiszka
@ 2011-07-27 12:55 ` Fabien Chouteau
0 siblings, 0 replies; 7+ messages in thread
From: Fabien Chouteau @ 2011-07-27 12:55 UTC (permalink / raw)
To: Jan Kiszka; +Cc: qemu-devel@nongnu.org
On 27/07/2011 12:49, Jan Kiszka wrote:
> 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.
OK, it was just in case slirp can be re-initialized.
>> +{
>> + 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.
Yes, simple round-robin.
>> 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.
Actually I don't think it is a hack at all.
Makes me think I should update the ARP table here, with the IP address assigned
to this client.
Thanks for your review, I will fix the others style problems.
Regards,
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-07-27 12:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 ` [Qemu-devel] [PATCH 1/2] [SLIRP] Simple ARP table Jan Kiszka
2011-07-27 12:55 ` Fabien Chouteau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).