lkml.org 
[lkml]   [2018]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3] staging: vt6655: check for memory allocation failures
From
Date


On 2018/3/30 11:39, Ji-Hun Kim wrote:
> On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote:
>>
>> On 2018/3/30 10:44, Ji-Hun Kim wrote:
>>> @@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
>>> }
>>> dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n");
>>> - device_init_rd0_ring(priv);
>>> - device_init_rd1_ring(priv);
>>> - device_init_td0_ring(priv);
>>> - device_init_td1_ring(priv);
>>> + ret = device_init_rd0_ring(priv);
>>> + if (ret)
>>> + goto error;
>>> + ret = device_init_rd1_ring(priv);
>>> + if (ret)
>>> + goto error;
>>> + ret = device_init_td0_ring(priv);
>>> + if (ret)
>>> + goto error;
>>> + ret = device_init_td1_ring(priv);
>>> + if (ret)
>>> + goto error;
>>> device_init_registers(priv);
>>> @@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
>>> ieee80211_wake_queues(hw);
>>> return 0;
>>> +error:
>>> + return ret;
>>> }
>> This code will lead to memory leaks when device_init_rd1_ring()
>> fails, because the memory allocated by device_init_rd0_ring() is not
>> freed.
>>
>> I think this one will be better:
>> ret = device_init_rd0_ring(priv);
>> if (ret)
>> goto error_init_rd0_ring;
>> ret = device_init_rd1_ring(priv);
>> if (ret)
>> goto error_init_rd1_ring;
>> ret = device_init_td0_ring(priv);
>> if (ret)
>> goto error_init_td0_ring;
>> ret = device_init_td1_ring(priv);
>> if (ret)
>> goto error_init_td1_ring;
>> ......
>> error_init_td1_ring:
>> device_free_td0_ring(priv);
>> error_init_td0_ring:
>> device_free_rd1_ring(priv);
>> error_init_rd1_ring:
>> device_free_rd0_ring(priv);
>> error_init_rd0_ring:
>> return ret;
>>
>>
>> Best wishes,
>> Jia-Ju Bai
>>
>>
> But, those freeing function are already placed in the each device_init
> functions for allocation fail like below.

I think it is okay.
I suppose that, for each device_init function, you only free the
resources allocated in this function, and do not handle the resources in
other functions.

> @@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
> + return 0;
> +error:
> + device_free_rd0_ring(priv);
> + return ret;

This code is needed to free the resources allocated in this function.


Best wishes,
Jia-Ju Bai

\
 
 \ /
  Last update: 2018-03-30 05:51    [W:0.269 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site