[U-Boot] [PATCH] fs: add fs_readdir()
sjg at chromium.org
Sun Aug 6 05:16:36 UTC 2017
On 3 August 2017 at 13:36, Rob Clark <robdclark at gmail.com> wrote:
> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan
> <Stefan.Bruens at 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 at 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(-)
>> 1. The current code is already much to convoluted. Your changes add to this
> 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
> 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()).
>> 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,
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.
More information about the U-Boot