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

[Tien Fong Chee]

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.