[U-Boot] [PATCH] FAT: make FAT compile without VFAT

newer
[U-Boot] Pull request - microblaze

older
[U-Boot] [PATCHv2] cmd_time: merge...

Richard Genoud

4 Dec 2012 4 Dec '12

2:04 p.m.

Signed-off-by: Richard Genoud richard.genoud@gmail.com --- fs/fat/fat.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..defdd74 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -589,7 +589,9 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, char *filename, dir_entry *retdent, int dols) { +#ifdef CONFIG_SUPPORT_VFAT __u16 prevcksum = 0xffff; +#endif __u32 curclust = START(retdent); int files = 0, dirs = 0;

@@ -828,7 +830,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, fsdata datablock; fsdata *mydata = &datablock; dir_entry *dentptr = NULL; +#ifdef CONFIG_SUPPORT_VFAT __u16 prevcksum = 0xffff; +#endif char *subname = ""; __u32 cursect; int idx, isdir = 0; @@ -944,7 +948,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer,

for (i = 0; i < DIRENTSPERBLOCK; i++) { char s_name[14], l_name[VFAT_MAXLEN_BYTES]; +#ifdef CONFIG_SUPPORT_VFAT __u8 csum; +#endif

l_name[0] = '\0'; if (dentptr->name[0] == DELETED_FLAG) { @@ -952,7 +958,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, continue; }

+#ifdef CONFIG_SUPPORT_VFAT csum = mkcksum(dentptr->name, dentptr->ext); +#endif if (dentptr->attr & ATTR_VOLUME) { #ifdef CONFIG_SUPPORT_VFAT if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT &&

-- 1.7.2.5

Show replies by date

Marek Vasut

4 Dec 4 Dec

3:24 p.m.

Dear Richard Genoud,

Please provide a reasonable commit message. Why is this needed?

I really dislike the amount of added #ifdefs :( [...]

Best regards, Marek Vasut

Richard Genoud

4:32 p.m.

2012/12/4 Marek Vasut marex@denx.de:

...

Dear Richard Genoud,

Please provide a reasonable commit message. Why is this needed?

hi, You're right, my commit message is nearly inexistent.. sorry.

This is needed in the case where we don't want the VFAT support (for whatever reason). in this case, we #undef CONFIG_SUPPORT_VFAT (in fat.h) and it breaks at : csum = mkcksum(dentptr->name, dentptr->ext);

...

I really dislike the amount of added #ifdefs :( [...]

yes, I wasn't very comfortable with that either, I'll come with something cleaner.

Best regards,

Richard.

...

Best regards, Marek Vasut

-- for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

Stefano Babic

5:16 p.m.

On 04/12/2012 14:04, Richard Genoud wrote:

...

Signed-off-by: Richard Genoud richard.genoud@gmail.com

fs/fat/fat.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..defdd74 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -589,7 +589,9 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, char *filename, dir_entry *retdent, int dols) { +#ifdef CONFIG_SUPPORT_VFAT __u16 prevcksum = 0xffff; +#endif

You can declare the variable __maybe_unused without adding the #ifdef

...

__u32 curclust = START(retdent); int files = 0, dirs = 0;

@@ -828,7 +830,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, fsdata datablock; fsdata *mydata = &datablock; dir_entry *dentptr = NULL; +#ifdef CONFIG_SUPPORT_VFAT __u16 prevcksum = 0xffff; +#endif

Ditto

...

+#ifdef CONFIG_SUPPORT_VFAT csum = mkcksum(dentptr->name, dentptr->ext); +#endif

I think this is the only place where #ifdef is necessary

Best regards, Stefano Babic

-- ===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de =====================================================================

Richard Genoud

13 Dec 13 Dec

11:47 a.m.

New subject: [U-Boot] [PATCH 0/2] clean some FAT code

Hi,

Instead of adding some more ugly #ifdefs, I came with another approach. It makes the code easier to read, and correct the compilation error when VFAT wasn't enabled.

Best regards,

Richard Genoud (2): FAT: remove ifdefs to make the code more readable FAT: use toupper/tolower instead of recoding them

fs/fat/fat.c | 58 +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 14 ++++-------- include/fat.h | 3 -- 3 files changed, 36 insertions(+), 39 deletions(-)

-- 1.7.2.5

Marek Vasut

1:45 p.m.

New subject: [U-Boot] [PATCH 0/2] clean some FAT code

Dear Richard Genoud,

...

Hi,

Instead of adding some more ugly #ifdefs, I came with another approach. It makes the code easier to read, and correct the compilation error when VFAT wasn't enabled.

I feel bad for the code when you say it's FAT so explicitly ... it certainly wishes it was SLIM, but it can't ;-)

Best regards, Marek Vasut

Richard Genoud

1:57 p.m.

New subject: [U-Boot] [PATCH 0/2] clean some FAT code

2012/12/13 Marek Vasut marex@denx.de:

...

Dear Richard Genoud,

I feel bad for the code when you say it's FAT so explicitly ... it certainly wishes it was SLIM, but it can't ;-)

