[U-Boot] [PATCH v0 01/20] fs: add fs_readdir()

[Rob Clark]

On Mon, Aug 7, 2017 at 2:19 PM, Brüns, Stefan
<Stefan.Bruens at rwth-aachen.de> wrote:
> On Freitag, 4. August 2017 21:31:43 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.
>>
>> For reference, the expected EFI semantics are described in (v2.7 of UEFI
>> spec) in section 13.5 (page 609).  Or for convenience, see:
>>
>>   http://wiki.phoenix.com/wiki/index.php/EFI_FILE_PROTOCOL#Read.28.29
>>
>> The EFI level semantics are implemented in a later patch, so they are
>> not too important to the understanding of this patch.
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>>  fs/fs.c      | 25 +++++++++++++++++++++++++
>>  include/fs.h | 21 +++++++++++++++++++++
>>  2 files changed, 46 insertions(+)
>
> Still, the commit message is in no way helpful when trying to understand what
> your changes are actually doing.

You were the one that wanted a reference to the relevant EFI protocol,
even though it is not terribly useful for understanding this patch ;-)

> You introduce an arbitrary new API in the filesystem level (you neither expose
> an existing API, nor are you implementing the API requested by EFI, nor
> anything roughly resembling it).

I am exposing the API needed to implement the EFI API.  I am not sure
why you describe it as arbitrary.  I would describe it as posix
readdir() ported to fs's stateless design.  Ie. not quite the same,
neither are any of fs's other APIs.

> The API you expose adds an index-based directory lookup, while EFI wants an
> POSIX-like directory stream. After reading through both the EFI spec and U-
> Boots file system code, its clear you want to have some matching layer between
> the mostly stateless U-Boot filesystem layer and the stateful EFI API.

What EFI wants and the way the u-boot filesystem API works are two
completely different things.  The u-boot fs APIs are stateless.  EFI
is not, not just for directories but also for file read/write.  Please
see patch 16/20.

> Please provide a thorough description why you create this new API in the fs
> layer, state that it is a hack to achieve what you want. If sometime later
> someone else wants to clean this up (both the FAT implementation, and the
> API), she/he should not have to go through all the code.

The fat implementation is a hack, but the API is not.  Ie. it does
exactly what the comment in fs.h describes.  (I might do it
differently if u-boot had a concept of file handles that were
stateful.  But that is a bit of a departure from how u-boot's fs
works.)

BR,
-R