bond_create() initialize six workqueues used later on. In the unlikely
event that the device registration fails, these structures are initialized
unnecessarily, so move the initialization out of the error path.
Also, create an error label to remove some duplicated code.
Signed-off-by: Matteo Croce <[email protected]>
---
drivers/net/bonding/bond_main.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index fcb7c2f7f001..8756b6a023d7 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4889,8 +4889,8 @@ int bond_create(struct net *net, const char *name)
bond_setup, tx_queues);
if (!bond_dev) {
pr_err("%s: eek! can't alloc netdev!\n", name);
- rtnl_unlock();
- return -ENOMEM;
+ res = -ENOMEM;
+ goto out_unlock;
}
/*
@@ -4905,14 +4905,17 @@ int bond_create(struct net *net, const char *name)
bond_dev->rtnl_link_ops = &bond_link_ops;
res = register_netdevice(bond_dev);
+ if (res < 0) {
+ free_netdev(bond_dev);
+ goto out_unlock;
+ }
netif_carrier_off(bond_dev);
bond_work_init_all(bond);
+out_unlock:
rtnl_unlock();
- if (res < 0)
- free_netdev(bond_dev);
return res;
}
--
2.23.0
On Tue, 10 Dec 2019 16:24:54 +0100, Matteo Croce wrote:
> bond_create() initialize six workqueues used later on.
Work _entries_ not _queues_ no?
> In the unlikely event that the device registration fails, these
> structures are initialized unnecessarily, so move the initialization
> out of the error path. Also, create an error label to remove some
> duplicated code.
Does the initialization of work entries matter? Is this prep for further
changes?
> Signed-off-by: Matteo Croce <[email protected]>
> ---
> drivers/net/bonding/bond_main.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index fcb7c2f7f001..8756b6a023d7 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4889,8 +4889,8 @@ int bond_create(struct net *net, const char *name)
> bond_setup, tx_queues);
> if (!bond_dev) {
> pr_err("%s: eek! can't alloc netdev!\n", name);
If this is a clean up patch I think this pr_err() could also be removed?
Memory allocation usually fail very loudly so there should be no reason
to print more errors.
> - rtnl_unlock();
> - return -ENOMEM;
> + res = -ENOMEM;
> + goto out_unlock;
> }
>
> /*
> @@ -4905,14 +4905,17 @@ int bond_create(struct net *net, const char *name)
> bond_dev->rtnl_link_ops = &bond_link_ops;
>
> res = register_netdevice(bond_dev);
> + if (res < 0) {
> + free_netdev(bond_dev);
> + goto out_unlock;
> + }
>
> netif_carrier_off(bond_dev);
>
> bond_work_init_all(bond);
>
> +out_unlock:
> rtnl_unlock();
> - if (res < 0)
> - free_netdev(bond_dev);
> return res;
> }
>
I do appreciate that the change makes the error handling follow a more
usual kernel pattern, but IMHO it'd be even better if the error
handling was completely moved. IOW the success path should end with
return 0; and the error path should contain free_netdev(bond_dev);
- int res;
+ int err;
[...]
rtnl_unlock();
return 0;
err_free_netdev:
free_netdev(bond_dev);
err_unlock:
rtnl_unlock();
return err;
I'm just not 100% sold on the improvement made by this patch being
worth the code churn, please convince me, respin or get an ack from
one of the maintainers? :)
On Sat, Dec 14, 2019 at 3:11 AM Jakub Kicinski
<[email protected]> wrote:
>
> On Tue, 10 Dec 2019 16:24:54 +0100, Matteo Croce wrote:
> > bond_create() initialize six workqueues used later on.
>
> Work _entries_ not _queues_ no?
>
Right
> > In the unlikely event that the device registration fails, these
> > structures are initialized unnecessarily, so move the initialization
> > out of the error path. Also, create an error label to remove some
> > duplicated code.
>
> Does the initialization of work entries matter? Is this prep for further
> changes?
>
Not a big issue, I just found useless to initialize those data and
free a bit later.
Just a cleanup.
> > Signed-off-by: Matteo Croce <[email protected]>
> > ---
> > drivers/net/bonding/bond_main.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index fcb7c2f7f001..8756b6a023d7 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -4889,8 +4889,8 @@ int bond_create(struct net *net, const char *name)
> > bond_setup, tx_queues);
> > if (!bond_dev) {
> > pr_err("%s: eek! can't alloc netdev!\n", name);
>
> If this is a clean up patch I think this pr_err() could also be removed?
> Memory allocation usually fail very loudly so there should be no reason
> to print more errors.
>
Sure, I just didn't want to alter the behaviour too much.
> > - rtnl_unlock();
> > - return -ENOMEM;
> > + res = -ENOMEM;
> > + goto out_unlock;
> > }
> >
> > /*
> > @@ -4905,14 +4905,17 @@ int bond_create(struct net *net, const char *name)
> > bond_dev->rtnl_link_ops = &bond_link_ops;
> >
> > res = register_netdevice(bond_dev);
> > + if (res < 0) {
> > + free_netdev(bond_dev);
> > + goto out_unlock;
> > + }
> >
> > netif_carrier_off(bond_dev);
> >
> > bond_work_init_all(bond);
> >
> > +out_unlock:
> > rtnl_unlock();
> > - if (res < 0)
> > - free_netdev(bond_dev);
> > return res;
> > }
> >
>
> I do appreciate that the change makes the error handling follow a more
> usual kernel pattern, but IMHO it'd be even better if the error
> handling was completely moved. IOW the success path should end with
> return 0; and the error path should contain free_netdev(bond_dev);
>
> - int res;
> + int err;
>
> [...]
>
> rtnl_unlock();
>
> return 0;
>
> err_free_netdev:
> free_netdev(bond_dev);
> err_unlock:
> rtnl_unlock();
> return err;
>
> I'm just not 100% sold on the improvement made by this patch being
> worth the code churn, please convince me, respin or get an ack from
> one of the maintainers? :)
>
ACK :)
--
Matteo Croce
per aspera ad upstream