:) I didn't write that on purpose... But it's definitely a good war cry !

Richard Genoud

11:47 a.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

ifdefs in the code are making it harder to read. The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. (the code is discarded by the compiler and linker instead of the preprocessor.)

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

Signed-off-by: Richard Genoud richard.genoud@gmail.com --- fs/fat/fat.c | 55 +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 11 ++------- 2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@ #include <malloc.h> #include <linux/compiler.h>

+#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define VFAT_ENABLED 0 +#endif + /* * Convert a string to lowercase. */ @@ -441,7 +447,6 @@ getit: } while (1); }

-#ifdef CONFIG_SUPPORT_VFAT /* * Extract the file name information from 'slotptr' into 'l_name', * starting at l_name[*idx]. @@ -576,7 +581,6 @@ static __u8 mkcksum(const char name[8], const char ext[3])

return ret; } -#endif /* CONFIG_SUPPORT_VFAT */

/* * Get the directory entry associated with 'filename' from the directory @@ -617,8 +621,8 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, continue; } if ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = ((dir_slot *)dentptr)->alias_checksum; get_vfatname(mydata, curclust, @@ -658,9 +662,7 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, continue; } debug("vfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ -674,14 +676,15 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, debug("Dentname == NULL - %d\n", i); return NULL; } -#ifdef CONFIG_SUPPORT_VFAT - __u8 csum = mkcksum(dentptr->name, dentptr->ext); - if (dols && csum == prevcksum) { - prevcksum = 0xffff; - dentptr++; - continue; + if (VFAT_ENABLED) { + __u8 csum = mkcksum(dentptr->name, dentptr->ext); + if (dols && csum == prevcksum) { + prevcksum = 0xffff; + dentptr++; + continue; + } } -#endif + get_name(dentptr, s_name); if (dols) { int isdir = (dentptr->attr & ATTR_DIR); @@ -884,9 +887,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, return -1; }

-#ifdef CONFIG_SUPPORT_VFAT - debug("VFAT Support enabled\n"); -#endif + if (VFAT_ENABLED) + debug("VFAT Support enabled\n"); + debug("FAT%d, fat_sect: %d, fatlength: %d\n", mydata->fatsize, mydata->fat_sect, mydata->fatlength); debug("Rootdir begins at cluster: %d, sector: %d, offset: %x\n" @@ -952,10 +955,12 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, continue; }

- csum = mkcksum(dentptr->name, dentptr->ext); + if (VFAT_ENABLED) + csum = mkcksum(dentptr->name, dentptr->ext); + if (dentptr->attr & ATTR_VOLUME) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = ((dir_slot *)dentptr)->alias_checksum; @@ -999,9 +1004,7 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } debug("Rootvfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ -1015,13 +1018,13 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } goto exit; } -#ifdef CONFIG_SUPPORT_VFAT - else if (dols == LS_ROOT && csum == prevcksum) { + else if (VFAT_ENABLED && + dols == LS_ROOT && csum == prevcksum) { prevcksum = 0xffff; dentptr++; continue; } -#endif + get_name(dentptr, s_name);

if (dols == LS_ROOT) { diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 4a1bda0..ed3eaa0 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -248,7 +248,6 @@ static __u32 get_fatent_value(fsdata *mydata, __u32 entry) return ret; }

-#ifdef CONFIG_SUPPORT_VFAT /* * Set the file name information from 'name' into 'slotptr', */ @@ -468,8 +467,6 @@ get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster, return 0; }

-#endif - /* * Set the entry at index 'entry' in a FAT (16/32) table. */ @@ -853,16 +850,14 @@ static dir_entry *find_directory_entry(fsdata *mydata, int startsect, continue; } if ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) && + if (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { get_long_file_name(mydata, curclust, get_dentfromdir_block, &dentptr, l_name); debug("vfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; if (is_next_clust(mydata, dentptr))

