| Message ID | 1371663286-12518-3-git-send-email-vfalico@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 19/06/13 19:34, Veaceslav Falico wrote: > Add function bond_get_targets_ip(targets, ip) which searches through > targets array of ips (arp_targets) and returns the position of first > match. If ip == 0, returns the first free slot. On failure to find the > ip or free slot, return -1. > > Use it to verify if the arp we've received is valid and in sysfs. > > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > drivers/net/bonding/bond_main.c | 17 ++++------- > drivers/net/bonding/bond_sysfs.c | 56 +++++++++++++++---------------------- > drivers/net/bonding/bonding.h | 16 +++++++++++ > 3 files changed, 45 insertions(+), 44 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 3d8b5ba..09a79eb 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -2599,22 +2599,17 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave) > > static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32 sip, __be32 tip) > { > - int i; > - __be32 *targets = bond->params.arp_targets; > - > if (!bond_has_this_ip(bond, tip)) { > pr_debug("bva: tip %pI4 not found\n", &tip); > return; > } > > - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) { > - pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip)\n", > - &sip, &tip, i, &targets[i]); > - if (sip == targets[i]) { > - slave->last_arp_rx = jiffies; > - return; > - } > + if (bond_get_targets_ip(bond->params.arp_targets, sip) == -1) { > + pr_debug("bva: sip %pI4 not found in targets\n", &sip); > + return; > } Here you should probably check if sip != 0 (0.0.0.0) because ARP probes with such src address are common to check if there's an address conflict for example. > + small nitpick: I don't think this newline is really necessary. > + slave->last_arp_rx = jiffies; > } > > static int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, > @@ -4831,7 +4826,7 @@ static int __net_init bond_net_init(struct net *net) > > bond_create_proc_dir(bn); > bond_create_sysfs(bn); > - > + > return 0; > } > > diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c > index f8bee4c..e680151 100644 > --- a/drivers/net/bonding/bond_sysfs.c > +++ b/drivers/net/bonding/bond_sysfs.c > @@ -591,7 +591,7 @@ static ssize_t bonding_store_arp_targets(struct device *d, > const char *buf, size_t count) > { > __be32 newtarget; > - int i = 0, done = 0, ret = count; > + int i = 0, ret = -EINVAL; > struct bonding *bond = to_bond(d); > __be32 *targets; > > @@ -602,57 +602,46 @@ static ssize_t bonding_store_arp_targets(struct device *d, > if ((newtarget == 0) || (newtarget == htonl(INADDR_BROADCAST))) { > pr_err("%s: invalid ARP target %pI4 specified for addition\n", > bond->dev->name, &newtarget); > - ret = -EINVAL; > goto out; > } > - /* look for an empty slot to put the target in, and check for dupes */ > - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) { > - if (targets[i] == newtarget) { /* duplicate */ > - pr_err("%s: ARP target %pI4 is already present\n", > - bond->dev->name, &newtarget); > - ret = -EINVAL; > - goto out; > - } > - if (targets[i] == 0) { > - pr_info("%s: adding ARP target %pI4.\n", > - bond->dev->name, &newtarget); > - done = 1; > - targets[i] = newtarget; > - } > + > + if (bond_get_targets_ip(targets, newtarget) != -1) { /* dup */ > + pr_err("%s: ARP target %pI4 is already present\n", > + bond->dev->name, &newtarget); > + goto out; > } > - if (!done) { > + > + i = bond_get_targets_ip(targets, 0); /* first free slot */ > + if (i == -1) { > pr_err("%s: ARP target table is full!\n", > bond->dev->name); > - ret = -EINVAL; > goto out; > } > > + pr_info("%s: adding ARP target %pI4.\n", bond->dev->name, > + &newtarget); > + targets[i] = newtarget; > + Extra new line here. > } else if (buf[0] == '-') { > if ((newtarget == 0) || (newtarget == htonl(INADDR_BROADCAST))) { > pr_err("%s: invalid ARP target %pI4 specified for removal\n", > bond->dev->name, &newtarget); > - ret = -EINVAL; > goto out; > } > > - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) { > - if (targets[i] == newtarget) { > - int j; > - pr_info("%s: removing ARP target %pI4.\n", > - bond->dev->name, &newtarget); > - for (j = i; (j < (BOND_MAX_ARP_TARGETS-1)) && targets[j+1]; j++) > - targets[j] = targets[j+1]; > - > - targets[j] = 0; > - done = 1; > - } > - } > - if (!done) { > + i = bond_get_targets_ip(targets, newtarget); > + if (i == -1) { > pr_info("%s: unable to remove nonexistent ARP target %pI4.\n", > bond->dev->name, &newtarget); > - ret = -EINVAL; > goto out; > } > + > + pr_info("%s: removing ARP target %pI4.\n", bond->dev->name, > + &newtarget); > + for (; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++) > + targets[i] = targets[i+1]; > + targets[i] = 0; > + Another extra new line. > } else { > pr_err("no command found in arp_ip_targets file for bond %s. Use +<addr> or -<addr>.\n", > bond->dev->name); > @@ -660,6 +649,7 @@ static ssize_t bonding_store_arp_targets(struct device *d, > goto out; > } > > + ret = count; > out: > return ret; > } > diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h > index b38609b..7feab6c 100644 > --- a/drivers/net/bonding/bonding.h > +++ b/drivers/net/bonding/bonding.h > @@ -464,6 +464,22 @@ static inline struct slave *bond_slave_has_mac(struct bonding *bond, > return NULL; > } > > +/* Check if the ip is present in arp ip list, or first free slot if ip == 0 > + * Returns -1 if not found, index if found > + */ > +static inline int bond_get_targets_ip(__be32 *targets, __be32 ip) > +{ > + int i; > + > + for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) > + if (targets[i] == ip) > + return i; > + else if (targets[i] == 0) > + break; > + > + return -1; > +} > + > /* exported from bond_main.c */ > extern int bond_net_id; > extern const struct bond_parm_tbl bond_lacp_tbl[]; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 19, 2013 at 11:32:57PM +0200, Nikolay Aleksandrov wrote: >On 19/06/13 19:34, Veaceslav Falico wrote: >> Add function bond_get_targets_ip(targets, ip) which searches through >> targets array of ips (arp_targets) and returns the position of first >> match. If ip == 0, returns the first free slot. On failure to find the >> ip or free slot, return -1. >> >> Use it to verify if the arp we've received is valid and in sysfs. >> >> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> >> --- >> drivers/net/bonding/bond_main.c | 17 ++++------- >> drivers/net/bonding/bond_sysfs.c | 56 +++++++++++++++---------------------- >> drivers/net/bonding/bonding.h | 16 +++++++++++ >> 3 files changed, 45 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 3d8b5ba..09a79eb 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -2599,22 +2599,17 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave) >> >> static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32 sip, __be32 tip) >> { >> - int i; >> - __be32 *targets = bond->params.arp_targets; >> - >> if (!bond_has_this_ip(bond, tip)) { >> pr_debug("bva: tip %pI4 not found\n", &tip); >> return; >> } >> >> - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) { >> - pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip)\n", >> - &sip, &tip, i, &targets[i]); >> - if (sip == targets[i]) { >> - slave->last_arp_rx = jiffies; >> - return; >> - } >> + if (bond_get_targets_ip(bond->params.arp_targets, sip) == -1) { >> + pr_debug("bva: sip %pI4 not found in targets\n", &sip); >> + return; >> } >Here you should probably check if sip != 0 (0.0.0.0) because ARP probes >with such src address are common to check if there's an address conflict >for example. Agree, thank you! Will add this and remove extra lines in v2. Thanks for the review! >> + >small nitpick: I don't think this newline is really necessary. >> + slave->last_arp_rx = jiffies; >> } >> >> static int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, >> @@ -4831,7 +4826,7 @@ static int __net_init bond_net_init(struct net *net) >> >> bond_create_proc_dir(bn); >> bond_create_sysfs(bn); >> - >> + >> return 0; >> } >> >> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c >> index f8bee4c..e680151 100644 >> --- a/drivers/net/bonding/bond_sysfs.c >> +++ b/drivers/net/bonding/bond_sysfs.c >> @@ -591,7 +591,7 @@ static ssize_t bonding_store_arp_targets(struct device *d, >> const char *buf, size_t count) >> { >> __be32 newtarget; >> - int i = 0, done = 0, ret = count; >> + int i = 0, ret = -EINVAL; >> struct bonding *bond = to_bond(d); >> __be32 *targets; >> >> @@ -602,57 +602,46 @@ static ssize_t bonding_store_arp_targets(struct device *d, >> if ((newtarget == 0) || (newtarget == htonl(INADDR_BROADCAST))) { >> pr_err("%s: invalid ARP target %pI4 specified for addition\n", >> bond->dev->name, &newtarget); >> - ret = -EINVAL; >> goto out; >> } >> - /* look for an empty slot to put the target in, and check for dupes */ >> - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) { >> - if (targets[i] == newtarget) { /* duplicate */ >> - pr_err("%s: ARP target %pI4 is already present\n", >> - bond->dev->name, &newtarget); >> - ret = -EINVAL; >> - goto out; >> - } >> - if (targets[i] == 0) { >> - pr_info("%s: adding ARP target %pI4.\n", >> - bond->dev->name, &newtarget); >> - done = 1; >> - targets[i] = newtarget; >> - } >> + >> + if (bond_get_targets_ip(targets, newtarget) != -1) { /* dup */ >> + pr_err("%s: ARP target %pI4 is already present\n", >> + bond->dev->name, &newtarget); >> + goto out; >> } >> - if (!done) { >> + >> + i = bond_get_targets_ip(targets, 0); /* first free slot */ >> + if (i == -1) { >> pr_err("%s: ARP target table is full!\n", >> bond->dev->name); >> - ret = -EINVAL; >> goto out; >> } >> >> + pr_info("%s: adding ARP target %pI4.\n", bond->dev->name, >> + &newtarget); >> + targets[i] = newtarget; >> + >Extra new line here. >> } else if (buf[0] == '-') { >> if ((newtarget == 0) || (newtarget == htonl(INADDR_BROADCAST))) { >> pr_err("%s: invalid ARP target %pI4 specified for removal\n", >> bond->dev->name, &newtarget); >> - ret = -EINVAL; >> goto out; >> } >> >> - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) { >> - if (targets[i] == newtarget) { >> - int j; >> - pr_info("%s: removing ARP target %pI4.\n", >> - bond->dev->name, &newtarget); >> - for (j = i; (j < (BOND_MAX_ARP_TARGETS-1)) && targets[j+1]; j++) >> - targets[j] = targets[j+1]; >> - >> - targets[j] = 0; >> - done = 1; >> - } >> - } >> - if (!done) { >> + i = bond_get_targets_ip(targets, newtarget); >> + if (i == -1) { >> pr_info("%s: unable to remove nonexistent ARP target %pI4.\n", >> bond->dev->name, &newtarget); >> - ret = -EINVAL; >> goto out; >> } >> + >> + pr_info("%s: removing ARP target %pI4.\n", bond->dev->name, >> + &newtarget); >> + for (; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++) >> + targets[i] = targets[i+1]; >> + targets[i] = 0; >> + >Another extra new line. >> } else { >> pr_err("no command found in arp_ip_targets file for bond %s. Use +<addr> or -<addr>.\n", >> bond->dev->name); >> @@ -660,6 +649,7 @@ static ssize_t bonding_store_arp_targets(struct device *d, >> goto out; >> } >> >> + ret = count; >> out: >> return ret; >> } >> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h >> index b38609b..7feab6c 100644 >> --- a/drivers/net/bonding/bonding.h >> +++ b/drivers/net/bonding/bonding.h >> @@ -464,6 +464,22 @@ static inline struct slave *bond_slave_has_mac(struct bonding *bond, >> return NULL; >> } >> >> +/* Check if the ip is present in arp ip list, or first free slot if ip == 0 >> + * Returns -1 if not found, index if found >> + */ >> +static inline int bond_get_targets_ip(__be32 *targets, __be32 ip) >> +{ >> + int i; >> + >> + for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) >> + if (targets[i] == ip) >> + return i; >> + else if (targets[i] == 0) >> + break; >> + >> + return -1; >> +} >> + >> /* exported from bond_main.c */ >> extern int bond_net_id; >> extern const struct bond_parm_tbl bond_lacp_tbl[]; > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 3d8b5ba..09a79eb 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2599,22 +2599,17 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave) static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32 sip, __be32 tip) { - int i; - __be32 *targets = bond->params.arp_targets; - if (!bond_has_this_ip(bond, tip)) { pr_debug("bva: tip %pI4 not found\n", &tip); return; } - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) { - pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip)\n", - &sip, &tip, i, &targets[i]); - if (sip == targets[i]) { - slave->last_arp_rx = jiffies; - return; - } + if (bond_get_targets_ip(bond->params.arp_targets, sip) == -1) { + pr_debug("bva: sip %pI4 not found in targets\n", &sip); + return; } + + slave->last_arp_rx = jiffies; } static int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, @@ -4831,7 +4826,7 @@ static int __net_init bond_net_init(struct net *net) bond_create_proc_dir(bn); bond_create_sysfs(bn); - + return 0; } diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index f8bee4c..e680151 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -591,7 +591,7 @@ static ssize_t bonding_store_arp_targets(struct device *d, const char *buf, size_t count) { __be32 newtarget; - int i = 0, done = 0, ret = count; + int i = 0, ret = -EINVAL; struct bonding *bond = to_bond(d); __be32 *targets; @@ -602,57 +602,46 @@ static ssize_t bonding_store_arp_targets(struct device *d, if ((newtarget == 0) || (newtarget == htonl(INADDR_BROADCAST))) { pr_err("%s: invalid ARP target %pI4 specified for addition\n", bond->dev->name, &newtarget); - ret = -EINVAL; goto out; } - /* look for an empty slot to put the target in, and check for dupes */ - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) { - if (targets[i] == newtarget) { /* duplicate */ - pr_err("%s: ARP target %pI4 is already present\n", - bond->dev->name, &newtarget); - ret = -EINVAL; - goto out; - } - if (targets[i] == 0) { - pr_info("%s: adding ARP target %pI4.\n", - bond->dev->name, &newtarget); - done = 1; - targets[i] = newtarget; - } + + if (bond_get_targets_ip(targets, newtarget) != -1) { /* dup */ + pr_err("%s: ARP target %pI4 is already present\n", + bond->dev->name, &newtarget); + goto out; } - if (!done) { + + i = bond_get_targets_ip(targets, 0); /* first free slot */ + if (i == -1) { pr_err("%s: ARP target table is full!\n", bond->dev->name); - ret = -EINVAL; goto out; } + pr_info("%s: adding ARP target %pI4.\n", bond->dev->name, + &newtarget); + targets[i] = newtarget; + } else if (buf[0] == '-') { if ((newtarget == 0) || (newtarget == htonl(INADDR_BROADCAST))) { pr_err("%s: invalid ARP target %pI4 specified for removal\n", bond->dev->name, &newtarget); - ret = -EINVAL; goto out; } - for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) { - if (targets[i] == newtarget) { - int j; - pr_info("%s: removing ARP target %pI4.\n", - bond->dev->name, &newtarget); - for (j = i; (j < (BOND_MAX_ARP_TARGETS-1)) && targets[j+1]; j++) - targets[j] = targets[j+1]; - - targets[j] = 0; - done = 1; - } - } - if (!done) { + i = bond_get_targets_ip(targets, newtarget); + if (i == -1) { pr_info("%s: unable to remove nonexistent ARP target %pI4.\n", bond->dev->name, &newtarget); - ret = -EINVAL; goto out; } + + pr_info("%s: removing ARP target %pI4.\n", bond->dev->name, + &newtarget); + for (; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++) + targets[i] = targets[i+1]; + targets[i] = 0; + } else { pr_err("no command found in arp_ip_targets file for bond %s. Use +<addr> or -<addr>.\n", bond->dev->name); @@ -660,6 +649,7 @@ static ssize_t bonding_store_arp_targets(struct device *d, goto out; } + ret = count; out: return ret; } diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index b38609b..7feab6c 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -464,6 +464,22 @@ static inline struct slave *bond_slave_has_mac(struct bonding *bond, return NULL; } +/* Check if the ip is present in arp ip list, or first free slot if ip == 0 + * Returns -1 if not found, index if found + */ +static inline int bond_get_targets_ip(__be32 *targets, __be32 ip) +{ + int i; + + for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) + if (targets[i] == ip) + return i; + else if (targets[i] == 0) + break; + + return -1; +} + /* exported from bond_main.c */ extern int bond_net_id; extern const struct bond_parm_tbl bond_lacp_tbl[];
Add function bond_get_targets_ip(targets, ip) which searches through targets array of ips (arp_targets) and returns the position of first match. If ip == 0, returns the first free slot. On failure to find the ip or free slot, return -1. Use it to verify if the arp we've received is valid and in sysfs. Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/bonding/bond_main.c | 17 ++++------- drivers/net/bonding/bond_sysfs.c | 56 +++++++++++++++---------------------- drivers/net/bonding/bonding.h | 16 +++++++++++ 3 files changed, 45 insertions(+), 44 deletions(-)