* [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table
@ 2011-07-29 16:25 Fabien Chouteau
2011-07-29 16:25 ` [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets Fabien Chouteau
2011-07-29 17:34 ` [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table Jan Kiszka
0 siblings, 2 replies; 10+ messages in thread
From: Fabien Chouteau @ 2011-07-29 16:25 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 | 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])
+{
+ 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++) {
+ 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);
+ }
+
saddr.sin_addr = slirp->vhost_addr;
saddr.sin_port = htons(BOOTP_SERVER);
@@ -218,7 +225,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..ba76757 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;
@@ -599,42 +599,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 +611,12 @@ 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->ar_sip, ah->ar_sha);
+ 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 +629,8 @@ 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->ar_sip, ah->ar_sha);
/* ARP request for alias/dns mac address */
memcpy(reh->h_dest, pkt + ETH_ALEN, ETH_ALEN);
@@ -679,11 +651,7 @@ static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
}
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->ar_sip, ah->ar_sha);
break;
default:
break;
@@ -729,15 +697,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
@@ -765,7 +734,7 @@ 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));
} 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..103b39d 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -170,6 +170,51 @@ 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_add(ArpTable *arptbl,
+ int ip_addr,
+ uint8_t ethaddr[ETH_ALEN]);
+
+bool arp_table_search(ArpTable *arptbl,
+ int in_ip_addr,
+ uint8_t out_ethaddr[ETH_ALEN]);
struct Slirp {
QTAILQ_ENTRY(Slirp) entry;
@@ -181,9 +226,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 +269,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] 10+ messages in thread* [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets
2011-07-29 16:25 [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table Fabien Chouteau
@ 2011-07-29 16:25 ` Fabien Chouteau
2011-07-29 17:35 ` Jan Kiszka
2011-07-29 17:34 ` [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table Jan Kiszka
1 sibling, 1 reply; 10+ messages in thread
From: Fabien Chouteau @ 2011-07-29 16:25 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).
With this patch, Slirp will send the ARP request, re-queue the packet and try
to send it later. The packet is dropped after one second if the ARP reply is
not received.
Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
slirp/if.c | 28 ++++++++++++++++++++---
slirp/main.h | 2 +-
slirp/mbuf.c | 2 +
slirp/mbuf.h | 2 +
slirp/slirp.c | 67 ++++++++++++++++++++++++++++++--------------------------
slirp/slirp.h | 15 ++++++++++++
6 files changed, 80 insertions(+), 36 deletions(-)
diff --git a/slirp/if.c b/slirp/if.c
index 0f04e13..80a5c4e 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -6,6 +6,7 @@
*/
#include <slirp.h>
+#include "qemu-timer.h"
#define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm))
@@ -105,6 +106,9 @@ if_output(struct socket *so, struct mbuf *ifm)
ifs_init(ifm);
insque(ifm, ifq);
+ /* Expiration date = Now + 1 second */
+ ifm->expiration_date = qemu_get_clock_ns(vm_clock) + 1000000000ULL;
+
diddit:
slirp->if_queued++;
@@ -153,6 +157,9 @@ diddit:
void
if_start(Slirp *slirp)
{
+ int requeued = 0;
+ uint64_t now;
+
struct mbuf *ifm, *ifqt;
DEBUG_CALL("if_start");
@@ -165,6 +172,8 @@ if_start(Slirp *slirp)
if (!slirp_can_output(slirp->opaque))
return;
+ now = qemu_get_clock_ns(vm_clock);
+
/*
* See which queue to get next packet from
* If there's something in the fastq, select it immediately
@@ -199,11 +208,22 @@ if_start(Slirp *slirp)
ifm->ifq_so->so_nqueued = 0;
}
- /* Encapsulate the packet for sending */
- if_encap(slirp, (uint8_t *)ifm->m_data, ifm->m_len);
-
- m_free(ifm);
+ if (ifm->expiration_date < now) {
+ /* Expired */
+ m_free(ifm);
+ } else {
+ /* Encapsulate the packet for sending */
+ if (if_encap(slirp, ifm)) {
+ m_free(ifm);
+ } else {
+ /* re-queue */
+ insque(ifm, ifqt);
+ requeued++;
+ }
+ }
if (slirp->if_queued)
goto again;
+
+ slirp->if_queued = requeued;
}
diff --git a/slirp/main.h b/slirp/main.h
index 0dd8d81..028df4b 100644
--- a/slirp/main.h
+++ b/slirp/main.h
@@ -42,5 +42,5 @@ extern int tcp_keepintvl;
#define PROTO_PPP 0x2
#endif
-void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len);
+int if_encap(Slirp *slirp, struct mbuf *ifm);
ssize_t slirp_send(struct socket *so, const void *buf, size_t len, int flags);
diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index ce2eb84..c699c75 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -70,6 +70,8 @@ m_get(Slirp *slirp)
m->m_len = 0;
m->m_nextpkt = NULL;
m->m_prevpkt = NULL;
+ m->arp_requested = false;
+ m->expiration_date = (uint64_t)-1;
end_error:
DEBUG_ARG("m = %lx", (long )m);
return m;
diff --git a/slirp/mbuf.h b/slirp/mbuf.h
index b74544b..55170e5 100644
--- a/slirp/mbuf.h
+++ b/slirp/mbuf.h
@@ -86,6 +86,8 @@ struct mbuf {
char m_dat_[1]; /* ANSI don't like 0 sized arrays */
char *m_ext_;
} M_dat;
+ bool arp_requested;
+ uint64_t expiration_date;
};
#define m_next m_hdr.mh_next
diff --git a/slirp/slirp.c b/slirp/slirp.c
index ba76757..b006620 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -237,6 +237,8 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
QTAILQ_INSERT_TAIL(&slirp_instances, slirp, entry);
+ slirp->delayed = NULL;
+
return slirp;
}
@@ -693,54 +695,57 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
}
/* output the IP packet to the ethernet device */
-void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
+int if_encap(Slirp *slirp, struct mbuf *ifm)
{
uint8_t buf[1600];
struct ethhdr *eh = (struct ethhdr *)buf;
uint8_t ethaddr[ETH_ALEN];
- const struct ip *iph = (const struct ip *)ip_data;
+ const struct ip *iph = (const struct ip *)ifm->m_data;
- if (ip_data_len + ETH_HLEN > sizeof(buf))
- return;
+ if (ifm->m_len + ETH_HLEN > sizeof(buf))
+ return 1;
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);
- /* 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. */
- 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);
- reh->h_proto = htons(ETH_P_ARP);
- rah->ar_hrd = htons(1);
- rah->ar_pro = htons(ETH_P_IP);
- rah->ar_hln = ETH_ALEN;
- rah->ar_pln = 4;
- rah->ar_op = htons(ARPOP_REQUEST);
- /* source hw addr */
- memcpy(rah->ar_sha, special_ethaddr, ETH_ALEN - 4);
- memcpy(&rah->ar_sha[2], &slirp->vhost_addr, 4);
- /* source IP */
- rah->ar_sip = slirp->vhost_addr.s_addr;
- /* target hw addr (none) */
- memset(rah->ar_tha, 0, ETH_ALEN);
- /* target IP */
- rah->ar_tip = iph->ip_dst.s_addr;
- slirp->client_ipaddr = iph->ip_dst;
- slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
+ if (!ifm->arp_requested) {
+
+ /* If the client addr is not known, send an ARP request */
+ 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);
+ reh->h_proto = htons(ETH_P_ARP);
+ rah->ar_hrd = htons(1);
+ rah->ar_pro = htons(ETH_P_IP);
+ rah->ar_hln = ETH_ALEN;
+ rah->ar_pln = 4;
+ rah->ar_op = htons(ARPOP_REQUEST);
+ /* source hw addr */
+ memcpy(rah->ar_sha, special_ethaddr, ETH_ALEN - 4);
+ memcpy(&rah->ar_sha[2], &slirp->vhost_addr, 4);
+ /* source IP */
+ rah->ar_sip = slirp->vhost_addr.s_addr;
+ /* target hw addr (none) */
+ memset(rah->ar_tha, 0, ETH_ALEN);
+ /* target IP */
+ rah->ar_tip = iph->ip_dst.s_addr;
+ slirp->client_ipaddr = iph->ip_dst;
+ slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
+ ifm->arp_requested = true;
+ }
+ return 0;
} else {
+ ifm->arp_requested = false;
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);
eh->h_proto = htons(ETH_P_IP);
- memcpy(buf + sizeof(struct ethhdr), ip_data, ip_data_len);
- slirp_output(slirp->opaque, buf, ip_data_len + ETH_HLEN);
+ memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
+ slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN);
+ return 1;
}
}
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 103b39d..07d7d29 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -216,6 +216,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;
@@ -271,6 +280,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] 10+ messages in thread* Re: [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets
2011-07-29 16:25 ` [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets Fabien Chouteau
@ 2011-07-29 17:35 ` Jan Kiszka
2011-08-01 12:50 ` Fabien Chouteau
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2011-07-29 17:35 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: qemu-devel@nongnu.org
On 2011-07-29 18:25, 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).
>
> With this patch, Slirp will send the ARP request, re-queue the packet and try
> to send it later. The packet is dropped after one second if the ARP reply is
> not received.
>
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
> slirp/if.c | 28 ++++++++++++++++++++---
> slirp/main.h | 2 +-
> slirp/mbuf.c | 2 +
> slirp/mbuf.h | 2 +
> slirp/slirp.c | 67 ++++++++++++++++++++++++++++++--------------------------
> slirp/slirp.h | 15 ++++++++++++
> 6 files changed, 80 insertions(+), 36 deletions(-)
>
> diff --git a/slirp/if.c b/slirp/if.c
> index 0f04e13..80a5c4e 100644
> --- a/slirp/if.c
> +++ b/slirp/if.c
> @@ -6,6 +6,7 @@
> */
>
> #include <slirp.h>
> +#include "qemu-timer.h"
>
> #define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm))
>
> @@ -105,6 +106,9 @@ if_output(struct socket *so, struct mbuf *ifm)
> ifs_init(ifm);
> insque(ifm, ifq);
>
> + /* Expiration date = Now + 1 second */
> + ifm->expiration_date = qemu_get_clock_ns(vm_clock) + 1000000000ULL;
Slirp uses rt_clock for expiry, let's stick with that clock.
> +
> diddit:
> slirp->if_queued++;
>
> @@ -153,6 +157,9 @@ diddit:
> void
> if_start(Slirp *slirp)
> {
> + int requeued = 0;
> + uint64_t now;
> +
> struct mbuf *ifm, *ifqt;
>
> DEBUG_CALL("if_start");
> @@ -165,6 +172,8 @@ if_start(Slirp *slirp)
> if (!slirp_can_output(slirp->opaque))
> return;
>
> + now = qemu_get_clock_ns(vm_clock);
> +
> /*
> * See which queue to get next packet from
> * If there's something in the fastq, select it immediately
> @@ -199,11 +208,22 @@ if_start(Slirp *slirp)
> ifm->ifq_so->so_nqueued = 0;
> }
>
> - /* Encapsulate the packet for sending */
> - if_encap(slirp, (uint8_t *)ifm->m_data, ifm->m_len);
> -
> - m_free(ifm);
> + if (ifm->expiration_date < now) {
> + /* Expired */
> + m_free(ifm);
> + } else {
> + /* Encapsulate the packet for sending */
> + if (if_encap(slirp, ifm)) {
> + m_free(ifm);
> + } else {
> + /* re-queue */
> + insque(ifm, ifqt);
> + requeued++;
> + }
> + }
>
> if (slirp->if_queued)
> goto again;
> +
> + slirp->if_queued = requeued;
> }
> diff --git a/slirp/main.h b/slirp/main.h
> index 0dd8d81..028df4b 100644
> --- a/slirp/main.h
> +++ b/slirp/main.h
> @@ -42,5 +42,5 @@ extern int tcp_keepintvl;
> #define PROTO_PPP 0x2
> #endif
>
> -void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len);
> +int if_encap(Slirp *slirp, struct mbuf *ifm);
> ssize_t slirp_send(struct socket *so, const void *buf, size_t len, int flags);
> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
> index ce2eb84..c699c75 100644
> --- a/slirp/mbuf.c
> +++ b/slirp/mbuf.c
> @@ -70,6 +70,8 @@ m_get(Slirp *slirp)
> m->m_len = 0;
> m->m_nextpkt = NULL;
> m->m_prevpkt = NULL;
> + m->arp_requested = false;
> + m->expiration_date = (uint64_t)-1;
> end_error:
> DEBUG_ARG("m = %lx", (long )m);
> return m;
> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
> index b74544b..55170e5 100644
> --- a/slirp/mbuf.h
> +++ b/slirp/mbuf.h
> @@ -86,6 +86,8 @@ struct mbuf {
> char m_dat_[1]; /* ANSI don't like 0 sized arrays */
> char *m_ext_;
> } M_dat;
> + bool arp_requested;
> + uint64_t expiration_date;
> };
>
> #define m_next m_hdr.mh_next
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index ba76757..b006620 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -237,6 +237,8 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
>
> QTAILQ_INSERT_TAIL(&slirp_instances, slirp, entry);
>
> + slirp->delayed = NULL;
> +
> return slirp;
> }
>
> @@ -693,54 +695,57 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> }
>
> /* output the IP packet to the ethernet device */
> -void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
> +int if_encap(Slirp *slirp, struct mbuf *ifm)
> {
> uint8_t buf[1600];
> struct ethhdr *eh = (struct ethhdr *)buf;
> uint8_t ethaddr[ETH_ALEN];
> - const struct ip *iph = (const struct ip *)ip_data;
> + const struct ip *iph = (const struct ip *)ifm->m_data;
>
> - if (ip_data_len + ETH_HLEN > sizeof(buf))
> - return;
> + if (ifm->m_len + ETH_HLEN > sizeof(buf))
> + return 1;
Even if the old cold lacked them as well: { }
>
> 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);
>
> - /* 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. */
> - 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);
> - reh->h_proto = htons(ETH_P_ARP);
> - rah->ar_hrd = htons(1);
> - rah->ar_pro = htons(ETH_P_IP);
> - rah->ar_hln = ETH_ALEN;
> - rah->ar_pln = 4;
> - rah->ar_op = htons(ARPOP_REQUEST);
> - /* source hw addr */
> - memcpy(rah->ar_sha, special_ethaddr, ETH_ALEN - 4);
> - memcpy(&rah->ar_sha[2], &slirp->vhost_addr, 4);
> - /* source IP */
> - rah->ar_sip = slirp->vhost_addr.s_addr;
> - /* target hw addr (none) */
> - memset(rah->ar_tha, 0, ETH_ALEN);
> - /* target IP */
> - rah->ar_tip = iph->ip_dst.s_addr;
> - slirp->client_ipaddr = iph->ip_dst;
> - slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
> + if (!ifm->arp_requested) {
> +
This newline is superfluous...
> + /* If the client addr is not known, send an ARP request */
> + 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);
> + reh->h_proto = htons(ETH_P_ARP);
> + rah->ar_hrd = htons(1);
> + rah->ar_pro = htons(ETH_P_IP);
> + rah->ar_hln = ETH_ALEN;
> + rah->ar_pln = 4;
> + rah->ar_op = htons(ARPOP_REQUEST);
...while it would improve readability here (and below).
> + /* source hw addr */
> + memcpy(rah->ar_sha, special_ethaddr, ETH_ALEN - 4);
> + memcpy(&rah->ar_sha[2], &slirp->vhost_addr, 4);
> + /* source IP */
> + rah->ar_sip = slirp->vhost_addr.s_addr;
> + /* target hw addr (none) */
> + memset(rah->ar_tha, 0, ETH_ALEN);
> + /* target IP */
> + rah->ar_tip = iph->ip_dst.s_addr;
> + slirp->client_ipaddr = iph->ip_dst;
> + slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
> + ifm->arp_requested = true;
> + }
> + return 0;
> } else {
> + ifm->arp_requested = false;
Why? If that is required, you would have to make expiry checks depend on
arp_requested as well - or reset the expiry date here.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets
2011-07-29 17:35 ` Jan Kiszka
@ 2011-08-01 12:50 ` Fabien Chouteau
0 siblings, 0 replies; 10+ messages in thread
From: Fabien Chouteau @ 2011-08-01 12:50 UTC (permalink / raw)
To: Jan Kiszka; +Cc: qemu-devel@nongnu.org
On 29/07/2011 19:35, Jan Kiszka wrote:
> On 2011-07-29 18:25, 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).
>>
>> With this patch, Slirp will send the ARP request, re-queue the packet and try
>> to send it later. The packet is dropped after one second if the ARP reply is
>> not received.
>>
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>> ---
>> slirp/if.c | 28 ++++++++++++++++++++---
>> slirp/main.h | 2 +-
>> slirp/mbuf.c | 2 +
>> slirp/mbuf.h | 2 +
>> slirp/slirp.c | 67 ++++++++++++++++++++++++++++++--------------------------
>> slirp/slirp.h | 15 ++++++++++++
>> 6 files changed, 80 insertions(+), 36 deletions(-)
>>
>> diff --git a/slirp/if.c b/slirp/if.c
>> index 0f04e13..80a5c4e 100644
>> --- a/slirp/if.c
>> +++ b/slirp/if.c
>> @@ -6,6 +6,7 @@
>> */
>>
>> #include <slirp.h>
>> +#include "qemu-timer.h"
>>
>> #define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm))
>>
>> @@ -105,6 +106,9 @@ if_output(struct socket *so, struct mbuf *ifm)
>> ifs_init(ifm);
>> insque(ifm, ifq);
>>
>> + /* Expiration date = Now + 1 second */
>> + ifm->expiration_date = qemu_get_clock_ns(vm_clock) + 1000000000ULL;
>
> Slirp uses rt_clock for expiry, let's stick with that clock.
OK, fixed.
>> @@ -693,54 +695,57 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>> }
>>
>> /* output the IP packet to the ethernet device */
>> -void if_encap(Slirp *slirp, const uint8_t *ip_data, int ip_data_len)
>> +int if_encap(Slirp *slirp, struct mbuf *ifm)
>> {
>> uint8_t buf[1600];
>> struct ethhdr *eh = (struct ethhdr *)buf;
>> uint8_t ethaddr[ETH_ALEN];
>> - const struct ip *iph = (const struct ip *)ip_data;
>> + const struct ip *iph = (const struct ip *)ifm->m_data;
>>
>> - if (ip_data_len + ETH_HLEN > sizeof(buf))
>> - return;
>> + if (ifm->m_len + ETH_HLEN > sizeof(buf))
>> + return 1;
>
> Even if the old cold lacked them as well: { }
I forgot to use checkpatch.pl...
>> + memset(rah->ar_tha, 0, ETH_ALEN);
>> + /* target IP */
>> + rah->ar_tip = iph->ip_dst.s_addr;
>> + slirp->client_ipaddr = iph->ip_dst;
>> + slirp_output(slirp->opaque, arp_req, sizeof(arp_req));
>> + ifm->arp_requested = true;
>> + }
>> + return 0;
>> } else {
>> + ifm->arp_requested = false;
>
> Why? If that is required, you would have to make expiry checks depend on
> arp_requested as well - or reset the expiry date here.
>
That's right, the packet is sent so there's no need to reset this value.
Thanks for your review.
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table
2011-07-29 16:25 [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table Fabien Chouteau
2011-07-29 16:25 ` [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets Fabien Chouteau
@ 2011-07-29 17:34 ` Jan Kiszka
2011-07-30 9:09 ` Jan Kiszka
1 sibling, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2011-07-29 17:34 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: qemu-devel@nongnu.org
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table
2011-07-29 17:34 ` [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table Jan Kiszka
@ 2011-07-30 9:09 ` Jan Kiszka
2011-07-30 9:19 ` Jan Kiszka
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2011-07-30 9:09 UTC (permalink / raw)
Cc: qemu-devel@nongnu.org, Fabien Chouteau
[-- Attachment #1: Type: text/plain, Size: 6800 bytes --]
On 2011-07-29 19:34, Jan Kiszka wrote:
> 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.
>
Unfortunately, this patch also has a more sever issues: it breaks DHCP e.g.
Slirp's bootp sends out all replies, also acks and offers, as
broadcasts. Normal servers already use the clients IP address as
destination here.
Even if bootp is fixed, you still lack logic to deal with special
addresses in your arp table lookup. At least broadcasts need to be
handled, I think other multicasts aren't supported by slirp anyway.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table
2011-07-30 9:09 ` Jan Kiszka
@ 2011-07-30 9:19 ` Jan Kiszka
2011-08-01 15:03 ` Fabien Chouteau
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2011-07-30 9:19 UTC (permalink / raw)
To: Fabien Chouteau, qemu-devel@nongnu.org
[-- Attachment #1: Type: text/plain, Size: 7178 bytes --]
On 2011-07-30 11:09, Jan Kiszka wrote:
> On 2011-07-29 19:34, Jan Kiszka wrote:
>> 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.
>>
>
> Unfortunately, this patch also has a more sever issues: it breaks DHCP e.g.
>
> Slirp's bootp sends out all replies, also acks and offers, as
> broadcasts. Normal servers already use the clients IP address as
> destination here.
Obviously, using a broadcast address on return is valid as well. So this
is no slirp bug, it's purely an ARP table lookup issue introduced by
this patch.
>
> Even if bootp is fixed, you still lack logic to deal with special
> addresses in your arp table lookup. At least broadcasts need to be
> handled, I think other multicasts aren't supported by slirp anyway.
>
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table
2011-07-30 9:19 ` Jan Kiszka
@ 2011-08-01 15:03 ` Fabien Chouteau
2011-08-01 15:11 ` Jan Kiszka
0 siblings, 1 reply; 10+ messages in thread
From: Fabien Chouteau @ 2011-08-01 15:03 UTC (permalink / raw)
To: Jan Kiszka; +Cc: qemu-devel@nongnu.org
On 30/07/2011 11:19, Jan Kiszka wrote:
> On 2011-07-30 11:09, Jan Kiszka wrote:
>> On 2011-07-29 19:34, Jan Kiszka wrote:
>>> 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.
OK I'll change it to keep consistent style.
Unfortunately, there's nothing on this subject in the CODING_STYLE...
>>>
>>>> +{
>>>> + 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.
Isn't 0.0.0.0 a reserved address? I think it's safe to use it here.
>>>> diff --git a/slirp/bootp.c b/slirp/bootp.c
>>>> index 1eb2ed1..07cbb80 100644
>>>> --- a/slirp/bootp.c
>>>> +++ b/slirp/bootp.c
>>>> @@ -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.
I will handle this case directly in arp_table_add to get a consistent behavior.
>>
>> Unfortunately, this patch also has a more sever issues: it breaks DHCP e.g.
>>
>> Slirp's bootp sends out all replies, also acks and offers, as
>> broadcasts. Normal servers already use the clients IP address as
>> destination here.
>
> Obviously, using a broadcast address on return is valid as well. So this
> is no slirp bug, it's purely an ARP table lookup issue introduced by
> this patch.
>
>>
>> Even if bootp is fixed, you still lack logic to deal with special
>> addresses in your arp table lookup. At least broadcasts need to be
>> handled, I think other multicasts aren't supported by slirp anyway.
>>
>
That's right, I will add broadcast handling in the next version.
Thanks for your review.
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table
2011-08-01 15:03 ` Fabien Chouteau
@ 2011-08-01 15:11 ` Jan Kiszka
2011-08-01 15:55 ` Fabien Chouteau
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2011-08-01 15:11 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: qemu-devel@nongnu.org
[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]
On 2011-08-01 17:03, Fabien Chouteau wrote:
> On 30/07/2011 11:19, Jan Kiszka wrote:
>> On 2011-07-30 11:09, Jan Kiszka wrote:
>>> On 2011-07-29 19:34, Jan Kiszka wrote:
>>>> 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.
>
> OK I'll change it to keep consistent style.
>
> Unfortunately, there's nothing on this subject in the CODING_STYLE...
We should add a section on consistency - but I guess that will always
remain a subjective matter. :)
>
>>>>
>>>>> +{
>>>>> + 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.
>
> Isn't 0.0.0.0 a reserved address? I think it's safe to use it here.
Actually, the whole 0.0.0.0/8 is source-only, ie. it should never show
up in the ARP table.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table
2011-08-01 15:11 ` Jan Kiszka
@ 2011-08-01 15:55 ` Fabien Chouteau
0 siblings, 0 replies; 10+ messages in thread
From: Fabien Chouteau @ 2011-08-01 15:55 UTC (permalink / raw)
To: Jan Kiszka; +Cc: qemu-devel@nongnu.org
On 01/08/2011 17:11, Jan Kiszka wrote:
> On 2011-08-01 17:03, Fabien Chouteau wrote:
>> On 30/07/2011 11:19, Jan Kiszka wrote:
>>> On 2011-07-30 11:09, Jan Kiszka wrote:
>>>> On 2011-07-29 19:34, Jan Kiszka wrote:
>>>>> 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.
>>
>> OK I'll change it to keep consistent style.
>>
>> Unfortunately, there's nothing on this subject in the CODING_STYLE...
>
> We should add a section on consistency - but I guess that will always
> remain a subjective matter. :)
Indeed, I bet we can find in Qemu an example of every C coding style...
>
>>
>>>>>
>>>>>> +{
>>>>>> + 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.
>>
>> Isn't 0.0.0.0 a reserved address? I think it's safe to use it here.
>
> Actually, the whole 0.0.0.0/8 is source-only, ie. it should never show
> up in the ARP table.
>
Great, so I will just prevent insertion and search of 0.0.0.0/8 addresses.
Regards,
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-08-01 15:56 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-29 16:25 [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table Fabien Chouteau
2011-07-29 16:25 ` [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets Fabien Chouteau
2011-07-29 17:35 ` Jan Kiszka
2011-08-01 12:50 ` Fabien Chouteau
2011-07-29 17:34 ` [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table Jan Kiszka
2011-07-30 9:09 ` Jan Kiszka
2011-07-30 9:19 ` Jan Kiszka
2011-08-01 15:03 ` Fabien Chouteau
2011-08-01 15:11 ` Jan Kiszka
2011-08-01 15: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).