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

[Chee, Tien Fong]

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.
> 
> > 
> > +		free(tmp_buffer);
> >  		*gotsize += actsize;
> >  		if (!filesize)
> >  			return 0;
> > 
>