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

[Stefan Bruens]

On Sonntag, 13. August 2017 23:36:57 CEST Simon Glass wrote:
> 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..

You might have forgotten, but I sent a quite large initial implementation of 
pytest fstests a year ago. These where largely rejected, as these still 
depends on the ability to run as run to create the images.

Regards,

Stefan


-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019