On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass sjg@chromium.org wrote:
Hi Rob,
On 3 August 2017 at 13:36, Rob Clark robdclark@gmail.com wrote:
On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan Stefan.Bruens@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 robdclark@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
- 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.
- 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.)
Usage of an index too lookup the next entry is also quite convoluted.
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 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?
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.)
BR, -R