On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <s...@chromium.org> wrote: > Hi Rob, > > On 3 August 2017 at 13:36, Rob Clark <robdcl...@gmail.com> wrote: >> 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 > > How can we get some tests for this code? We have fs-tests.sh - perhaps > we should build on that? If it helps I could bring that into the > pytest framework and you could take it from there? > > With tests we at least have the possibility of refactoring later. >
Hmm, good question. I'm not super-familiar with how the test framework works.. I guess it would need something exposed in u-boot scripting environment? I suppose I could implement a ls2 command that does the same thing as ls but using fs_readdir()? BR, -R _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
