Hi Stefan,
On 13 August 2017 at 17:01, Stefan Bruens stefan.bruens@rwth-aachen.de wrote:
On Sonntag, 13. August 2017 23:36:57 CEST Simon Glass wrote:
Hi Rob,
On 10 August 2017 at 12:13, Rob Clark robdclark@gmail.com wrote:
On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass sjg@chromium.org wrote:
Hi Rob,
On 3 August 2017 at 13:36, Rob Clark robdclark@gmail.com wrote:
On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan
Stefan.Bruens@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@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
- 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.
- 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.)
- Usage of an index too lookup the next entry is also quite
convoluted.
- 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.
Run as root?
I don't see a problem with that (e.g. to use sudo). Some tests require this.
Regards, Simon