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