lkml.org 
[lkml]   [2020]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 2/7] staging: qlge: Initialize devlink health dump framework
On Wed, Oct 14, 2020 at 04:08:46PM +0300, Dan Carpenter wrote:
>On Wed, Oct 14, 2020 at 06:43:01PM +0800, Coiby Xu wrote:
>> static int qlge_probe(struct pci_dev *pdev,
>> const struct pci_device_id *pci_entry)
>> {
>> struct net_device *ndev = NULL;
>> struct qlge_adapter *qdev = NULL;
>> + struct devlink *devlink;
>> static int cards_found;
>> int err = 0;
>>
>> - ndev = alloc_etherdev_mq(sizeof(struct qlge_adapter),
>> + devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_adapter));
>> + if (!devlink)
>> + return -ENOMEM;
>> +
>> + qdev = devlink_priv(devlink);
>> +
>> + ndev = alloc_etherdev_mq(sizeof(struct qlge_netdev_priv),
>> min(MAX_CPUS,
>> netif_get_num_default_rss_queues()));
>> if (!ndev)
>> - return -ENOMEM;
>> + goto devlink_free;
>>
>> - err = qlge_init_device(pdev, ndev, cards_found);
>> - if (err < 0) {
>> - free_netdev(ndev);
>> - return err;
>
>In the old code, if qlge_init_device() fails then it frees "ndev".
>
>> - }
>> + qdev->ndev = ndev;
>> + err = qlge_init_device(pdev, qdev, cards_found);
>> + if (err < 0)
>> + goto devlink_free;
>
>But the patch introduces a new resource leak.

Thank you for spotting this issue!

>
>>
>> - qdev = netdev_priv(ndev);
>> SET_NETDEV_DEV(ndev, &pdev->dev);
>> ndev->hw_features = NETIF_F_SG |
>> NETIF_F_IP_CSUM |
>> @@ -4611,8 +4619,14 @@ static int qlge_probe(struct pci_dev *pdev,
>> qlge_release_all(pdev);
>> pci_disable_device(pdev);
>> free_netdev(ndev);
>> - return err;
>> + goto devlink_free;
>> }
>> +
>> + err = devlink_register(devlink, &pdev->dev);
>> + if (err)
>> + goto devlink_free;
>> +
>> + qlge_health_create_reporters(qdev);
>> /* Start up the timer to trigger EEH if
>> * the bus goes dead
>> */
>> @@ -4623,6 +4637,10 @@ static int qlge_probe(struct pci_dev *pdev,
>> atomic_set(&qdev->lb_count, 0);
>> cards_found++;
>> return 0;
>> +
>> +devlink_free:
>> + devlink_free(devlink);
>> + return err;
>> }
>
>The best way to write error handling code is keep tracke of the most
>recent allocation which was allocated successfully.
>
> one = alloc();
> if (!one)
> return -ENOMEM; // <-- nothing allocated successfully
>
> two = alloc();
> if (!two) {
> ret = -ENOMEM;
> goto free_one; // <-- one was allocated successfully
> // Notice that the label name says what
> // the goto does.
> }
>
> three = alloc();
> if (!three) {
> ret = -ENOMEM;
> goto free_two; // <-- two allocated, two freed.
> }
>
> ...
>
> return 0;
>
>free_two:
> free(two);
>free_one:
> free(one);
>
> return ret;
>
Thank you for teaching me this pattern!

>In the old code qlge_probe() freed things before returning, and that's
>fine if there is only two allocations in the function but when there are
>three or more allocations, then use gotos to unwind.
>
>Ideally there would be a ql_deinit_device() function to mirror the
>ql_init_device() function. The ql_init_device() is staging quality
>code with leaks and bad label names. It should be re-written to free
>things one step at a time instead of calling ql_release_all().
>
I'll see how I can improve ql_init_device. Thank you for the suggestion!

>Anyway, let's not introduce new leaks at least.
>
>regards,
>dan carpenter

--
Best regards,
Coiby

\
 
 \ /
  Last update: 2020-10-15 06:29    [W:0.059 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site