On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan <stefan.bru...@rwth-aachen.de> wrote: > On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: >> Needed to support efi file protocol. The fallback.efi loader wants >> to be able to read the contents of the /EFI directory to find an OS >> to boot. >> >> Currently only implemented for FAT, but that is all that UEFI is >> required to support. >> >> Signed-off-by: Rob Clark <robdcl...@gmail.com> >> --- >> fs/fat/fat.c | 59 >> ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c | >> 25 +++++++++++++++++++++++++ >> include/fat.h | 4 +++- >> include/fs.h | 21 +++++++++++++++++++++ >> 4 files changed, 95 insertions(+), 14 deletions(-) >> > > NAK > > 1. The current code is already much to convoluted. Your changes add to this > significantly
I agree with the first part of that statement, but not the second. The code is pretty awful, but apparently works for people, and I don't know (or have the time to learn) enough about FAT to do a massive re-write. I'll split this patch so we can add the interface without FAT implementation, and I'll just carry around the second part downstream. > 2. Your patch description neither references the exact part of the EFI > specification you want to support (which took me some time, for reference it > is "13.: Protocols - Media Access, 13.5: File Protocol"), nor are you > specifying the required semantics (which is "open", "read", "close", where > each read returns a single directory entry, similar to the POSIX opendir(), > readdir() calls. I can add a note in the commit message.. although I didn't really think it would be too relevant to this patch. (More relevant to the patch which adds the efi_loader part, which depends on this patch.) > 3. Usage of an index too lookup the next entry is also quite convoluted. > > 4. As far as I can see, your code will fail to find files in the root > directory (look for LS_ROOT). You could be right.. nothing ever traverses the root directory. > I think it would be much better to first restructure the current code to use > an readdir like interface internally, and then do everything EFI needs on top. tbh, it would be nice even to implement fs_ls() generically on top of readdir().. although I didn't do that since it would be slower (without a re-write of FAT implementation, since we currently have to re-traverse things for each readdir()). BR, -R > This would get rid of the 4 almost identical copies to print the current > directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the remaining > directory traversal and and also avoid the bug in (4.). > > Kind regards, > > Stefan _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