-- 1.7.2.5

Marek Vasut

1:46 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

Dear Richard Genoud,

...

ifdefs in the code are making it harder to read. The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. (the code is discarded by the compiler and linker instead of the preprocessor.)

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

Signed-off-by: Richard Genoud richard.genoud@gmail.com

fs/fat/fat.c | 55 +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 11 ++------- 2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@ #include <malloc.h> #include <linux/compiler.h>

+#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define VFAT_ENABLED 0 +#endif

[...]

Make it static const int maybe ?

Best regards, Marek Vasut

Richard Genoud

2 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

2012/12/13 Marek Vasut marex@denx.de:

...

Dear Richard Genoud,

...
ifdefs in the code are making it harder to read. The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. (the code is discarded by the compiler and linker instead of the preprocessor.)

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

Signed-off-by: Richard Genoud richard.genoud@gmail.com

fs/fat/fat.c | 55 +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 11 ++------- 2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@ #include <malloc.h> #include <linux/compiler.h>

+#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define VFAT_ENABLED 0 +#endif

[...]

Make it static const int maybe ?

I hesitate to make them static const, I can't figure why it's better to use static const variable instead of defines. Could you enlighten me ?

Best regards, Richard.

Marek Vasut

2:12 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

Dear Richard Genoud,

...

2012/12/13 Marek Vasut marex@denx.de:

...
Dear Richard Genoud,

...
ifdefs in the code are making it harder to read. The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. (the code is discarded by the compiler and linker instead of the preprocessor.)

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

Signed-off-by: Richard Genoud richard.genoud@gmail.com

fs/fat/fat.c | 55

+++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 11 ++-------

2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@

#include <malloc.h> #include <linux/compiler.h>

+#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define VFAT_ENABLED 0 +#endif

[...]

Make it static const int maybe ?

I hesitate to make them static const, I can't figure why it's better to use static const variable instead of defines. Could you enlighten me ?

Because you get the type-checking. Preprocessor is evil ;-)

Best regards, Marek Vasut

Richard Genoud

2:16 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

2012/12/13 Marek Vasut marex@denx.de:

...

Dear Richard Genoud,

...
2012/12/13 Marek Vasut marex@denx.de:

...
Dear Richard Genoud,

...
ifdefs in the code are making it harder to read. The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. (the code is discarded by the compiler and linker instead of the preprocessor.)

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

Signed-off-by: Richard Genoud richard.genoud@gmail.com

fs/fat/fat.c | 55

+++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 11 ++-------

2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@

#include <malloc.h> #include <linux/compiler.h>

+#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define VFAT_ENABLED 0 +#endif

[...]

Make it static const int maybe ?

I hesitate to make them static const, I can't figure why it's better to use static const variable instead of defines. Could you enlighten me ?

Because you get the type-checking. Preprocessor is evil ;-)

Best regards, Marek Vasut

Ok ! Thanks, I'll resend it with the change.

Richard.

Marek Vasut

2:19 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

Dear Richard Genoud,

...

2012/12/13 Marek Vasut marex@denx.de:

...
Dear Richard Genoud,

...
2012/12/13 Marek Vasut marex@denx.de:

...
Dear Richard Genoud,

...
ifdefs in the code are making it harder to read. The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. (the code is discarded by the compiler and linker instead of the preprocessor.)

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

Signed-off-by: Richard Genoud richard.genoud@gmail.com

fs/fat/fat.c | 55

+++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 11 ++-------

2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@

#include <malloc.h> #include <linux/compiler.h>

+#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define VFAT_ENABLED 0 +#endif

[...]

Make it static const int maybe ?

I hesitate to make them static const, I can't figure why it's better to use static const variable instead of defines. Could you enlighten me ?

Because you get the type-checking. Preprocessor is evil ;-)

Best regards, Marek Vasut

Ok ! Thanks, I'll resend it with the change.

Thanks!

Yes, preprocessor eats kittens too ;-)

Best regards, Marek Vasut

Richard Genoud

2:30 p.m.

New subject: [U-Boot] [PATCHv2] FAT: remove ifdefs to make the code more readable

ifdefs in the code are making it harder to read. The use of simple if(vfat_enabled) makes no more code and is cleaner. (the code is discarded by the compiler instead of the preprocessor.) NB: if -O0 is used, the code won't be discarded

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..7c23b67 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@ #include <malloc.h> #include <linux/compiler.h>

