Hi Rob, On 10 August 2017 at 12:13, Rob Clark <robdcl...@gmail.com> wrote: > 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. >> > > So I haven't had a whole lot of luck getting fs-tests.sh working > properly (on master).. > > With the ext4 tests, at some point mounting the loopback image fails, > I end up with this in dmesg: > > EXT4-fs (loop0): ext4_check_descriptors: Checksum for group 0 failed > (50995!=31053) > EXT4-fs (loop0): group descriptors corrupted!
I haven't seen that one before! > > I guess technically I don't need to run ext4 tests, so if I skip those > and just run the fat tests, I still end up with some fails with things > like: > > => fatload host 0:0 0x01000008 ./1MB.file.w2 > ** Unable to read file ./1MB.file.w2 ** > > I'm not sure if this is down to some differences in my environment, or > if these tests just don't get run often? It could be either We should convert this to the pytest framework so that it will be run on each pull request.. > > What I have done is add a ls2 cmd, which implements ls on top of > fs_readdir(), which would at least make testing possible. (And fixed > a few bugs that turns up with some manual testing.) OK, well I don't have any great ideas. Do you have time to convert the test? I just sent patches to convert test/image/test-fit.py > > BR, > -R Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
