[U-Boot] [PATCH] fs: add fs_readdir()

[Simon Glass]

Hi Rob,

On 10 August 2017 at 12:13, Rob Clark <robdclark at gmail.com> wrote:
> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Rob,
>>
>> 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(-)
>>>>>
>>>>
>>>> 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