Hi Rob,
On 3 September 2017 at 23:16, Łukasz Majewski <lu...@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 <lu...@denx.de>
>
>
>> Signed-off-by: Rob Clark <robdcl...@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.
>> + info->uuid[0] = 0;
>> +#endif
>> +#ifdef CONFIG_PARTITION_TYPE_GUID
Here too. And below.
>> + 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?
>> };
>> 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?
[...]
>> 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.
>> + 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
