On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <s...@chromium.org> wrote:
> Hi Rob,
> On 3 August 2017 at 13:36, Rob Clark <robdcl...@gmail.com> wrote:
>> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan
>> <stefan.bru...@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 <robdcl...@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.

Hmm, good question.  I'm not super-familiar with how the test
framework works.. I guess it would need something exposed in u-boot
scripting environment?  I suppose I could implement a ls2 command that
does the same thing as ls but using fs_readdir()?

U-Boot mailing list

Reply via email to