[U-Boot] [PATCH v2] FAT: Add FAT write feature

[Mike Frysinger]

On Tue, Oct 25, 2011 at 09:15, Donggeun Kim wrote:
> In some cases, saving data in RAM as a file with FAT format is required.
> This patch allows the file to be written in FAT formatted partition.

i thought Wolfgang NAK-ed FS write patches in the past ...

> --- a/fs/fat/Makefile
> +++ b/fs/fat/Makefile
>
>  COBJS-$(CONFIG_CMD_FAT)        := fat.o
> +COBJS-$(CONFIG_FAT_WRITE):= fat_write.o

missing whitespace before that ":="

> --- /dev/null
> +++ b/fs/fat/fat_write.c
>
> +/*
> + * fat_write.c
> + *
> + * R/W (V)FAT 12/16/32 filesystem implementation by Donggeun Kim
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */

there's no actual copyright here.  you wrote this all from scratch ?

> +static void set_name(dir_entry *dirent, const char *filename)
> +{
> +       int period_location, len, i, ext_num;
> +
> +       len = strlen(filename);

i/period_location/len should be size_t, not int

> +       memcpy(s_name, filename, len);
> +       uppercase(s_name, len);

you use this uppercase() func exactly once, and do a memcpy right
before.  drop the memcpy, and inline the uppercase func and use the
proper toupper helper from linux/ctype.h

> +       /* Pad spaces when the length of file name is shorter than eight */
> +       if (period_location < 8) {
> +               memcpy(dirent->name, s_name, period_location);
> +               for (i = period_location; i < 8; i++)
> +                       dirent->name[i] = ' ';
> +       } else if (period_location == 8) {
> +               memcpy(dirent->name, s_name, period_location);
> +       } else {
> +               memcpy(dirent->name, s_name, 6);
> +               dirent->name[6] = '~';
> +               dirent->name[7] = '1';
> +       }

err, doesn't this "~1" hardcode assume that there is no "~1" file
already existing ?  and if there happens to be, you blow it
away/corrupt the fs ?

> +static __u32 get_fatent_value(fsdata *mydata, __u32 entry)
> +{
> +       /* Get the actual entry from the table */
> +       switch (mydata->fatsize) {
> +       case 32:
> +               ret = FAT2CPU32(((__u32 *) mydata->fatbuf)[offset]);
> +               break;
> +       case 16:
> +               ret = FAT2CPU16(((__u16 *) mydata->fatbuf)[offset]);

ugh, whoever dreamt up these FAT2CPU* macros should get smacked.
these need to get scrubbed from fat.h and all code in u-boot and
replaced with proper get/put unaligned macros from include/linux/.

> +static int is_next_clust(fsdata *mydata, dir_entry *dentptr);
> +static void flush_dir_table(fsdata *mydata, dir_entry **dentptr);

re-order these tiny funcs in the file to avoid having to use prototypes

> +/*
> + * Fill dir_slot entries with appropriate name, id, and attr
> + * The real directory entry is returned by 'dentptr'
> + */
> +static void
> +fill_dir_slot(fsdata *mydata, dir_entry **dentptr, const char *l_name)
> +{
> +       dir_slot *slotptr = (dir_slot *)get_vfatname_block;
> +       __u8 counter, checksum;
> +       int idx = 0, ret;
> +       char s_name[16];
> +
> +       /* Get short file name and checksum value */
> +       strncpy(s_name, (*dentptr)->name, 16);
> +       checksum = mkcksum(s_name);
> +
> +       do {
> +               memset(slotptr, 0x00, sizeof(dir_slot));
> +               ret = str2slot(slotptr, l_name, &idx);
> +               slotptr->id = ++counter;
> +               slotptr->attr = ATTR_VFAT;
> +               slotptr->alias_checksum = checksum;
> +               slotptr++;
> +       } while (ret == 0);
> +
> +       slotptr--;
> +       slotptr->id |= LAST_LONG_ENTRY_MASK;
> +
> +       while (counter >= 1) {
> +               if (is_next_clust(mydata, *dentptr)) {
> +                       /* A new cluster is allocated for directory table */
> +                       flush_dir_table(mydata, dentptr);
> +               }
> +               memcpy(*dentptr, slotptr, sizeof(dir_slot));
> +               (*dentptr)++;
> +               slotptr--;
> +               counter--;
> +       }
> +
> +       if (is_next_clust(mydata, *dentptr)) {
> +               /* A new cluster is allocated for directory table */
> +               flush_dir_table(mydata, dentptr);
> +       }
> +}
> +
> +static __u32 dir_curclust;

all these global variables make me cry.  i guess it's a good thing
u-boot is single threaded.

> +static int set_fatent_value(fsdata *mydata, __u32 entry, __u32 entry_value)
> +{
> ...
> +       /* Set the actual entry */
> +       switch (mydata->fatsize) {
> +       case 32:
> +               ((__u32 *) mydata->fatbuf)[offset] = cpu_to_le32(entry_value);
> +               break;
> +       case 16:
> +               ((__u16 *) mydata->fatbuf)[offset] = cpu_to_le16(entry_value);

use proper put_unaligned helpers from include/linux/

> +static int is_next_clust(fsdata *mydata, dir_entry *dentptr)
> +{
> ...
> +       cur_position = (__u8 *)dentptr - get_dentfromdir_block;

you've got a lot of random casts throughout this file.  makes me
really wonder as to its sanity.

> +                       printf("Error: flush fat buffer\n");

all of these printf() calls without any format args should be puts() instead

> --- a/include/fat.h
> +++ b/include/fat.h
>
>  #define TOLOWER(c)     if((c) >= 'A' && (c) <= 'Z'){(c)+=('a' - 'A');}
> +#define TOUPPER(c)     if ((c) >= 'a' && (c) <= 'z') \
> +                               (c) -= ('a' - 'A');

why do these exist ?  we already have tolower/toupper in
linux/ctype.h, and macros shouldn't be embedding ugly if/assignments.
-mike