diff mbox

[U-Boot,v1,02/15] fs/fat: implement readdir

Message ID 20170810182948.27653-3-robdclark@gmail.com
State Deferred
Delegated to: Alexander Graf
Headers show

Commit Message

Rob Clark Aug. 10, 2017, 6:29 p.m. UTC 
Yes, this is super-hacky.  The FAT code is quite ugly, and this doesn't
improve things.  But it doesn't make it significantly worse either.  The
better option would be a massive FAT re-write to get rid of the hacky
way that fat_file_ls() works.  Volunteers welcome.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 fs/fat/fat.c  | 94 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 fs/fs.c       |  2 +-
 include/fat.h |  4 ++-
 3 files changed, 81 insertions(+), 19 deletions(-)

Comments

Alexander Graf Aug. 12, 2017, 12:17 p.m. UTC | #1 
On 10.08.17 20:29, Rob Clark wrote:
> Yes, this is super-hacky.  The FAT code is quite ugly, and this doesn't
> improve things.  But it doesn't make it significantly worse either.  The
> better option would be a massive FAT re-write to get rid of the hacky
> way that fat_file_ls() works.  Volunteers welcome.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

What concerns me the most in patch 1/15 and this patch is the limited 
scope. Yes, you make readdir work for FAT, but all other file systems 
are still unimplemented. In fact, they're even all still implementing 
their own hand-written ls logic.

One of the goals of the efi_loader code is to integrate with U-Boot as 
much as possible, to reuse code where we can. And if current interfaces 
are terrible, I think it's ok to just replace them for something that 
fits everyone's needs better.

How feasible do you think it would be to implement an ls function based 
on readdir and just convert all file systems to it, completely replacing 
the current (quite crude) ls logic?


Alex
Rob Clark Aug. 12, 2017, 2:04 p.m. UTC | #2 
On Sat, Aug 12, 2017 at 8:17 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 10.08.17 20:29, Rob Clark wrote:
>>
>> Yes, this is super-hacky.  The FAT code is quite ugly, and this doesn't
>> improve things.  But it doesn't make it significantly worse either.  The
>> better option would be a massive FAT re-write to get rid of the hacky
>> way that fat_file_ls() works.  Volunteers welcome.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>
>
> What concerns me the most in patch 1/15 and this patch is the limited scope.
> Yes, you make readdir work for FAT, but all other file systems are still
> unimplemented. In fact, they're even all still implementing their own
> hand-written ls logic.
>
> One of the goals of the efi_loader code is to integrate with U-Boot as much
> as possible, to reuse code where we can. And if current interfaces are
> terrible, I think it's ok to just replace them for something that fits
> everyone's needs better.
>
> How feasible do you think it would be to implement an ls function based on
> readdir and just convert all file systems to it, completely replacing the
> current (quite crude) ls logic?

So I went ahead and re-wrote the fat directory traversal[1].  I should
be posting to list in the next day or two but still want to make a few
small cleanups.  (And to get rid of some hacks in efi_file, I think I
need to add an fs_isdir() too :-/)

As far as the various other filesys's, I agree that a generic ls would
be a nice goal.  But the scope of the efi_loader patchset has already
expanded way too much, and at this point I'm pretty much limited by
what I can finish this weekend.  At the end of the day, FAT is all
that UEFI expects, so I think it is fine to let the other filesystems
catch up on their own schedule.

I could write a generic ls helper, and just plug it in for FAT, which
could be re-used later when other filesys's gain readdir support.

BR,
-R

[1] https://github.com/robclark/u-boot/commits/readdir
Alexander Graf Aug. 12, 2017, 5:22 p.m. UTC | #3 
> Am 12.08.2017 um 16:04 schrieb Rob Clark <robdclark@gmail.com>:
> 
>> On Sat, Aug 12, 2017 at 8:17 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>>> On 10.08.17 20:29, Rob Clark wrote:
>>> 
>>> Yes, this is super-hacky.  The FAT code is quite ugly, and this doesn't
>>> improve things.  But it doesn't make it significantly worse either.  The
>>> better option would be a massive FAT re-write to get rid of the hacky
>>> way that fat_file_ls() works.  Volunteers welcome.
>>> 
>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> 
>> 
>> What concerns me the most in patch 1/15 and this patch is the limited scope.
>> Yes, you make readdir work for FAT, but all other file systems are still
>> unimplemented. In fact, they're even all still implementing their own
>> hand-written ls logic.
>> 
>> One of the goals of the efi_loader code is to integrate with U-Boot as much
>> as possible, to reuse code where we can. And if current interfaces are
>> terrible, I think it's ok to just replace them for something that fits
>> everyone's needs better.
>> 
>> How feasible do you think it would be to implement an ls function based on
>> readdir and just convert all file systems to it, completely replacing the
>> current (quite crude) ls logic?
> 
> So I went ahead and re-wrote the fat directory traversal[1].  I should
> be posting to list in the next day or two but still want to make a few
> small cleanups.  (And to get rid of some hacks in efi_file, I think I
> need to add an fs_isdir() too :-/)
> 
> As far as the various other filesys's, I agree that a generic ls would
> be a nice goal.  But the scope of the efi_loader patchset has already
> expanded way too much, and at this point I'm pretty much limited by
> what I can finish this weekend.  At the end of the day, FAT is all
> that UEFI expects, so I think it is fine to let the other filesystems
> catch up on their own schedule.
> 
> I could write a generic ls helper, and just plug it in for FAT, which
> could be re-used later when other filesys's gain readdir support.

