Tracking Issue: high-level FS abstraction

[phip1611]

We want a high-level FS abstraction to access UEFI volumes based on the (Simple)FileSystemProtocol. This is a large portion of work and split into multiple tasks/PRs tracked in this issue:

• initial fs module - Introducing a high-level FS abstraction #472
  • minimal public API (just errors and fs functions but no path abstraction)
  • users use &str as path type
  • module API close to std::fs
  • integration test
• discuss and implement a nice Path abstraction - uefi/fs: add path and pathbuf abstraction #771
  • will there be Path and PathBuf?
  • users should pass a &str as well as PathType to all functions
    -> param is something like: Into<PathType>
  • what will be part of public API?
• implement fs::remove_dir_all method that is missing in the initial merge request - fs: implement remove_dir_all #799
• check public API again after all of the above was merged (before the first release containing the new functionality)
• check the integration test again: are all corner-cases and valid use-cases covered?

[phip1611]

My thoughts regarding a Path abstraction

In the standard library, a Path is a wrapper around &OsStr and PathBuf around OsString. In uefi-rs, we have to model this with &CStr16 respectively CString16. Unlike as on Windows or Unix, we won't be able to transform &str easily to &CStr16. So, accepting P: AsRef<Path> is no option for us. Instead, I think that every method in uefi::fs::FileSystem should accept a P: Into<PathBuf>. The extra allocations (even when passing in a valid uefi string) to me, as:

• the API is simpler this way
• micro performance optimizations doesn't matter at all on this level with the typical use-cases of an EFI app in mind

Additionally, for simplicity, I think it will be totally sufficient to only accept absolute paths (the leading \\ will be omitted as we pretend that the present working directory is the root) and also do not allow . and .. for simplicity. I don't think that developers need them.

[phip1611]

• check public API again after all of the above was merged

Can you please give me your thoughts about it? @nicholasbishop

[nicholasbishop]

I think it's looking good. I'm sure we'll discover some things to tweak here and there given that it's a large new API, but I wouldn't have any objection to releasing it in it's current form.

[phip1611]

• check the integration test again: are all corner-cases and valid use-cases covered?

@nicholasbishop can you please take a look and then close this issue?

If everything is good, do we want to ship a new release soon?

[medhefgo]

I'll just quickly mention here that any fs abstractions here should be aware of broken firmware/drivers and should probably employ some quirks to work around them:

• large read buffers breaking the file handle in weird ways (even when reopened)¹
• Read() on directories can still advance the read position when the buffer is too small²
• Read() on directories may not return needed buffer size at all ³

Footnotes

1. https://github.com/systemd/systemd/issues/25911 ↩

2. https://github.com/systemd/systemd/issues/22073 ↩

3. https://github.com/rhboot/shim/issues/558 ↩

[phip1611]

broken firmware/drivers

Dear lord, I always asked myself when it's the first time that we have to cope with broken firmware in this crate 😁

Thanks for the pointers!

Read() on directories can still advance the read position when the buffer is too small

I think that shouldn't be a problem in our case as we always pass in a correctly sized buffer.

[phip1611]

I close this issue. The initial major work is finished. For further adjustments, we can have dedicated issues.