Patchwork [V2,2/2,SLIRP] Delayed IP packets

login
register
mail settings
Submitter Fabien Chouteau
Date July 29, 2011, 4:25 p.m.
Message ID <1311956703-24788-2-git-send-email-chouteau@adacore.com>
Download mbox | patch
Permalink /patch/107421/
State New
Headers show

Comments

Fabien Chouteau - July 29, 2011, 4:25 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).

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(-)
Jan Kiszka - July 29, 2011, 5:35 p.m.
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
Fabien Chouteau - Aug. 1, 2011, 12:50 p.m.
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.

Patch

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