Patchwork [2/2,SLIRP] Delayed IP packets

login
register
mail settings
Submitter Fabien Chouteau
Date July 26, 2011, 4:21 p.m.
Message ID <1311697292-9845-2-git-send-email-chouteau@adacore.com>
Download mbox | patch
Permalink /patch/106890/
State New
Headers show

Comments

Fabien Chouteau - July 26, 2011, 4:21 p.m.
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(-)
Jan Kiszka - July 27, 2011, 9:30 a.m.
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
Fabien Chouteau - July 27, 2011, 10:14 a.m.
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,
Jan Kiszka - July 27, 2011, 10:32 a.m.
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

Patch

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;
 };