[Qemu-devel] Re: [PATCH] slirp: ensure minimum packet size

[Bruce Rogers]

 >>> On 2/12/2011 at 12:51 AM, Hervé Poussineau<address@hidden> wrote: 
> Hi,
> 
> address@hidden a écrit :
>> Date: Thu, 10 Feb 2011 15:54:28 -0700
>> From: "Bruce Rogers" <address@hidden>
>> Subject: [Qemu-devel] [PATCH] slirp: ensure minimum packet size
>> To: <address@hidden>
>> Message-ID: <address@hidden>
>> Content-Type: text/plain; charset=US-ASCII
>>
>> With recent gpxe eepro100 drivers, short packets are rejected,
>> so ensure the minimum ethernet packet size.
>>
>> Signed-off-by: Bruce Rogers <address@hidden>
>> ---
>>  slirp/slirp.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>> index 332d83b..b611cf7 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -697,7 +697,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int 
> ip_data_len)
>>          return;
>>      
>>      if (!memcmp(slirp->client_ethaddr, zero_ethaddr, ETH_ALEN)) {
>> -        uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
>> +        uint8_t arp_req[max(ETH_HLEN + sizeof(struct arphdr), 64)];
>>          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;
>>   
> Ack for this part.
> Slirp must not generate frames which are not 802.3 compliant, ie less 
> than 64 bytes.
> A similar fix has already been done at 
> http://git.qemu.org/qemu.git/commit/?id=dbf3c4b4baceb91eb64d09f787cbe92d6518
> 8813
> On a side note, maybe you need to add a memset to clear the unused part 
> of the buffer?
> 
>> @@ -734,7 +734,7 @@ void if_encap(Slirp *slirp, const uint8_t *ip_data, int 
> ip_data_len)
>>          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);
>> +        slirp_output(slirp->opaque, buf, max(ip_data_len + ETH_HLEN, 64));
>>      }
>>  }
>>  
>>   
> Nack for this part.
> Here, you're limiting the Ethernet frame to at most 64 bytes. You 
> probably meant min(ip_data_len + ETH_HLEN, 64).
> 
> Even with min(), I'm not sure if it is slirp responsability to increase 
> buffer size to meet Ethernet standards.
> 
> Hervé

Yeah - obviously that should have been min. ;)

Anyways, I think these patches are misguided, along with the one you referenced 
above.

Instead it appears the right solution to the failures that these changes are 
trying to solve, is to have the emulated nics add padding to receive data to 
ensure the minimum frame size that would have been enforced on a physical 
network.

I have submitted a patch to do that for the eepro100 case which I was 
interested in. Other more common emulated nics are already working that way, 
but some remain without that change.

Bruce