linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jay Vosburgh <fubar@us.ibm.com>
To: "David S. Miller" <davem@redhat.com>
Cc: Willy Tarreau <willy@w.ods.org>,
	jgarzik@pobox.com, marcelo@conectiva.com.br, netdev@oss.sgi.com,
	bonding-devel@lists.sourceforge.net,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] 2.4.22-pre9-bk : bonding bug fixes
Date: Wed, 30 Jul 2003 17:22:15 -0700	[thread overview]
Message-ID: <200307310022.h6V0MGjK012821@death.ibm.com> (raw)
In-Reply-To: Message from "David S. Miller" <davem@redhat.com>  of "Wed, 30 Jul 2003 16:49:07 PDT." <20030730164907.43b2d343.davem@redhat.com>

>On Wed, 30 Jul 2003 16:06:58 +0200
>Willy Tarreau <willy@w.ods.org> wrote:
>
>> there are still a few bugs in the current bonding driver. I've reported them
>> several times now, but perhaps not at the right places...
>
>So now we have these few bug fixes, and the backport of the
>2.6.x version of the bonding code, both submitted on the same
>day in fact :-)
>
>Jeff I'd recommend we put Willy's fixes in if you think they're
>OK, then we can think about the 2.6.x backport work for 2.4.23-preX

	I've been looking at Willy's fixes, and the typo (first patch)
and locking fix (third patch) both look good to me.  The second patch
(the dead code warning) points out a real problem, in that the code in
question really has no function, but the patch probably doesn't go far
enough for a final solution (the variable that code would set,
arp_target_hw_addr, is referenced in other places, but ends up always
being NULL because the dead code is the only place it was ever set).

	A more proper solution would be to simply delete the dead code
and the arp_target_hw_addr variable, and replace the variable
references with NULL.  This means that all of the ARP probes sent will
be sent out as broadcasts, which is what's already happening, this
just makes the code clearer.  Patch follows (which replaces Willy's
second patch).

	Does this sound reasonable to everybody?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com


--- linux-2.4.22-pre9-bk-wt/drivers/net/bonding/bond_main.c	2003-07-30 17:06:50.000000000 -0700
+++ linux-2.4.22-pre9-bk/drivers/net/bonding/bond_main.c	2003-07-30 17:08:53.000000000 -0700
@@ -463,7 +463,6 @@
 static unsigned long arp_target[MAX_ARP_IP_TARGETS] = { 0, } ;
 static int arp_ip_count = 0;
 static u32 my_ip = 0;
-char *arp_target_hw_addr = NULL;
 
 static char *primary= NULL;
 
@@ -596,8 +595,7 @@
 
 	for (i = 0; (i<MAX_ARP_IP_TARGETS) && arp_target[i]; i++) { 
 		arp_send(ARPOP_REQUEST, ETH_P_ARP, arp_target[i], slave->dev, 
-			 my_ip, arp_target_hw_addr, slave->dev->dev_addr,
-			 arp_target_hw_addr); 
+			 my_ip, NULL, slave->dev->dev_addr, NULL); 
 	} 
 }
  
@@ -1031,10 +1029,6 @@
 	}
 	if (arp_interval> 0) {  /* arp interval, in milliseconds. */
 		del_timer(&bond->arp_timer);
-                if (arp_target_hw_addr != NULL) {
-			kfree(arp_target_hw_addr); 
-			arp_target_hw_addr = NULL;
-		}
 	}
 
 	if (bond_mode == BOND_MODE_8023AD) {
@@ -3281,28 +3275,6 @@
 		memcpy(&my_ip, the_ip, 4);
 	}
 
-	/* if we are sending arp packets and don't know 
-	 * the target hw address, save it so we don't need 
-	 * to use a broadcast address.
-	 * don't do this if in active backup mode because the slaves must 
-	 * receive packets to stay up, and the only ones they receive are 
-	 * broadcasts. 
-	 */
-	if ( (bond_mode != BOND_MODE_ACTIVEBACKUP) && 
-             (arp_ip_count == 1) &&
-	     (arp_interval > 0) && (arp_target_hw_addr == NULL) &&
-	     (skb->protocol == __constant_htons(ETH_P_IP) ) ) {
-		struct ethhdr *eth_hdr = 
-			(struct ethhdr *) (((char *)skb->data));
-		struct iphdr *ip_hdr = (struct iphdr *)(eth_hdr + 1);
-
-		if (arp_target[0] == ip_hdr->daddr) {
-			arp_target_hw_addr = kmalloc(ETH_ALEN, GFP_KERNEL);
-			if (arp_target_hw_addr != NULL)
-				memcpy(arp_target_hw_addr, eth_hdr->h_dest, ETH_ALEN);
-		}
-	}
-
 	read_lock(&bond->lock);
 
 	read_lock(&bond->ptrlock);

  reply	other threads:[~2003-07-31  0:24 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2003-07-30 14:06 [PATCH] 2.4.22-pre9-bk : bonding bug fixes Willy Tarreau
2003-07-30 23:49 ` David S. Miller
2003-07-31  0:22   ` Jay Vosburgh [this message]
2003-07-31 18:50 ` Jeff Garzik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200307310022.h6V0MGjK012821@death.ibm.com \
    --to=fubar@us.ibm.com \
    --cc=bonding-devel@lists.sourceforge.net \
    --cc=davem@redhat.com \
    --cc=jgarzik@pobox.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcelo@conectiva.com.br \
    --cc=netdev@oss.sgi.com \
    --cc=willy@w.ods.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).