+#ifdef CONFIG_SUPPORT_VFAT +static const int vfat_enabled = 1; +#else +static const int vfat_enabled = 0; +#endif + /* * Convert a string to lowercase. */ @@ -441,7 +447,6 @@ getit: } while (1); }

return ret; } -#endif /* CONFIG_SUPPORT_VFAT */

/* * Get the directory entry associated with 'filename' from the directory @@ -617,8 +621,8 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, continue; } if ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if (vfat_enabled && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = ((dir_slot *)dentptr)->alias_checksum; get_vfatname(mydata, curclust, @@ -658,9 +662,7 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, continue; } debug("vfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ -674,14 +676,15 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, debug("Dentname == NULL - %d\n", i); return NULL; } -#ifdef CONFIG_SUPPORT_VFAT - __u8 csum = mkcksum(dentptr->name, dentptr->ext); - if (dols && csum == prevcksum) { - prevcksum = 0xffff; - dentptr++; - continue; + if (vfat_enabled) { + __u8 csum = mkcksum(dentptr->name, dentptr->ext); + if (dols && csum == prevcksum) { + prevcksum = 0xffff; + dentptr++; + continue; + } } -#endif + get_name(dentptr, s_name); if (dols) { int isdir = (dentptr->attr & ATTR_DIR); @@ -884,9 +887,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, return -1; }

-#ifdef CONFIG_SUPPORT_VFAT - debug("VFAT Support enabled\n"); -#endif + if (vfat_enabled) + debug("VFAT Support enabled\n"); + debug("FAT%d, fat_sect: %d, fatlength: %d\n", mydata->fatsize, mydata->fat_sect, mydata->fatlength); debug("Rootdir begins at cluster: %d, sector: %d, offset: %x\n" @@ -952,10 +955,12 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, continue; }

- csum = mkcksum(dentptr->name, dentptr->ext); + if (vfat_enabled) + csum = mkcksum(dentptr->name, dentptr->ext); + if (dentptr->attr & ATTR_VOLUME) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if (vfat_enabled && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = ((dir_slot *)dentptr)->alias_checksum; @@ -999,9 +1004,7 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } debug("Rootvfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ -1015,13 +1018,13 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } goto exit; } -#ifdef CONFIG_SUPPORT_VFAT - else if (dols == LS_ROOT && csum == prevcksum) { + else if (vfat_enabled && + dols == LS_ROOT && csum == prevcksum) { prevcksum = 0xffff; dentptr++; continue; } -#endif + get_name(dentptr, s_name);

if (dols == LS_ROOT) { diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 4a1bda0..e7cda17 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -248,7 +248,6 @@ static __u32 get_fatent_value(fsdata *mydata, __u32 entry) return ret; }

-#ifdef CONFIG_SUPPORT_VFAT /* * Set the file name information from 'name' into 'slotptr', */ @@ -468,8 +467,6 @@ get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster, return 0; }

-#endif - /* * Set the entry at index 'entry' in a FAT (16/32) table. */ @@ -853,16 +850,14 @@ static dir_entry *find_directory_entry(fsdata *mydata, int startsect, continue; } if ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) && + if (vfat_enabled && + (dentptr->attr & ATTR_VFAT) && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { get_long_file_name(mydata, curclust, get_dentfromdir_block, &dentptr, l_name); debug("vfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; if (is_next_clust(mydata, dentptr))

-- 1.7.2.5

Tom Rini

4 Feb 4 Feb

5:42 p.m.

New subject: [U-Boot] [U-Boot, PATCHv2] FAT: remove ifdefs to make the code more readable

On Thu, Dec 13, 2012 at 03:30:10AM -0000, Richard Genoud wrote:

...

ifdefs in the code are making it harder to read. The use of simple if(vfat_enabled) makes no more code and is cleaner. (the code is discarded by the compiler instead of the preprocessor.) NB: if -O0 is used, the code won't be discarded

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

Signed-off-by: Richard Genoud richard.genoud@gmail.com

Applied to u-boot/master, thanks!

-- Tom

Tom Rini

13 Dec 13 Dec

2:09 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

On 12/13/12 05:47, Richard Genoud wrote:

...

ifdefs in the code are making it harder to read. The use of simple if(VFAT_ENABLED) makes no more code and is cleaner. (the code is discarded by the compiler and linker instead of the preprocessor.)

