[U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer

[Marek Vasut]

On 2/1/19 9:11 AM, Chee, Tien Fong wrote:
> On Thu, 2019-01-31 at 15:22 +0100, Marek Vasut wrote:
>> On 1/31/19 1:42 PM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>
>>> Drop the statically allocated get_contents_vfatname_block and
>>> dynamically allocate a buffer only if required. This saves
>>> 64KiB of memory.
>>>
>>> Signed-off-by: Stefan Agner <stefan.ag... at toradex.com>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>>>
>>> ---
>>>
>>> changes for v2
>>> - Removed the change for debug message.
>>> - Set allocation based on actual required size instead of default
>>> max
>>>   cluster size
>>> ---
>>>  fs/fat/fat.c | 17 ++++++++++++-----
>>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>>> index ecfa255..347787e 100644
>>> --- a/fs/fat/fat.c
>>> +++ b/fs/fat/fat.c
>>> @@ -306,9 +306,6 @@ get_cluster(fsdata *mydata, __u32 clustnum,
>>> __u8 *buffer, unsigned long size)
>>>   * into 'buffer'.
>>>   * Update the number of bytes read in *gotsize or return -1 on
>>> fatal errors.
>>>   */
>>> -__u8 get_contents_vfatname_block[MAX_CLUSTSIZE]
>>> -	__aligned(ARCH_DMA_MINALIGN);
>>> -
>>>  static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t
>>> pos,
>>>  			__u8 *buffer, loff_t maxsize, loff_t
>>> *gotsize)
>>>  {
>>> @@ -351,15 +348,25 @@ static int get_contents(fsdata *mydata,
>>> dir_entry *dentptr, loff_t pos,
>>>  
>>>  	/* align to beginning of next cluster if any */
>>>  	if (pos) {
>>> +		__u8 *tmp_buffer;
>>> +
>>>  		actsize = min(filesize, (loff_t)bytesperclust);
>>> -		if (get_cluster(mydata, curclust,
>>> get_contents_vfatname_block,
>>> +		tmp_buffer = malloc_cache_aligned(actsize);
>>> +		if (!tmp_buffer) {
>>> +			debug("Error: allocating buffer\n");
>>> +			return -ENOMEM;
>>> +		}
>>> +
>>> +		if (get_cluster(mydata, curclust, tmp_buffer,
>>>  				(int)actsize) != 0) {
>> Is the cast of actsize needed ?
> Okay, i would remove it.
>>
>>>
>>>  			printf("Error reading cluster\n");
>>> +			free(tmp_buffer);
>>>  			return -1;
>>>  		}
>>>  		filesize -= actsize;
>>>  		actsize -= pos;
>>> -		memcpy(buffer, get_contents_vfatname_block + pos,
>>> actsize);
>>> +		memcpy(buffer, tmp_buffer + pos, actsize);
>> Hmmm, tmp_buffer is actsize big , but you're memcpy-ing actsize here,
>> so
>> this would mean you memcpy data past the end of tmp_buffer if pos >
>> 0, no?
> This wouldn't happen because the pos and actsize are reset based on
> beginning of current cluster instead of beginning of a file. So, the
> memcpy would start at pos based on beginning of current cluster until
> the end of current cluster, that means the size it copies is still
> within a cluster size.

So pos is always 0 ?

-- 
Best regards,
Marek Vasut