That at least sounds much nicer than duplicating ls functionality and moves us into the right direction.


Thanks!

Alex
diff mbox

Patch

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 9ad18f96ff..3d5dde0d9e 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -14,6 +14,7 @@ 
 #include <config.h>
 #include <exports.h>
 #include <fat.h>
+#include <fs.h>
 #include <asm/byteorder.h>
 #include <part.h>
 #include <malloc.h>
@@ -575,17 +576,25 @@  static __u8 mkcksum(const char name[8], const char ext[3])
 /*
  * Get the directory entry associated with 'filename' from the directory
  * starting at 'startsect'
+ *
+ * Last two args are only used for dols==LS_READDIR
  */
 __u8 get_dentfromdir_block[MAX_CLUSTSIZE]
 	__aligned(ARCH_DMA_MINALIGN);
 
-static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
-				  char *filename, dir_entry *retdent,
-				  int dols)
+static dir_entry *get_dentfromdir(fsdata *mydata, char *filename,
+				  dir_entry *retdent, int dols,
+				  loff_t pos, struct fs_dirent *d)
 {
 	__u16 prevcksum = 0xffff;
 	__u32 curclust = START(retdent);
 	int files = 0, dirs = 0;
+	int readdir = 0;
+
+	if (dols == LS_READDIR) {
+		readdir = 1;
+		dols = 0;
+	}
 
 	debug("get_dentfromdir: %s\n", filename);
 
@@ -618,7 +627,7 @@  static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 					get_vfatname(mydata, curclust,
 						     get_dentfromdir_block,
 						     dentptr, l_name);
-					if (dols) {
+					if (dols || readdir) {
 						int isdir;
 						char dirc;
 						int doit = 0;
@@ -637,7 +646,14 @@  static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 							}
 						}
 						if (doit) {
-							if (dirc == ' ') {
+							if (readdir) {
+								if ((dirs + files - 1) == pos) {
+									strcpy(d->name, l_name);
+									if (!isdir)
+										d->size = FAT2CPU32(dentptr->size);
+									return NULL;
+								}
+							} else if (dirc == ' ') {
 								printf(" %8u   %s%c\n",
 								       FAT2CPU32(dentptr->size),
 									l_name,
@@ -668,7 +684,7 @@  static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 			}
 			if (vfat_enabled) {
 				__u8 csum = mkcksum(dentptr->name, dentptr->ext);
-				if (dols && csum == prevcksum) {
+				if ((dols || readdir) && csum == prevcksum) {
 					prevcksum = 0xffff;
 					dentptr++;
 					continue;
@@ -676,7 +692,7 @@  static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 			}
 
 			get_name(dentptr, s_name);
-			if (dols) {
+			if (dols || readdir) {
 				int isdir = (dentptr->attr & ATTR_DIR);
 				char dirc;
 				int doit = 0;
@@ -694,7 +710,14 @@  static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
 				}
 
 				if (doit) {
-					if (dirc == ' ') {
+					if (readdir) {
+						if ((dirs + files - 1) == pos) {
+							strcpy(d->name, s_name);
+							if (!isdir)
+								d->size = FAT2CPU32(dentptr->size);
+							return NULL;
+						}
+					} else if (dirc == ' ') {
 						printf(" %8u   %s%c\n",
 						       FAT2CPU32(dentptr->size),
 							s_name, dirc);
@@ -825,13 +848,14 @@  int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 	__u32 cursect;
 	int idx, isdir = 0;
 	int files = 0, dirs = 0;
-	int ret = -1;
+	int ret = (dols == LS_READDIR) ? -ENOTDIR : -1;
 	int firsttime;
 	__u32 root_cluster = 0;
 	__u32 read_blk;
 	int rootdir_size = 0;
 	int buffer_blk_cnt;
 	int do_read;
+	int readdir = (dols == LS_READDIR);
 	__u8 *dir_ptr;
 
 	if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
@@ -1012,7 +1036,16 @@  root_reparse:
 							}
 						}
 						if (doit) {
-							if (dirc == ' ') {
+							if (readdir) {
+								if ((dirs + files - 1) == pos) {
+									struct fs_dirent *d = buffer;
+									strcpy(d->name, l_name);
+									if (!isdir)
+										d->size = FAT2CPU32(dentptr->size);
+									ret = 0;
+									goto exit;
+								}
+							} else if (dirc == ' ') {
 								printf(" %8u   %s%c\n",
 								       FAT2CPU32(dentptr->size),
 									l_name,
@@ -1035,7 +1068,9 @@  root_reparse:
 				}
 			} else if (dentptr->name[0] == 0) {
 				debug("RootDentname == NULL - %d\n", i);
-				if (dols == LS_ROOT) {
+				if (readdir) {
+					ret = -ENOENT;
+				} else if (dols == LS_ROOT) {
 					printf("\n%d file(s), %d dir(s)\n\n",
 						files, dirs);
 					ret = 0;
@@ -1070,7 +1105,16 @@  root_reparse:
 					}
 				}
 				if (doit) {
-					if (dirc == ' ') {
+					if (readdir) {
+						if ((dirs + files - 1) == pos) {
+							struct fs_dirent *d = buffer;
+							strcpy(d->name, s_name);
+							if (!isdir)
+								d->size = FAT2CPU32(dentptr->size);
+							ret = 0;
+							goto exit;
+						}
+					} else if (dirc == ' ') {
 						printf(" %8u   %s%c\n",
 						       FAT2CPU32(dentptr->size),
 							s_name, dirc);
@@ -1138,7 +1182,9 @@  root_reparse:
 
 		/* If end of rootdir reached */
 		if (rootdir_end) {
-			if (dols == LS_ROOT) {
+			if (readdir) {
+				ret = -ENOENT;
+			} else if (dols == LS_ROOT) {
 				printf("\n%d file(s), %d dir(s)\n\n",
 				       files, dirs);
 				*size = 0;
@@ -1150,9 +1196,12 @@  rootdir_done:
 
 	firsttime = 1;
 
+	if (readdir && dols == LS_ROOT) {
+		ret = -ENOENT;
+		goto exit;
+	}
+
 	while (isdir) {
-		int startsect = mydata->data_begin
-			+ START(dentptr) * mydata->clust_size;
 		dir_entry dent;
 		char *nextname = NULL;
 
@@ -1177,10 +1226,14 @@  rootdir_done:
 			}
 		}
 
-		if (get_dentfromdir(mydata, startsect, subname, dentptr,
-				     isdir ? 0 : dols) == NULL) {
+		if (get_dentfromdir(mydata, subname, dentptr,
+				    isdir ? 0 : dols, pos, buffer) == NULL) {
 			if (dols && !isdir)
 				*size = 0;
+			if (dols == LS_READDIR) {
+				struct fs_dirent *dent = buffer;
+				ret = dent->name[0] ? 0 : -ENOENT;
+			}
 			goto exit;
 		}
 
@@ -1353,6 +1406,13 @@  int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
 	return ret;
 }
 
+int fat_readdir(const char *filename, loff_t offset, struct fs_dirent *dent)
+{
+	loff_t actread;
+	return do_fat_read_at(filename, offset, dent, sizeof(*dent),
+			      LS_READDIR, 0, &actread);
+}
+
 void fat_close(void)
 {
 }
diff --git a/fs/fs.c b/fs/fs.c
index 5720ceec49..d6a2cdb22f 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -119,7 +119,7 @@  static struct fstype_info fstypes[] = {
 		.write = fs_write_unsupported,
 #endif
 		.uuid = fs_uuid_unsupported,
-		.readdir = fs_readdir_unsupported,
+		.readdir = fat_readdir,
 	},
 #endif
 #ifdef CONFIG_FS_EXT4
diff --git a/include/fat.h b/include/fat.h
index 71879f01ca..0ef3f5be16 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -61,8 +61,8 @@ 
 /* Flags telling whether we should read a file or list a directory */
 #define LS_NO		0
 #define LS_YES		1
-#define LS_DIR		1
 #define LS_ROOT		2
+#define LS_READDIR	3	/* read directory entry at specified offset */
 
 #define ISDIRDELIM(c)	((c) == '/' || (c) == '\\')
 
@@ -210,5 +210,7 @@  int file_fat_write(const char *filename, void *buf, loff_t offset, loff_t len,
 		   loff_t *actwrite);
 int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
 		  loff_t *actread);
+struct fs_dirent;
+int fat_readdir(const char *filename, loff_t offset, struct fs_dirent *dir);
 void fat_close(void);
 #endif /* _FAT_H_ */