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

[Fabien Chouteau <chouteau@adacore.com>]

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