Re: [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets

2011-08-01 Thread Fabien Chouteau
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 chout...@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) + 10ULL;
 
 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



Re: [Qemu-devel] [PATCH V2 2/2] [SLIRP] Delayed IP packets

2011-07-29 Thread Jan Kiszka
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 chout...@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) + 10ULL;

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 {
   charm_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