| Message ID | 20170804193205.24669-2-robdclark@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
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@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 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). 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. 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. Regards, Stefan
On Mon, Aug 7, 2017 at 2:19 PM, Brüns, Stefan <Stefan.Bruens@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@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
diff --git a/fs/fs.c b/fs/fs.c index 595ff1fe69..5ac4226ece 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -69,6 +69,12 @@ static inline int fs_uuid_unsupported(char *uuid_str) return -1; } +static inline int fs_readdir_unsupported(const char *filename, loff_t offset, + struct fs_dirent *dent) +{ + return -ENXIO; +} + struct fstype_info { int fstype; char *name; @@ -92,6 +98,7 @@ struct fstype_info { loff_t len, loff_t *actwrite); void (*close)(void); int (*uuid)(char *uuid_str); + int (*readdir)(const char *filename, loff_t offset, struct fs_dirent *dent); }; static struct fstype_info fstypes[] = { @@ -112,6 +119,7 @@ static struct fstype_info fstypes[] = { .write = fs_write_unsupported, #endif .uuid = fs_uuid_unsupported, + .readdir = fs_readdir_unsupported, }, #endif #ifdef CONFIG_FS_EXT4 @@ -131,6 +139,7 @@ static struct fstype_info fstypes[] = { .write = fs_write_unsupported, #endif .uuid = ext4fs_uuid, + .readdir = fs_readdir_unsupported, }, #endif #ifdef CONFIG_SANDBOX @@ -146,6 +155,7 @@ static struct fstype_info fstypes[] = { .read = fs_read_sandbox, .write = fs_write_sandbox, .uuid = fs_uuid_unsupported, + .readdir = fs_readdir_unsupported, }, #endif #ifdef CONFIG_CMD_UBIFS @@ -161,6 +171,7 @@ static struct fstype_info fstypes[] = { .read = ubifs_read, .write = fs_write_unsupported, .uuid = fs_uuid_unsupported, + .readdir = fs_readdir_unsupported, }, #endif { @@ -175,6 +186,7 @@ static struct fstype_info fstypes[] = { .read = fs_read_unsupported, .write = fs_write_unsupported, .uuid = fs_uuid_unsupported, + .readdir = fs_readdir_unsupported, }, }; @@ -334,6 +346,19 @@ int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len, return ret; } +int fs_readdir(const char *filename, loff_t offset, struct fs_dirent *dent) +{ + struct fstype_info *info = fs_get_info(fs_type); + int ret; + + memset(dent, 0, sizeof(*dent)); + + ret = info->readdir(filename, offset, dent); + fs_close(); + + return ret; +} + int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int fstype) { diff --git a/include/fs.h b/include/fs.h index 2f2aca8378..71051d7c66 100644 --- a/include/fs.h +++ b/include/fs.h @@ -79,6 +79,27 @@ int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len, loff_t *actwrite); /* + * A directory entry. + */ +struct fs_dirent { + loff_t size; + char name[256]; +}; + +/* + * fs_readdir - Read a directory. + * + * @filename: Name of file to read from + * @offset: The offset into the directory to read, ie. offset of N returns + * the N'th directory entry + * @dent: on success, filled in with directory entry + * @return 0 on success, -ENOTDIR if specified file is not a directory, + * or -ENOENT if offset is beyond last directory entry, or -ENXIO if + * operation is not supported. + */ +int fs_readdir(const char *filename, loff_t offset, struct fs_dirent *dent); + +/* * Common implementation for various filesystem commands, optionally limited * to a specific filesystem type via the fstype parameter. */
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@gmail.com> --- fs/fs.c | 25 +++++++++++++++++++++++++ include/fs.h | 21 +++++++++++++++++++++ 2 files changed, 46 insertions(+)