[U-Boot] [PATCH v2 4/8] fs: add fs_readdir()
Rob Clark
robdclark at gmail.com
Tue Sep 5 10:48:59 UTC 2017
On Tue, Sep 5, 2017 at 4:56 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Rob,
>
> On 3 September 2017 at 23:16, Łukasz Majewski <lukma at denx.de> wrote:
>> On 09/02/2017 06:37 PM, 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.
>>>
>>> Modelled after POSIX opendir()/readdir()/closedir(). Unlike the other
>>> fs APIs, this is stateful (ie. state is held in the FS_DIR "directory
>>> stream"), to avoid re-traversing of the directory structure at each
>>> step. The directory stream must be released with closedir() when it
>>> is no longer needed.
>>>
>>
>> Reviewed-by: Łukasz Majewski <lukma at denx.de>
>>
>>
>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>> ---
>>> disk/part.c | 31 ++++++++++++--------
>>> fs/fs.c | 91
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> include/fs.h | 55 +++++++++++++++++++++++++++++++++++
>>> include/part.h | 4 +++
>>> 4 files changed, 169 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/disk/part.c b/disk/part.c
>>> index c67fdacc79..aa9183d696 100644
>>> --- a/disk/part.c
>>> +++ b/disk/part.c
>>> @@ -331,6 +331,24 @@ int part_get_info(struct blk_desc *dev_desc, int
>>> part,
>>> return -1;
>>> }
>>> +int part_get_info_whole_disk(struct blk_desc *dev_desc,
>>> disk_partition_t *info)
>>> +{
>>> + info->start = 0;
>>> + info->size = dev_desc->lba;
>>> + info->blksz = dev_desc->blksz;
>>> + info->bootable = 0;
>>> + strcpy((char *)info->type, BOOT_PART_TYPE);
>>> + strcpy((char *)info->name, "Whole Disk");
>>> +#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
>
> Can you use if() instead of #if for this one? And below. It helps to
> reduce the number of code paths at build-time.
I don't think so, at least not without dropping the corresponding
#ifdef in disk_partition_t
>>> + info->uuid[0] = 0;
>>> +#endif
>>> +#ifdef CONFIG_PARTITION_TYPE_GUID
>
> Here too. And below.
same thing
>>> + info->type_guid[0] = 0;
>>> +#endif
>>> +
>>> + return 0;
>>> +}
>>> +
>>> int blk_get_device_by_str(const char *ifname, const char
>>> *dev_hwpart_str,
>>> struct blk_desc **dev_desc)
>>> {
>>> @@ -523,18 +541,7 @@ int blk_get_device_part_str(const char *ifname, const
>>> char *dev_part_str,
>>> (*dev_desc)->log2blksz = LOG2((*dev_desc)->blksz);
>>> - info->start = 0;
>>> - info->size = (*dev_desc)->lba;
>>> - info->blksz = (*dev_desc)->blksz;
>>> - info->bootable = 0;
>>> - strcpy((char *)info->type, BOOT_PART_TYPE);
>>> - strcpy((char *)info->name, "Whole Disk");
>>> -#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
>>> - info->uuid[0] = 0;
>>> -#endif
>>> -#ifdef CONFIG_PARTITION_TYPE_GUID
>>> - info->type_guid[0] = 0;
>>> -#endif
>>> + part_get_info_whole_disk(*dev_desc, info);
>>> ret = 0;
>>> goto cleanup;
>>> diff --git a/fs/fs.c b/fs/fs.c
>>> index 13cd3626c6..441c880654 100644
>>> --- a/fs/fs.c
>>> +++ b/fs/fs.c
>>> @@ -21,6 +21,7 @@
>>> DECLARE_GLOBAL_DATA_PTR;
>>> static struct blk_desc *fs_dev_desc;
>>> +static int fs_dev_part;
>>> static disk_partition_t fs_partition;
>>> static int fs_type = FS_TYPE_ANY;
>>> @@ -69,6 +70,11 @@ static inline int fs_uuid_unsupported(char *uuid_str)
>>> return -1;
>>> }
>>> +static inline int fs_opendir_unsupported(const char *filename, FS_DIR
>>> **dirp)
>>> +{
>>> + return -EACCES;
>>> +}
>>> +
>>> struct fstype_info {
>>> int fstype;
>>> char *name;
>>> @@ -92,6 +98,9 @@ struct fstype_info {
>>> loff_t len, loff_t *actwrite);
>>> void (*close)(void);
>>> int (*uuid)(char *uuid_str);
>>> + int (*opendir)(const char *filename, FS_DIR **dirp);
>>> + int (*readdir)(FS_DIR *dirp);
>>> + void (*closedir)(FS_DIR *dirp);
>
> Please comment these. Also can you use struct instead of typedef?
typedef-struct-caps here was based on how posix readdir() works. I
guess we can deviate, it isn't 100% clone of readdir().. but I figured
it was more clear to be more similar to posix.
>>> };
>>> static struct fstype_info fstypes[] = {
>>> @@ -112,6 +121,7 @@ static struct fstype_info fstypes[] = {
>>> .write = fs_write_unsupported,
>>> #endif
>>> .uuid = fs_uuid_unsupported,
>>> + .opendir = fs_opendir_unsupported,
>>> },
>>> #endif
>>> #ifdef CONFIG_FS_EXT4
>>> @@ -131,6 +141,7 @@ static struct fstype_info fstypes[] = {
>>> .write = fs_write_unsupported,
>>> #endif
>>> .uuid = ext4fs_uuid,
>>> + .opendir = fs_opendir_unsupported,
>>> },
>>> #endif
>>> #ifdef CONFIG_SANDBOX
>>> @@ -146,6 +157,7 @@ static struct fstype_info fstypes[] = {
>>> .read = fs_read_sandbox,
>>> .write = fs_write_sandbox,
>>> .uuid = fs_uuid_unsupported,
>>> + .opendir = fs_opendir_unsupported,
>>> },
>>> #endif
>>> #ifdef CONFIG_CMD_UBIFS
>>> @@ -161,6 +173,7 @@ static struct fstype_info fstypes[] = {
>>> .read = ubifs_read,
>>> .write = fs_write_unsupported,
>>> .uuid = fs_uuid_unsupported,
>>> + .opendir = fs_opendir_unsupported,
>>> },
>>> #endif
>>> {
>>> @@ -175,6 +188,7 @@ static struct fstype_info fstypes[] = {
>>> .read = fs_read_unsupported,
>>> .write = fs_write_unsupported,
>>> .uuid = fs_uuid_unsupported,
>>> + .opendir = fs_opendir_unsupported,
>>> },
>>> };
>>> @@ -228,6 +242,31 @@ int fs_set_blk_dev(const char *ifname, const char
>>> *dev_part_str, int fstype)
>>> if (!info->probe(fs_dev_desc, &fs_partition)) {
>>> fs_type = info->fstype;
>>> + fs_dev_part = part;
>>> + return 0;
>>> + }
>>> + }
>>> +
>>> + return -1;
>>> +}
>>> +
>>> +/* set current blk device w/ blk_desc + partition # */
>>> +int fs_set_blk_dev2(struct blk_desc *desc, int part)
>
> Please add a full function comment in the header file. See
> fs_set_blk() which has a comment.
>
> Also '2' is a bad name. How about a _with_part suffix, or something like that?
yeah, "2" was me running out of creativity.. _with_part sounds reasonable.
> [...]
>
>>> diff --git a/include/fs.h b/include/fs.h
>>> index 2f2aca8378..0a6a366078 100644
>>> --- a/include/fs.h
>>> +++ b/include/fs.h
>>> @@ -26,6 +26,8 @@
>>> */
>>> int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int
>>> fstype);
>>> +int fs_set_blk_dev2(struct blk_desc *desc, int part);
>
> Comment goes above this.
>
>>> +
>>> /*
>>> * Print the list of files on the partition previously set by
>>> fs_set_blk_dev(),
>>> * in directory "dirname".
>>> @@ -78,6 +80,59 @@ int fs_read(const char *filename, ulong addr, loff_t
>>> offset, loff_t len,
>>> int fs_write(const char *filename, ulong addr, loff_t offset, loff_t
>>> len,
>>> loff_t *actwrite);
>>> +/* Add additional FS_DT_* as supported by additional filesystems:*/
>>> +#define FS_DT_DIR 0x4 /* directory */
>>> +#define FS_DT_REG 0x8 /* regular file */
>>> +
>>> +/*
>>> + * A directory entry.
>>> + */
>>> +struct fs_dirent {
>>> + unsigned type; /* one of FS_DT_* */
>
> Is that a mask (so both can be set) or something else? If it is a mask
> please say so.
It is not a mask, despite how the corresponding readdir() DT_* are
defined. They match DT_* constants, or rather the subset of them that
make sense (ie. we don't have pipes or sockets.. maybe at some point
when someone implements this for ext4 it would be worth adding
FS_DT_LNK for symlinks)
BR,
-R
>>> + loff_t size;
>
> comment for this. Is it size of the file in bytes?
>
>>> + char name[256];
>>> +};
>>> +
>>> +typedef struct _FS_DIR FS_DIR;
>>> +
>
> Please drop the typedef as it doesn't seem necessary
>
>>> +/*
>>> + * fs_opendir - Open a directory
>>> + *
>>> + * @filename: the path to directory to open
>>> + * @return a pointer to the directory stream or NULL on error and errno
>>> + * set appropriately
>>> + */
>>> +FS_DIR *fs_opendir(const char *filename);
>>> +
>>> +/*
>>> + * fs_readdir - Read the next directory entry in the directory stream.
>>> + *
>>> + * @dirp: the directory stream
>>> + * @return the next directory entry (only valid until next fs_readdir()
>>> or
>>> + * fs_closedir() call, do not attempt to free()) or NULL if the end of
>>> + * the directory is reached.
>>> + */
>>> +struct fs_dirent *fs_readdir(FS_DIR *dirp);
>>> +
>>> +/*
>>> + * fs_closedir - close a directory stream
>>> + *
>>> + * @dirp: the directory stream
>>> + */
>>> +void fs_closedir(FS_DIR *dirp);
>>> +
>>> +/*
>>> + * private to fs implementations, would be in fs.c but we need to let
>>> + * implementations subclass:
>>> + */
>>> +
>>> +struct _FS_DIR {
>
> Lower case for struct names. Also please add struct member comments.
>
>>> + struct fs_dirent dirent;
>>> + /* private to fs layer: */
>>> + struct blk_desc *desc;
>>> + int part;
>>> +};
>>> +
>>> /*
>>> * Common implementation for various filesystem commands, optionally
>>> limited
>>> * to a specific filesystem type via the fstype parameter.
>>> diff --git a/include/part.h b/include/part.h
>>> index 0cd803a933..48e8ff6d8a 100644
>>> --- a/include/part.h
>>> +++ b/include/part.h
>>> @@ -98,6 +98,7 @@ int host_get_dev_err(int dev, struct blk_desc
>>> **blk_devp);
>>> /* disk/part.c */
>>> int part_get_info(struct blk_desc *dev_desc, int part, disk_partition_t
>>> *info);
>>> +int part_get_info_whole_disk(struct blk_desc *dev_desc, disk_partition_t
>>> *info);
>
> Please fully comment new functions here.
>
>>> void part_print(struct blk_desc *dev_desc);
>>> void part_init(struct blk_desc *dev_desc);
>>> void dev_print(struct blk_desc *dev_desc);
>>> @@ -203,6 +204,9 @@ static inline struct blk_desc *mg_disk_get_dev(int
>>> dev) { return NULL; }
>>> static inline int part_get_info(struct blk_desc *dev_desc, int part,
>>> disk_partition_t *info) { return -1; }
>>> +static inline int part_get_info_whole_disk(struct blk_desc *dev_desc,
>>> + disk_partition_t *info)
>>> +{ return -1; }
>>> static inline void part_print(struct blk_desc *dev_desc) {}
>>> static inline void part_init(struct blk_desc *dev_desc) {}
>>> static inline void dev_print(struct blk_desc *dev_desc) {}
>>>
>
> Regards,
> Simon
More information about the U-Boot
mailing list