and bonus, now the code compiles even if CONFIG_SUPPORT_VFAT is not defined.

Signed-off-by: Richard Genoud richard.genoud@gmail.com --- fs/fat/fat.c | 55 +++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c | 11 ++------- 2 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 393c378..c79e3e3 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -34,6 +34,12 @@ #include <malloc.h> #include <linux/compiler.h>

+#ifdef CONFIG_SUPPORT_VFAT +#define VFAT_ENABLED 1 +#else +#define VFAT_ENABLED 0 +#endif + /* * Convert a string to lowercase. */ @@ -441,7 +447,6 @@ getit: } while (1); }

-#ifdef CONFIG_SUPPORT_VFAT /* * Extract the file name information from 'slotptr' into 'l_name', * starting at l_name[*idx]. @@ -576,7 +581,6 @@ static __u8 mkcksum(const char name[8], const char ext[3])

return ret; } -#endif /* CONFIG_SUPPORT_VFAT */

/* * Get the directory entry associated with 'filename' from the directory @@ -617,8 +621,8 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, continue; } if ((dentptr->attr & ATTR_VOLUME)) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = ((dir_slot *)dentptr)->alias_checksum; get_vfatname(mydata, curclust, @@ -658,9 +662,7 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, continue; } debug("vfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ -674,14 +676,15 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, debug("Dentname == NULL - %d\n", i); return NULL; } -#ifdef CONFIG_SUPPORT_VFAT - __u8 csum = mkcksum(dentptr->name, dentptr->ext); - if (dols && csum == prevcksum) { - prevcksum = 0xffff; - dentptr++; - continue; + if (VFAT_ENABLED) { + __u8 csum = mkcksum(dentptr->name, dentptr->ext); + if (dols && csum == prevcksum) { + prevcksum = 0xffff; + dentptr++; + continue; + } } -#endif + get_name(dentptr, s_name); if (dols) { int isdir = (dentptr->attr & ATTR_DIR); @@ -884,9 +887,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, return -1; }

-#ifdef CONFIG_SUPPORT_VFAT - debug("VFAT Support enabled\n"); -#endif + if (VFAT_ENABLED) + debug("VFAT Support enabled\n"); + debug("FAT%d, fat_sect: %d, fatlength: %d\n", mydata->fatsize, mydata->fat_sect, mydata->fatlength); debug("Rootdir begins at cluster: %d, sector: %d, offset: %x\n" @@ -952,10 +955,12 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, continue; }
	csum = mkcksum(dentptr->name, dentptr->ext); +			if
(VFAT_ENABLED) + csum = mkcksum(dentptr->name, dentptr->ext); + if (dentptr->attr & ATTR_VOLUME) { -#ifdef CONFIG_SUPPORT_VFAT - if ((dentptr->attr & ATTR_VFAT) == ATTR_VFAT && + if (VFAT_ENABLED && + (dentptr->attr & ATTR_VFAT) == ATTR_VFAT && (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) { prevcksum = ((dir_slot *)dentptr)->alias_checksum; @@ -999,9 +1004,7 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } debug("Rootvfatname: |%s|\n", l_name); - } else -#endif - { + } else { /* Volume label or VFAT entry */ dentptr++; continue; @@ -1015,13 +1018,13 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } goto exit; } -#ifdef CONFIG_SUPPORT_VFAT - else if (dols == LS_ROOT && csum == prevcksum) { + else if (VFAT_ENABLED && + dols == LS_ROOT && csum == prevcksum) { prevcksum = 0xffff; dentptr++; continue; } -#endif + get_name(dentptr, s_name);

if (dols == LS_ROOT) { diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 4a1bda0..ed3eaa0 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -248,7 +248,6 @@ static __u32 get_fatent_value(fsdata *mydata, __u32 entry) return ret; }

-#ifdef CONFIG_SUPPORT_VFAT /* * Set the file name information from 'name' into 'slotptr', */ @@ -468,8 +467,6 @@ get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster, return 0; }

-#endif

Note that we don't use --gc-sections on all archs so I'm not sure we discard the unused VFAT functions on say ARM.

- -- Tom

...PGP SIGNATURE...

