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

[Laszlo Ersek]

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).

Hm, actually, the "arp_table[i].pkt_pending = 0" assignment does make
sense, at least superficially: the context visible above shows the
containing if statement that would be gated by that assignment. IOW,
"we've sent it now, don't send it again".

But, I still would like someone else with more SLOF knowledge to review
that change, separately.

Thanks!
Laszlo