Hi Stefan, On 13 August 2017 at 17:01, Stefan Bruens <stefan.bru...@rwth-aachen.de> wrote: > On Sonntag, 13. August 2017 23:36:57 CEST Simon Glass wrote: >> 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.. > > You might have forgotten, but I sent a quite large initial implementation of > pytest fstests a year ago. These where largely rejected, as these still > depends on the ability to run as run to create the images.
Run as root? I don't see a problem with that (e.g. to use sudo). Some tests require this. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
