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