-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQIcBAEBAgAGBQJQydOVAAoJENk4IS6UOR1WDYQP/2P6ywBeeE/qnx9C13qJnCcr wXG1IJWEpjgAEOViQZ77kE5Knn9ltiuXKj6EYc7e3oY4S2mBREkG/sHNmG+e+9/0 WHuZJoaiLPaia9ztL/d8BCyVPkSbJ88iyA20TgefR94j7SqITiVT+3yDA4XEB3bW C8rXZdmYzGjLzMBPI458uxNmIKSeHVfUZnDdWdA0rIOK8t6QN+OKLJRMezOYwx8e jM8Td4hDmJ20EGfFTpcO2spdUMCqi6FfO6bXJHeFHeUqGXqz482xyIxiZGpETut9 tFOVegHIpbKf0eBoMvs+3ajHprAsMGFlAqv0YSxh4w04ZIXUBT+DibjGiQtf/Xjl HDvAkBGEbQjljZaQDYlaAfNWFctU7lkapujiMuOKjv04DvClp0AgKEvOjAUW/P2N ERIQzc67dUdomhmdl0sl+yFYJDB4zjT1VnlYXGx/yz4KLIiD5t+gaQAyq3pGwYjH jZ3q5gAprYNR0VKl6QM8gQFdAjUKT0owJqUb0iZ2YqblIr+IHLIFHddhwOHB76cg ViLKtbJ7Y43aShdQUE2t8Ibth+ywba3EDBaieqkz4Q928k//kScGSal6nxqE4mhR AZ4ca5euEpADicacHZmKNSbtgeBYvhsC/c4ly27hdLPQjpAGOT/axPoaIfUxIe9H PKIJx4VC14A2u4tS1A6Q =1PBB -----END PGP SIGNATURE-----

Richard Genoud

2:15 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

2012/12/13 Tom Rini trini@ti.com:

...

Note that we don't use --gc-sections on all archs so I'm not sure we discard the unused VFAT functions on say ARM.

I tested it on at91sam9x5ek, and the vfat functions are not present in the System.map.

The problem is in my commit message: the linker doesn"t have anything to do with that because all discarded functions are static.

So I think it's ok on all archs (but maybe not with -O0)

Richard

Marek Vasut

2:51 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

Dear Tom Rini,

[...]

...

Note that we don't use --gc-sections on all archs so I'm not sure we discard the unused VFAT functions on say ARM.

Valid point, Albert?

Best regards, Marek Vasut

Richard Genoud

3:24 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

2012/12/13 Marek Vasut marex@denx.de:

...

Dear Tom Rini,

[...]

...
Note that we don't use --gc-sections on all archs so I'm not sure we discard the unused VFAT functions on say ARM.

Valid point, Albert?

Best regards, Marek Vasut

I check and the code is discarded (on ARM at91sam9x5ek). This is the case because the function to discard are all in the same file (fat.c or fat_write.c). The compiler will discard unused function with -Os. (but not with -O0)

Tom Rini

3:41 p.m.

New subject: [U-Boot] [PATCH 1/2] FAT: remove ifdefs to make the code more readable

On Thu, Dec 13, 2012 at 03:24:08PM +0100, Richard Genoud wrote:

...

2012/12/13 Marek Vasut marex@denx.de:

...
Dear Tom Rini,

[...]

...
Note that we don't use --gc-sections on all archs so I'm not sure we discard the unused VFAT functions on say ARM.

Valid point, Albert?

Best regards, Marek Vasut

I check and the code is discarded (on ARM at91sam9x5ek). This is the case because the function to discard are all in the same file (fat.c or fat_write.c). The compiler will discard unused function with -Os. (but not with -O0)

Thanks for checking.

-- Tom

Richard Genoud

11:47 a.m.

New subject: [U-Boot] [PATCH 2/2] FAT: use toupper/tolower instead of recoding them

toupper/tolower function are already declared, so use them.

Signed-off-by: Richard Genoud richard.genoud@gmail.com --- fs/fat/fat.c | 3 ++- fs/fat/fat_write.c | 3 ++- include/fat.h | 3 --- 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index c79e3e3..cff0526 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -33,6 +33,7 @@ #include <part.h> #include <malloc.h> #include <linux/compiler.h> +#include <linux/ctype.h>

#ifdef CONFIG_SUPPORT_VFAT #define VFAT_ENABLED 1 @@ -46,7 +47,7 @@ static void downcase(char *str) { while (*str != '\0') { - TOLOWER(*str); + *str = tolower(*str); str++; } } diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index ed3eaa0..13c4de1 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -28,6 +28,7 @@ #include <fat.h> #include <asm/byteorder.h> #include <part.h> +#include <linux/ctype.h> #include "fat.c"

static void uppercase(char *str, int len) @@ -35,7 +36,7 @@ static void uppercase(char *str, int len) int i;

for (i = 0; i < len; i++) { - TOUPPER(*str); + *str = toupper(*str); str++; } } diff --git a/include/fat.h b/include/fat.h index 706cd7a..b28c3fd 100644 --- a/include/fat.h +++ b/include/fat.h @@ -98,9 +98,6 @@ #endif #endif

-#define TOLOWER(c) if((c) >= 'A' && (c) <= 'Z'){(c)+=('a' - 'A');} -#define TOUPPER(c) if ((c) >= 'a' && (c) <= 'z') \ - (c) -= ('a' - 'A'); #define START(dent) (FAT2CPU16((dent)->start) \ + (mydata->fatsize != 32 ? 0 : \ (FAT2CPU16((dent)->starthi) << 16)))

-- 1.7.2.5

Marek Vasut

1:47 p.m.

New subject: [U-Boot] [PATCH 2/2] FAT: use toupper/tolower instead of recoding them

Dear Richard Genoud,

...

toupper/tolower function are already declared, so use them.

Signed-off-by: Richard Genoud richard.genoud@gmail.com

fs/fat/fat.c | 3 ++- fs/fat/fat_write.c | 3 ++- include/fat.h | 3 --- 3 files changed, 4 insertions(+), 5 deletions(-)

Acked-by: Marek Vasut marex@denx.de

Best regards, Marek Vasut

Stefano Babic

14 Dec 14 Dec

9:56 a.m.

New subject: [U-Boot] [PATCH 2/2] FAT: use toupper/tolower instead of recoding them

On 13/12/2012 11:47, Richard Genoud wrote:

...

toupper/tolower function are already declared, so use them.

Signed-off-by: Richard Genoud richard.genoud@gmail.com

fs/fat/fat.c | 3 ++- fs/fat/fat_write.c | 3 ++- include/fat.h | 3 --- 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index c79e3e3..cff0526 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -33,6 +33,7 @@ #include <part.h> #include <malloc.h> #include <linux/compiler.h> +#include <linux/ctype.h>

#ifdef CONFIG_SUPPORT_VFAT #define VFAT_ENABLED 1 @@ -46,7 +47,7 @@ static void downcase(char *str) { while (*str != '\0') {
TOLOWER(*str);
*str = tolower(*str);
str++; }
} diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index ed3eaa0..13c4de1 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -28,6 +28,7 @@ #include <fat.h> #include <asm/byteorder.h> #include <part.h> +#include <linux/ctype.h> #include "fat.c"

static void uppercase(char *str, int len) @@ -35,7 +36,7 @@ static void uppercase(char *str, int len) int i;

for (i = 0; i < len; i++) {
TOUPPER(*str);
*str = toupper(*str);
str++; }
} diff --git a/include/fat.h b/include/fat.h index 706cd7a..b28c3fd 100644 --- a/include/fat.h +++ b/include/fat.h @@ -98,9 +98,6 @@ #endif #endif

-#define TOLOWER(c) if((c) >= 'A' && (c) <= 'Z'){(c)+=('a' - 'A');} -#define TOUPPER(c) if ((c) >= 'a' && (c) <= 'z') \
		(c) -= ('a' - 'A');
#define START(dent) (FAT2CPU16((dent)->start) \ + (mydata->fatsize != 32 ? 0 : \ (FAT2CPU16((dent)->starthi) << 16)))

Acked-by: Stefano Babic sbabic@denx.de

Best regards, Stefano Babic

Tom Rini

4 Feb 4 Feb

5:41 p.m.

New subject: [U-Boot] [U-Boot, 2/2] FAT: use toupper/tolower instead of recoding them

On Thu, Dec 13, 2012 at 12:47:36AM -0000, Richard Genoud wrote:

...

toupper/tolower function are already declared, so use them.

Signed-off-by: Richard Genoud richard.genoud@gmail.com Acked-by: Marek Vasut marex@denx.de Acked-by: Stefano Babic sbabic@denx.de

Applied to u-boot/master, thanks!

-- Tom

4475

Age (days ago)

4537

Last active (days ago)

List overview

Download

23 comments

4 participants

tags (0)

participants (4)

Marek Vasut
Richard Genoud
Stefano Babic
Tom Rini