| Message ID | 1380291125-5671-3-git-send-email-vfalico@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
> @@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work) > > // check if agg_select_timer timer after initialize is timed out > if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) { > + slave = bond_first_slave(bond); > + port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL; > + > // select the active aggregator for the bond > - if ((port = __get_first_port(bond))) { > + if (port) { > if (!port->slave) { > pr_warning("%s: Warning: bond's first port is uninitialized\n", > bond->dev->name); > -- Looks like that could be: slave = bond_first_slave(bond); if (slave) { port = SLAVE_AD_INFO(slave).port; and I assume 'slave == port->slave' so there is no need for the latter check? David -- 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 Fri, Sep 27, 2013 at 03:50:12PM +0100, David Laight wrote: >> @@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work) >> >> // check if agg_select_timer timer after initialize is timed out >> if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) { >> + slave = bond_first_slave(bond); >> + port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL; >> + >> // select the active aggregator for the bond >> - if ((port = __get_first_port(bond))) { >> + if (port) { >> if (!port->slave) { >> pr_warning("%s: Warning: bond's first port is uninitialized\n", >> bond->dev->name); >> -- > >Looks like that could be: > slave = bond_first_slave(bond); > if (slave) { > port = SLAVE_AD_INFO(slave).port; >and I assume 'slave == port->slave' so there is no need for the latter check? I've also fallen to this trap at first - slave->port can (virtually) be NULL, and this way we'll panic on "if (!port->slave)". > > David > > > -- 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 Fri, Sep 27, 2013 at 04:58:25PM +0200, Veaceslav Falico wrote: >On Fri, Sep 27, 2013 at 03:50:12PM +0100, David Laight wrote: >>>@@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work) >>> >>> // check if agg_select_timer timer after initialize is timed out >>> if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) { >>>+ slave = bond_first_slave(bond); >>>+ port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL; >>>+ >>> // select the active aggregator for the bond >>>- if ((port = __get_first_port(bond))) { >>>+ if (port) { >>> if (!port->slave) { >>> pr_warning("%s: Warning: bond's first port is uninitialized\n", >>> bond->dev->name); >>>-- >> >>Looks like that could be: >> slave = bond_first_slave(bond); >> if (slave) { >> port = SLAVE_AD_INFO(slave).port; >>and I assume 'slave == port->slave' so there is no need for the latter check? > >I've also fallen to this trap at first - slave->port can (virtually) be >NULL, and this way we'll panic on "if (!port->slave)". Err, forgot to address the 'slave == port->slave' - yes, virtually it should be - if it's initialized (and, it should be) - however, as I've stated in the cover letter - there are *tons* of cleanups/optimizations of these kind that might be done here - and not to mix cleanups/optimizations with the thing that these patches should do (remove bond_next_slave) - I've decided to leave it as close to the original as possible. > >> >> David >> >> >> -- 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_3ad.c b/drivers/net/bonding/bond_3ad.c index c1535f8..0f86d2b 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -136,19 +136,6 @@ static inline struct bonding *__get_bond_by_port(struct port *port) } /** - * __get_first_port - get the first port in the bond - * @bond: the bond we're looking at - * - * Return the port of the first slave in @bond, or %NULL if it can't be found. - */ -static inline struct port *__get_first_port(struct bonding *bond) -{ - struct slave *first_slave = bond_first_slave(bond); - - return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL; -} - -/** * __get_first_agg - get the first aggregator in the bond * @bond: the bond we're looking at * @@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work) // check if agg_select_timer timer after initialize is timed out if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) { + slave = bond_first_slave(bond); + port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL; + // select the active aggregator for the bond - if ((port = __get_first_port(bond))) { + if (port) { if (!port->slave) { pr_warning("%s: Warning: bond's first port is uninitialized\n", bond->dev->name);
Currently we have only one user of it, so it's kind of useless and just obfusicates things. Remove it and move the logic to the only user - bond_3ad_state_machine_handler(). CC: Jay Vosburgh <fubar@us.ibm.com> CC: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/bonding/bond_3ad.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)