diff mbox

[U-Boot,1/2] fs/fat/fatwrite: Local variable as buffer to store dir_slot entries

Message ID 1468400470-28485-1-git-send-email-tfchee@altera.com
State Changes Requested
Delegated to: Tom Rini
Headers show

Commit Message

Tien Fong Chee July 13, 2016, 9:01 a.m. UTC 
fill_dir_slot use get_contents_vfatname_block as a temporary buffer for
constructing a list of dir_slot entries. To save the memory and providing
correct type of memory for above usage, a local buffer with accurate size
declaration is introduced.

The local array size 640 is used because for long file name entry,
each entry use 32 bytes, one entry can store up to 13 characters.
The maximum number of entry possible is 20. So, total size is
32*20=640bytes.

Signed-off-by: Genevieve Chan <ccheauya@altera.com>
Signed-off-by: Tien Fong Chee <tfchee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: ChinLiang <clsee@altera.com>
Cc: Vagrant Cascadian <vagrant@debian.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Benoît Thébaudeau <benoit@wsystem.com>
---
 fs/fat/fat_write.c |    3 ++-
 include/fat.h      |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

Comments

Stephen Warren July 14, 2016, 9 p.m. UTC | #1 
On 07/13/2016 03:01 AM, Tien Fong Chee wrote:
> fill_dir_slot use get_contents_vfatname_block as a temporary buffer for
> constructing a list of dir_slot entries. To save the memory and providing
> correct type of memory for above usage, a local buffer with accurate size
> declaration is introduced.
>
> The local array size 640 is used because for long file name entry,
> each entry use 32 bytes, one entry can store up to 13 characters.
> The maximum number of entry possible is 20. So, total size is
> 32*20=640bytes.

I'm fairly sure this series is correct[1], although the FAT write code 
is a little hard to follow.

[1] except in the face of disk read errors or corrupted data while 
parsing long filename entries, but the code already looks broken in the 
case of disk errors, and the corrupted data case probably isn't much 
worse now.

> diff --git a/include/fat.h b/include/fat.h

> +/* Maximum number of entry for long file name according to spec */
> +#define MAX_LFN_SLOT	20
> +
>
>   /* Filesystem identifiers */
>   #define FAT12_SIGN	"FAT12   "

Why 2 blank lines there?

Also, I'd suggest simply deleting get_contents_vfatname_block and 
renaming all references to it to use get_dentfromdir_block instead. That 
way, anyone reading the code in the future won't assume the two "block" 
variables refer to different memory, when in fact they share storage. 
Related, there's little point keeping the now-redundant memcpy() call at 
the end of get_long_file_name():

> 		memcpy(get_dentfromdir_block, get_contents_vfatname_block,
> 			mydata->clust_size * mydata->sect_size);
Tien Fong Chee July 19, 2016, 3:24 a.m. UTC | #2 
On Thu, 2016-07-14 at 15:00 -0600, Stephen Warren wrote:
> On 07/13/2016 03:01 AM, Tien Fong Chee wrote:
> > fill_dir_slot use get_contents_vfatname_block as a temporary buffer
> > for
> > constructing a list of dir_slot entries. To save the memory and
> > providing
> > correct type of memory for above usage, a local buffer with
> > accurate size
> > declaration is introduced.
> >
> > The local array size 640 is used because for long file name entry,
> > each entry use 32 bytes, one entry can store up to 13 characters.
> > The maximum number of entry possible is 20. So, total size is
> > 32*20=640bytes.
>
> I'm fairly sure this series is correct[1], although the FAT write
> code
> is a little hard to follow.
>
> [1] except in the face of disk read errors or corrupted data while
> parsing long filename entries, but the code already looks broken in
> the
> case of disk errors, and the corrupted data case probably isn't much
> worse now.
>
> > diff --git a/include/fat.h b/include/fat.h
>
> > +/* Maximum number of entry for long file name according to spec */
> > +#define MAX_LFN_SLOT       20
> > +
> >
> >   /* Filesystem identifiers */
> >   #define FAT12_SIGN        "FAT12   "
>
> Why 2 blank lines there?
Good catch!!
>
> Also, I'd suggest simply deleting get_contents_vfatname_block and
> renaming all references to it to use get_dentfromdir_block instead.
> That
> way, anyone reading the code in the future won't assume the two
> "block"
> variables refer to different memory, when in fact they share storage.
> Related, there's little point keeping the now-redundant memcpy() call
> at
> the end of get_long_file_name():
>
> >             memcpy(get_dentfromdir_block,
> > get_contents_vfatname_block,
> >                     mydata->clust_size * mydata->sect_size);
>
Yeah, i agree with you that we should renaming all to single name to
avoid any confusion. I would remove those redundant codes.

Thanks.
diff mbox

Patch

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index eb3a916..c1d48c5 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -323,7 +323,8 @@  static void flush_dir_table(fsdata *mydata, dir_entry **dentptr);
 static void
 fill_dir_slot(fsdata *mydata, dir_entry **dentptr, const char *l_name)
 {
-	dir_slot *slotptr = (dir_slot *)get_contents_vfatname_block;
+	__u8 temp_dir_slot_buffer[MAX_LFN_SLOT * sizeof(dir_slot)];
+	dir_slot *slotptr = (dir_slot *)temp_dir_slot_buffer;
 	__u8 counter = 0, checksum;
 	int idx = 0, ret;
 	char s_name[16];
diff --git a/include/fat.h b/include/fat.h
index 9d053e6..90fdeba 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -33,6 +33,9 @@ 
 #define FAT16BUFSIZE	(FATBUFSIZE/2)
 #define FAT32BUFSIZE	(FATBUFSIZE/4)
 
+/* Maximum number of entry for long file name according to spec */
+#define MAX_LFN_SLOT	20
+
 
 /* Filesystem identifiers */
 #define FAT12_SIGN	"FAT12   "