[SLOF] [PATCH] Clean up pending packet variable in ipv4 code

[Thomas Huth]

On 26/11/15 18:22, Laszlo Ersek wrote:
> On 11/26/15 14:52, Thomas Huth wrote:
>> The pending_pkt structure is not really an ARP cache entry, and
>> the fields .ipv4_addr, .mac_addr and .pkt_pending were only set
>> but never read again. So to avoid confusion, convert the pending
>> packet structure into a simple array for storing the packet and
>> an additional length variable.
>> Also change "pending_pkt.pkt_pending = 0" into
>> "arp_table[i].pkt_pending = 0" instead - otherwise we might
>> end up sending out old packets multiple times when receiving
>> ARP responses.
>>
>> Reported-by: Laszlo Ersek <lersek at redhat.com>
> 
> Oh wow. I reported this in an internal review, more than a year ago. I
> had to dig up my archives now, to find my original report. :)
> 
>> Signed-off-by: Signed-off-by: Thomas Huth <thuth at redhat.com>
>> ---
>>  clients/net-snk/app/netlib/ipv4.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/clients/net-snk/app/netlib/ipv4.c b/clients/net-snk/app/netlib/ipv4.c
>> index 8185de5..aef38ce 100644
>> --- a/clients/net-snk/app/netlib/ipv4.c
>> +++ b/clients/net-snk/app/netlib/ipv4.c
>> @@ -126,7 +126,9 @@ static       uint8_t multicast_mac[] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
>>  static unsigned int arp_consumer = 0;
>>  static unsigned int arp_producer = 0;
>>  static arp_entry_t  arp_table[ARP_ENTRIES];
>> -static arp_entry_t  pending_pkt;
>> +
>> +static uint8_t pending_pkt_frame[ETH_MTU_SIZE];
>> +static int pending_pkt_len;
>>  
>>  /* Function pointer send_ip. Points either to send_ipv4() or send_ipv6() */
>>  int   (*send_ip) (int fd, void *, int);
>> @@ -506,13 +508,11 @@ send_ipv4(int fd, void* buffer, int len)
>>  		arp_entry->pkt_pending = 1;
>>  		arp_entry->ipv4_addr = ip_dst;
>>  		memset(arp_entry->mac_addr, 0, 6);
>> -		pending_pkt.ipv4_addr = ip_dst;
>> -		memset(pending_pkt.mac_addr, 0, 6);
>> -		fill_ethhdr (pending_pkt.eth_frame, htons(ETHERTYPE_IP),
>> +		fill_ethhdr (pending_pkt_frame, htons(ETHERTYPE_IP),
>>  		             get_mac_address(), null_mac_addr);
>> -		memcpy(&pending_pkt.eth_frame[sizeof(struct ethhdr)],
>> +		memcpy(&pending_pkt_frame[sizeof(struct ethhdr)],
>>  		       buffer, len);
>> -		pending_pkt.eth_len = len + sizeof(struct ethhdr);
>> +		pending_pkt_len = len + sizeof(struct ethhdr);
>>  
>>  		set_timer(TICKS_SEC);
>>  		do {
>> @@ -754,11 +754,11 @@ handle_arp(int fd, uint8_t * packet, int32_t packetsize)
>>  
>>  		// do we have something to send
>>  		if (arp_table[i].pkt_pending) {
>> -			struct ethhdr * ethh = (struct ethhdr *) pending_pkt.eth_frame;
>> +			struct ethhdr * ethh = (struct ethhdr *) pending_pkt_frame;
>>  			memcpy(ethh -> dest_mac, arp_table[i].mac_addr, 6);
>>  
>> -			send_ether(fd, pending_pkt.eth_frame, pending_pkt.eth_len);
>> -			pending_pkt.pkt_pending = 0;
>> +			send_ether(fd, pending_pkt_frame, pending_pkt_len);
>> +			arp_table[i].pkt_pending = 0;
>>  			arp_table[i].eth_len = 0;
>>  		}
>>  		return 0; // no error
>>
> 
> Can you please split off the "arp_table[i].pkt_pending = 0" assignment
> to a separate patch? Three reasons:
> 
> - looks like a separate bugfix
> 
> - you can remove the "pending_pkt.pkt_pending" assignment without the
>   new assignment, since pending_pkt.pkt_pending is never read (so
>   its assignment doesn't have to be "atomically" replaced with another
>   assignment)
> 
> - I don't have the bandwidth to review that separate bugfix, but for
>   the rest, I would like to give my R-b (strictly based on my old
>   report, which I have re-read now).

Ok, sure, makes sense, I'll separate it and resend.

 Thomas