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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot