Skip to contentSkip to conversation

FAT: make FAT compile without VFAT

PATCH New Richard Genoud 4 participants 24 messages Dec 4, 2012 – Feb 4, 2013 VFAT/FAT/msdos filesystem

lore
Share & tools
lore

Richard Genoud

author
#0 lore raw

PATCH 1/1 FAT: make FAT compile without VFAT

Signed-off-by Richard Genoud <[email protected]>
---
 fs/fat/fat.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
fs/fat/fat.c +8 -0
index 393c378..defdd74 100644
@@ -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 &&
Signature · 1 line
1.7.2.5
#1 lore raw

PATCH 1/1 FAT: make FAT compile without VFAT

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

author
#2 lore raw

PATCH 1/1 FAT: make FAT compile without VFAT

2012/12/4 Marek Vasut <[email protected]>:

Quoted 3 lines

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);

Quoted 2 lines

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.

Quoted 3 lines

Best regards,
Marek Vasut

Signature · 1 line
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
#3 lore raw

PATCH 1/1 FAT: make FAT compile without VFAT

In reply to Richard Genoud
Quoted 10 lines · Richard Genoud
Signed-off-by Richard Genoud <[email protected]>
---
 fs/fat/fat.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
fs/fat/fat.c +0 -0
index 393c378..defdd74 100644

fs/fat/fat.c static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, · line 594

Quoted 6 lines
fs/fat/fat.c +2 -0
@@ -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

fs/fat/fat.c do_fat_read_at(const char *filename, unsigned long pos, void *buffer, · line 835

Quoted 10 lines
fs/fat/fat.c +2 -0
 	__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

fs/fat/fat.c do_fat_read_at(const char *filename, unsigned long pos, void *buffer, · line 963

Quoted 3 lines
fs/fat/fat.c +2 -0
+#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

Signature · 5 lines
=====================================================================
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 at denx.de
=====================================================================
author
#4 lore raw

cover 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(-)
Signature · 1 line
1.7.2.5
#5 lore raw

cover clean some FAT code

Dear Richard Genoud,

Quoted 5 lines

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

author
#6 lore raw

cover clean some FAT code

2012/12/13 Marek Vasut <[email protected]>:

Quoted 5 lines

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 !

author
#7 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

In reply to Stefano Babic

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 <[email protected]>
---
 fs/fat/fat.c       |   55 +++++++++++++++++++++++++++------------------------
 fs/fat/fat_write.c |   11 ++-------
 2 files changed, 32 insertions(+), 34 deletions(-)
fs/fat/fat.c +29 -26
index 393c378..c79e3e3 100644
@@ -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) {
fs/fat/fat_write.c +3 -8
index 4a1bda0..ed3eaa0 100644
@@ -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))
Signature · 1 line
1.7.2.5
#8 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

Dear Richard Genoud,

Quoted 20 lines

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 <[email protected]>
---
 fs/fat/fat.c       |   55
+++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c |  
11 ++-------
 2 files changed, 32 insertions(+), 34 deletions(-)
fs/fat/fat.c +0 -0
index 393c378..c79e3e3 100644

fs/fat/fat.c line 41

Quoted 8 lines
fs/fat/fat.c +5 -0
@@ -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

author
#9 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

2012/12/13 Marek Vasut <[email protected]>:

Quoted 25 lines

Dear Richard Genoud,

Quoted 23 lines

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 <[email protected]>
---
 fs/fat/fat.c       |   55
+++++++++++++++++++++++++++------------------------ fs/fat/fat_write.c |
11 ++-------
 2 files changed, 32 insertions(+), 34 deletions(-)
fs/fat/fat.c +0 -0
index 393c378..c79e3e3 100644
@@ -34,6 +34,12 @@
 #include <malloc.h>
 #include <linux/compiler.h>
Quoted 8 lines

fs/fat/fat.c line 41

Quoted 5 lines
fs/fat/fat.c +5 -0
+#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.

#10 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

Dear Richard Genoud,

Quoted 38 lines

2012/12/13 Marek Vasut <[email protected]>:

Quoted 37 lines

Dear Richard Genoud,

Quoted 32 lines

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 <[email protected]>

---

 fs/fat/fat.c       |   55

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

 2 files changed, 32 insertions(+), 34 deletions(-)
fs/fat/fat.c +5 -0
index 393c378..c79e3e3 100644
@@ -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

[...]

Quoted 5 lines
Quoted 1 line

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

author
#11 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

2012/12/13 Marek Vasut <[email protected]>:

Quoted 42 lines

Dear Richard Genoud,

Quoted 40 lines

2012/12/13 Marek Vasut <[email protected]>:

Quoted 38 lines

Dear Richard Genoud,

Quoted 23 lines

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 <[email protected]>

---

 fs/fat/fat.c       |   55

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

 2 files changed, 32 insertions(+), 34 deletions(-)
fs/fat/fat.c +0 -0
index 393c378..c79e3e3 100644

fs/fat/fat.c line 42

Quoted 9 lines
fs/fat/fat.c +5 -0
@@ -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 ?

Quoted 8 lines
Quoted 3 lines

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.

#12 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

Dear Richard Genoud,

Quoted 47 lines

2012/12/13 Marek Vasut <[email protected]>:

Quoted 46 lines

Dear Richard Genoud,

Quoted 43 lines

2012/12/13 Marek Vasut <[email protected]>:

Quoted 37 lines

Dear Richard Genoud,

Quoted 32 lines

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 <[email protected]>

---

 fs/fat/fat.c       |   55

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

 2 files changed, 32 insertions(+), 34 deletions(-)
fs/fat/fat.c +5 -0
index 393c378..c79e3e3 100644
@@ -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

[...]

Quoted 1 line

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 ?

Quoted 6 lines
Quoted 4 lines

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

author
#13 lore raw

PATCH v2 1/1 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.

Signed-off-by Richard Genoud <[email protected]>
---
 fs/fat/fat.c       |   55 +++++++++++++++++++++++++++------------------------
 fs/fat/fat_write.c |   11 ++-------
 2 files changed, 32 insertions(+), 34 deletions(-)
fs/fat/fat.c +29 -26
index 393c378..7c23b67 100644
@@ -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);
 }
 
-#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) {
fs/fat/fat_write.c +3 -8
index 4a1bda0..e7cda17 100644
@@ -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))
Signature · 1 line
1.7.2.5
#14 lore raw

PATCH v2 1/1 FAT: remove ifdefs to make the code more readable

Quoted 9 lines · 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 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 <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130204/17ccf778/attachment.pgp>
#15 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

In reply to Richard Genoud

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

Quoted 84 lines · 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 <[email protected]> ---

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

fs/fat/fat.c index 393c378..c79e3e3 renamed from fs/fat/fat.c +0 -0

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; }

fs/fat/fat.c index 393c378..c79e3e3 renamed from fs/fat/fat.c +0 -2
-#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; }

fs/fat/fat.c index 393c378..c79e3e3 renamed from fs/fat/fat.c +0 -1
-			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; }

Quoted 6 lines

-#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
-----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-----

author
#16 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

2012/12/13 Tom Rini <[email protected]>:

Quoted 3 lines

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

#17 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

In reply to Tom Rini

Dear Tom Rini,

[...]

Quoted 2 lines

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

author
#18 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

2012/12/13 Marek Vasut <[email protected]>:

Quoted 11 lines

Dear Tom Rini,

[...]

Quoted 2 lines

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)

#19 lore raw

PATCH 1/2 FAT: remove ifdefs to make the code more readable

Quoted 10 lines · Richard Genoud

2012/12/13 Marek Vasut <[email protected]>:

Quoted 9 lines

Dear Tom Rini,

[...]

Quoted 2 lines

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?

Quoted 7 lines
Quoted 2 lines

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.

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121213/79a23308/attachment.pgp>
author
#20 lore raw

PATCH 2/2 FAT: use toupper/tolower instead of recoding them

In reply to Stefano Babic

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

Signed-off-by Richard Genoud <[email protected]>
---
 fs/fat/fat.c       |    3 ++-
 fs/fat/fat_write.c |    3 ++-
 include/fat.h      |    3 ---
 3 files changed, 4 insertions(+), 5 deletions(-)
fs/fat/fat.c +2 -1
index c79e3e3..cff0526 100644
@@ -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++;
 	}
 }
fs/fat/fat_write.c +2 -1
index ed3eaa0..13c4de1 100644
@@ -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++;
 	}
 }
include/fat.h +0 -3
index 706cd7a..b28c3fd 100644
@@ -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)))
Signature · 1 line
1.7.2.5
#22 lore raw

PATCH 2/2 FAT: use toupper/tolower instead of recoding them

In reply to Richard Genoud
Quoted 57 lines · Richard Genoud

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

Signed-off-by Richard Genoud <[email protected]>
---
 fs/fat/fat.c       |    3 ++-
 fs/fat/fat_write.c |    3 ++-
 include/fat.h      |    3 ---
 3 files changed, 4 insertions(+), 5 deletions(-)
fs/fat/fat.c +2 -1
index c79e3e3..cff0526 100644
@@ -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++;
 	}
 }
fs/fat/fat_write.c +2 -1
index ed3eaa0..13c4de1 100644
@@ -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++;
 	}
 }
include/fat.h +0 -0
index 706cd7a..b28c3fd 100644

include/fat.h line 103

Quoted 9 lines
include/fat.h +0 -3
@@ -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 <[email protected]>

Best regards,
Stefano Babic

Signature · 5 lines
=====================================================================
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 at denx.de
=====================================================================
#23 lore raw

PATCH 2/2 FAT: use toupper/tolower instead of recoding them

In reply to Richard Genoud
Quoted 5 lines · Richard Genoud

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

Signed-off-by Richard Genoud <[email protected]>
Acked-by Marek Vasut <[email protected]>
Acked-by Stefano Babic <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130204/4caeb4b1/attachment.pgp>