[PATCH 0/5] fs: ext4: implement opendir, readdir, closedir
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
A memory leak in ext4fs_exists is resolved.
ext4fs_iterate_dir is simplified by removing a redundant pointer copy.
Heinrich Schuchardt (5): fs: ext4: simplify ext4fs_iterate_dir() fs: ext4: free directory node in ext4fs_exists() fs: ext4: implement opendir, readdir, closedir efi_loader: fix GetInfo and SetInfo fs: ext4: use fs_ls_generic
fs/ext4/ext4_common.c | 48 ++------ fs/ext4/ext4fs.c | 177 +++++++++++++++++++++++++--- fs/fs.c | 6 +- include/ext4fs.h | 4 + lib/efi_loader/efi_file.c | 30 +++-- test/py/tests/test_env.py | 2 +- test/py/tests/test_fs/test_basic.py | 5 +- 7 files changed, 197 insertions(+), 75 deletions(-)
Remove copying a pointer with a cast to the very same type.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- fs/ext4/ext4_common.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 76f7102456e..c1e38978bb9 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2046,24 +2046,23 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, unsigned int fpos = 0; int status; loff_t actread; - struct ext2fs_node *diro = (struct ext2fs_node *) dir;
#ifdef DEBUG if (name != NULL) printf("Iterate dir %s\n", name); #endif /* of DEBUG */ - if (!diro->inode_read) { - status = ext4fs_read_inode(diro->data, diro->ino, &diro->inode); + if (!dir->inode_read) { + status = ext4fs_read_inode(dir->data, dir->ino, &dir->inode); if (status == 0) return 0; } /* Search the file. */ - while (fpos < le32_to_cpu(diro->inode.size)) { + while (fpos < le32_to_cpu(dir->inode.size)) { struct ext2_dirent dirent;
- status = ext4fs_read_file(diro, fpos, - sizeof(struct ext2_dirent), - (char *)&dirent, &actread); + status = ext4fs_read_file(dir, fpos, + sizeof(struct ext2_dirent), + (char *)&dirent, &actread); if (status < 0) return 0;
@@ -2077,7 +2076,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, struct ext2fs_node *fdiro; int type = FILETYPE_UNKNOWN;
- status = ext4fs_read_file(diro, + status = ext4fs_read_file(dir, fpos + sizeof(struct ext2_dirent), dirent.namelen, filename, @@ -2089,7 +2088,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, if (!fdiro) return 0;
- fdiro->data = diro->data; + fdiro->data = dir->data; fdiro->ino = le32_to_cpu(dirent.inode);
filename[dirent.namelen] = '\0'; @@ -2104,7 +2103,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, else if (dirent.filetype == FILETYPE_REG) type = FILETYPE_REG; } else { - status = ext4fs_read_inode(diro->data, + status = ext4fs_read_inode(dir->data, le32_to_cpu (dirent.inode), &fdiro->inode); @@ -2140,10 +2139,10 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, } } else { if (fdiro->inode_read == 0) { - status = ext4fs_read_inode(diro->data, - le32_to_cpu( - dirent.inode), - &fdiro->inode); + status = ext4fs_read_inode(dir->data, + le32_to_cpu( + dirent.inode), + &fdiro->inode); if (status == 0) { free(fdiro); return 0;
Hi
On Sat, Oct 26, 2024 at 8:41 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Remove copying a pointer with a cast to the very same type.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4_common.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 76f7102456e..c1e38978bb9 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2046,24 +2046,23 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, unsigned int fpos = 0; int status; loff_t actread;
struct ext2fs_node *diro = (struct ext2fs_node *) dir;#ifdef DEBUG if (name != NULL) printf("Iterate dir %s\n", name); #endif /* of DEBUG */
if (!diro->inode_read) {status = ext4fs_read_inode(diro->data, diro->ino, &diro->inode);
if (!dir->inode_read) {status = ext4fs_read_inode(dir->data, dir->ino, &dir->inode); if (status == 0) return 0; } /* Search the file. */
while (fpos < le32_to_cpu(diro->inode.size)) {
while (fpos < le32_to_cpu(dir->inode.size)) { struct ext2_dirent dirent;
status = ext4fs_read_file(diro, fpos,sizeof(struct ext2_dirent),(char *)&dirent, &actread);
status = ext4fs_read_file(dir, fpos,sizeof(struct ext2_dirent),(char *)&dirent, &actread); if (status < 0) return 0;@@ -2077,7 +2076,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, struct ext2fs_node *fdiro; int type = FILETYPE_UNKNOWN;
status = ext4fs_read_file(diro,
status = ext4fs_read_file(dir, fpos + sizeof(struct ext2_dirent), dirent.namelen, filename,@@ -2089,7 +2088,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, if (!fdiro) return 0;
fdiro->data = diro->data;
fdiro->data = dir->data; fdiro->ino = le32_to_cpu(dirent.inode); filename[dirent.namelen] = '\0';@@ -2104,7 +2103,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, else if (dirent.filetype == FILETYPE_REG) type = FILETYPE_REG; } else {
status = ext4fs_read_inode(diro->data,
status = ext4fs_read_inode(dir->data, le32_to_cpu (dirent.inode), &fdiro->inode);@@ -2140,10 +2139,10 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, } } else { if (fdiro->inode_read == 0) {
status = ext4fs_read_inode(diro->data,le32_to_cpu(dirent.inode),&fdiro->inode);
status = ext4fs_read_inode(dir->data,le32_to_cpu(dirent.inode),&fdiro->inode); if (status == 0) { free(fdiro); return 0;-- 2.45.2
Nit I don't know if this can a const pointer from the function call prospective
Reviewed-by: Michael Trimarchi michael@amarulasolutions.com
On Sat, 26 Oct 2024 at 08:41, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Remove copying a pointer with a cast to the very same type.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4_common.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, 26 Oct 2024 at 09:41, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Remove copying a pointer with a cast to the very same type.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4_common.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 76f7102456e..c1e38978bb9 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2046,24 +2046,23 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, unsigned int fpos = 0; int status; loff_t actread;
struct ext2fs_node *diro = (struct ext2fs_node *) dir;#ifdef DEBUG if (name != NULL) printf("Iterate dir %s\n", name); #endif /* of DEBUG */
if (!diro->inode_read) {status = ext4fs_read_inode(diro->data, diro->ino, &diro->inode);
if (!dir->inode_read) {status = ext4fs_read_inode(dir->data, dir->ino, &dir->inode); if (status == 0) return 0; } /* Search the file. */
while (fpos < le32_to_cpu(diro->inode.size)) {
while (fpos < le32_to_cpu(dir->inode.size)) { struct ext2_dirent dirent;
status = ext4fs_read_file(diro, fpos,sizeof(struct ext2_dirent),(char *)&dirent, &actread);
status = ext4fs_read_file(dir, fpos,sizeof(struct ext2_dirent),(char *)&dirent, &actread); if (status < 0) return 0;@@ -2077,7 +2076,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, struct ext2fs_node *fdiro; int type = FILETYPE_UNKNOWN;
status = ext4fs_read_file(diro,
status = ext4fs_read_file(dir, fpos + sizeof(struct ext2_dirent), dirent.namelen, filename,@@ -2089,7 +2088,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, if (!fdiro) return 0;
fdiro->data = diro->data;
fdiro->data = dir->data; fdiro->ino = le32_to_cpu(dirent.inode); filename[dirent.namelen] = '\0';@@ -2104,7 +2103,7 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, else if (dirent.filetype == FILETYPE_REG) type = FILETYPE_REG; } else {
status = ext4fs_read_inode(diro->data,
status = ext4fs_read_inode(dir->data, le32_to_cpu (dirent.inode), &fdiro->inode);@@ -2140,10 +2139,10 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, } } else { if (fdiro->inode_read == 0) {
status = ext4fs_read_inode(diro->data,le32_to_cpu(dirent.inode),&fdiro->inode);
status = ext4fs_read_inode(dir->data,le32_to_cpu(dirent.inode),&fdiro->inode); if (status == 0) { free(fdiro); return 0;-- 2.45.2
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
The directory retrieved in ext4fs_exists() should be freed to avoid a memory leak.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- fs/ext4/ext4fs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 15587e92e3e..21714149ef5 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -209,12 +209,17 @@ int ext4fs_exists(const char *filename) { struct ext2fs_node *dirnode = NULL; int filetype; + int ret;
if (!filename) return 0;
- return ext4fs_find_file1(filename, &ext4fs_root->diropen, &dirnode, - &filetype); + ret = ext4fs_find_file1(filename, &ext4fs_root->diropen, &dirnode, + &filetype); + if (dirnode) + ext4fs_free_node(dirnode, &ext4fs_root->diropen); + + return ret; }
int ext4fs_size(const char *filename, loff_t *size)
Hi
On Sat, Oct 26, 2024 at 8:42 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
The directory retrieved in ext4fs_exists() should be freed to avoid a memory leak.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4fs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 15587e92e3e..21714149ef5 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -209,12 +209,17 @@ int ext4fs_exists(const char *filename) { struct ext2fs_node *dirnode = NULL; int filetype;
int ret; if (!filename) return 0;
return ext4fs_find_file1(filename, &ext4fs_root->diropen, &dirnode,&filetype);
ret = ext4fs_find_file1(filename, &ext4fs_root->diropen, &dirnode,&filetype);if (dirnode)ext4fs_free_node(dirnode, &ext4fs_root->diropen);return ret;}
Reviewed-by: Michael Trimarchi michael@amarulasolutions.com
int ext4fs_size(const char *filename, loff_t *size)
2.45.2
On Sat, 26 Oct 2024 at 08:41, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
The directory retrieved in ext4fs_exists() should be freed to avoid a memory leak.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4fs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, 26 Oct 2024 at 09:41, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
The directory retrieved in ext4fs_exists() should be freed to avoid a memory leak.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4fs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 15587e92e3e..21714149ef5 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -209,12 +209,17 @@ int ext4fs_exists(const char *filename) { struct ext2fs_node *dirnode = NULL; int filetype;
int ret; if (!filename) return 0;
return ext4fs_find_file1(filename, &ext4fs_root->diropen, &dirnode,&filetype);
ret = ext4fs_find_file1(filename, &ext4fs_root->diropen, &dirnode,&filetype);if (dirnode)ext4fs_free_node(dirnode, &ext4fs_root->diropen);return ret;}
int ext4fs_size(const char *filename, loff_t *size)
2.45.2
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
For accessing directories from the EFI sub-system a file system must implement opendir, readdir, closedir. Provide the missing implementation.
With this patch the eficonfig command can be used to define load options for the ext4 file system.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- fs/ext4/ext4fs.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++- fs/fs.c | 4 +- include/ext4fs.h | 4 ++ 3 files changed, 166 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 21714149ef5..32693198aeb 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -21,17 +21,36 @@ */
#include <blk.h> +#include <div64.h> +#include <errno.h> #include <ext_common.h> #include <ext4fs.h> -#include "ext4_common.h" -#include <div64.h> #include <malloc.h> #include <part.h> #include <u-boot/uuid.h> +#include "ext4_common.h"
int ext4fs_symlinknest; struct ext_filesystem ext_fs;
+/** + * struct ext4_dir_stream - ext4 directory stream + * + * @parent: partition data used by fs layer. + * This field must be at the beginning of the structure. + * All other fields are private to the ext4 driver. + * @root: root directory node + * @dir: directory node + * @dirent: directory stream entry + * @fpos: file position in directory + */ +struct ext4_dir_stream { + struct fs_dir_stream parent; + char *dirname; + struct fs_dirent dirent; + unsigned int fpos; +}; + struct ext_filesystem *get_fs(void) { return &ext_fs; @@ -205,6 +224,144 @@ int ext4fs_ls(const char *dirname) return 0; }
+int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) +{ + struct ext4_dir_stream *dirs; + struct ext2fs_node *dir = NULL; + int ret; + + *dirsp = NULL; + + dirs = calloc(1, sizeof(struct ext4_dir_stream)); + if (!dirs) + return -ENOMEM; + dirs->dirname = strdup(dirname); + if (!dirs) { + free(dirs); + return -ENOMEM; + } + + ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, + FILETYPE_DIRECTORY); + if (ret == 1) { + ret = 0; + *dirsp = (struct fs_dir_stream *)dirs; + } else { + ret = -ENOENT; + } + + if (dir) + ext4fs_free_node(dir, &ext4fs_root->diropen); + + return ret; +} + +int ext4fs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp) +{ + struct ext4_dir_stream *dirs = (struct ext4_dir_stream *)fs_dirs; + struct fs_dirent *dent = &dirs->dirent; + struct ext2fs_node *dir = NULL; + int ret; + loff_t actread; + struct ext2fs_node fdiro; + int len; + struct ext2_dirent dirent; + + *dentp = NULL; + + ret = ext4fs_find_file(dirs->dirname, &ext4fs_root->diropen, &dir, + FILETYPE_DIRECTORY); + if (ret != 1) { + ret = -ENOENT; + goto out; + } + if (!dir->inode_read) { + ret = ext4fs_read_inode(dir->data, dir->ino, &dir->inode); + if (!ret) { + ret = -EIO; + goto out; + } + } + + if (dirs->fpos >= le32_to_cpu(dir->inode.size)) + return -ENOENT; + + memset(dent, 0, sizeof(struct fs_dirent)); + + while (dirs->fpos < le32_to_cpu(dir->inode.size)) { + ret = ext4fs_read_file(dir, dirs->fpos, + sizeof(struct ext2_dirent), + (char *)&dirent, &actread); + if (ret < 0) + return -ret; + + if (!dirent.direntlen) + return -EIO; + + if (dirent.namelen) + break; + + dirs->fpos += le16_to_cpu(dirent.direntlen); + } + + len = min(FS_DIRENT_NAME_LEN - 1, (int)dirent.namelen); + + ret = ext4fs_read_file(dir, dirs->fpos + sizeof(struct ext2_dirent), + len, dent->name, &actread); + if (ret < 0) + goto out; + dent->name[len] = '\0'; + + fdiro.data = dir->data; + fdiro.ino = le32_to_cpu(dirent.inode); + + ret = ext4fs_read_inode(dir->data, fdiro.ino, &fdiro.inode); + if (!ret) { + ret = -EIO; + goto out; + } + + switch (le16_to_cpu(fdiro.inode.mode) & FILETYPE_INO_MASK) { + case FILETYPE_INO_DIRECTORY: + dent->type = FS_DT_DIR; + break; + case FILETYPE_INO_SYMLINK: + dent->type = FS_DT_LNK; + break; + case FILETYPE_INO_REG: + dent->type = FS_DT_REG; + break; + default: + dent->type = FILETYPE_UNKNOWN; + } + + rtc_to_tm(fdiro.inode.atime, &dent->access_time); + rtc_to_tm(fdiro.inode.ctime, &dent->create_time); + rtc_to_tm(fdiro.inode.mtime, &dent->change_time); + + dirs->fpos += le16_to_cpu(dirent.direntlen); + dent->size = fdiro.inode.size; + *dentp = dent; + ret = 0; + +out: + if (dir) + ext4fs_free_node(dir, &ext4fs_root->diropen); + + return ret; +} + +void ext4fs_closedir(struct fs_dir_stream *fs_dirs) +{ + struct ext4_dir_stream *dirs = (struct ext4_dir_stream *)fs_dirs; + + if (!dirs) + return; + + free(dirs->dirname); + free(dirs); +} + int ext4fs_exists(const char *filename) { struct ext2fs_node *dirnode = NULL; diff --git a/fs/fs.c b/fs/fs.c index e2915e7cf79..a515905c4c9 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -232,7 +232,9 @@ static struct fstype_info fstypes[] = { .ln = fs_ln_unsupported, #endif .uuid = ext4fs_uuid, - .opendir = fs_opendir_unsupported, + .opendir = ext4fs_opendir, + .readdir = ext4fs_readdir, + .closedir = ext4fs_closedir, .unlink = fs_unlink_unsupported, .mkdir = fs_mkdir_unsupported, }, diff --git a/include/ext4fs.h b/include/ext4fs.h index 41f9eb8bd33..fe3fb301ec8 100644 --- a/include/ext4fs.h +++ b/include/ext4fs.h @@ -27,6 +27,7 @@ #ifndef __EXT4__ #define __EXT4__ #include <ext_common.h> +#include <fs.h>
struct disk_partition;
@@ -218,4 +219,7 @@ int ext4fs_uuid(char *uuid_str); void ext_cache_init(struct ext_block_cache *cache); void ext_cache_fini(struct ext_block_cache *cache); int ext_cache_read(struct ext_block_cache *cache, lbaint_t block, int size); +int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp); +int ext4fs_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp); +void ext4fs_closedir(struct fs_dir_stream *dirs); #endif
Hi
On Sat, Oct 26, 2024 at 8:41 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
For accessing directories from the EFI sub-system a file system must implement opendir, readdir, closedir. Provide the missing implementation.
With this patch the eficonfig command can be used to define load options for the ext4 file system.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4fs.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++- fs/fs.c | 4 +- include/ext4fs.h | 4 ++ 3 files changed, 166 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 21714149ef5..32693198aeb 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -21,17 +21,36 @@ */
#include <blk.h> +#include <div64.h> +#include <errno.h> #include <ext_common.h> #include <ext4fs.h> -#include "ext4_common.h" -#include <div64.h> #include <malloc.h> #include <part.h> #include <u-boot/uuid.h> +#include "ext4_common.h"
int ext4fs_symlinknest; struct ext_filesystem ext_fs;
+/**
- struct ext4_dir_stream - ext4 directory stream
- @parent: partition data used by fs layer.
- This field must be at the beginning of the structure.
- All other fields are private to the ext4 driver.
- @root: root directory node
- @dir: directory node
- @dirent: directory stream entry
- @fpos: file position in directory
- */
+struct ext4_dir_stream {
struct fs_dir_stream parent;char *dirname;struct fs_dirent dirent;unsigned int fpos;+};
struct ext_filesystem *get_fs(void) { return &ext_fs; @@ -205,6 +224,144 @@ int ext4fs_ls(const char *dirname) return 0; }
+int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) +{
struct ext4_dir_stream *dirs;struct ext2fs_node *dir = NULL;int ret;*dirsp = NULL;dirs = calloc(1, sizeof(struct ext4_dir_stream));if (!dirs)return -ENOMEM;dirs->dirname = strdup(dirname);if (!dirs) {free(dirs);return -ENOMEM;}
You can move it after find_file
ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);if (ret == 1) {ret = 0;*dirsp = (struct fs_dir_stream *)dirs;} else {ret = -ENOENT;
You should free dirs here if you did not find anything
Michael
}if (dir)ext4fs_free_node(dir, &ext4fs_root->diropen);return ret;+}
+int ext4fs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp) +{
struct ext4_dir_stream *dirs = (struct ext4_dir_stream *)fs_dirs;struct fs_dirent *dent = &dirs->dirent;struct ext2fs_node *dir = NULL;int ret;loff_t actread;struct ext2fs_node fdiro;int len;struct ext2_dirent dirent;*dentp = NULL;ret = ext4fs_find_file(dirs->dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);if (ret != 1) {ret = -ENOENT;goto out;}if (!dir->inode_read) {ret = ext4fs_read_inode(dir->data, dir->ino, &dir->inode);if (!ret) {ret = -EIO;goto out;}}if (dirs->fpos >= le32_to_cpu(dir->inode.size))return -ENOENT;memset(dent, 0, sizeof(struct fs_dirent));while (dirs->fpos < le32_to_cpu(dir->inode.size)) {ret = ext4fs_read_file(dir, dirs->fpos,sizeof(struct ext2_dirent),(char *)&dirent, &actread);if (ret < 0)return -ret;if (!dirent.direntlen)return -EIO;if (dirent.namelen)break;dirs->fpos += le16_to_cpu(dirent.direntlen);}len = min(FS_DIRENT_NAME_LEN - 1, (int)dirent.namelen);ret = ext4fs_read_file(dir, dirs->fpos + sizeof(struct ext2_dirent),len, dent->name, &actread);if (ret < 0)goto out;dent->name[len] = '\0';fdiro.data = dir->data;fdiro.ino = le32_to_cpu(dirent.inode);ret = ext4fs_read_inode(dir->data, fdiro.ino, &fdiro.inode);if (!ret) {ret = -EIO;goto out;}switch (le16_to_cpu(fdiro.inode.mode) & FILETYPE_INO_MASK) {case FILETYPE_INO_DIRECTORY:dent->type = FS_DT_DIR;break;case FILETYPE_INO_SYMLINK:dent->type = FS_DT_LNK;break;case FILETYPE_INO_REG:dent->type = FS_DT_REG;break;default:dent->type = FILETYPE_UNKNOWN;}rtc_to_tm(fdiro.inode.atime, &dent->access_time);rtc_to_tm(fdiro.inode.ctime, &dent->create_time);rtc_to_tm(fdiro.inode.mtime, &dent->change_time);dirs->fpos += le16_to_cpu(dirent.direntlen);dent->size = fdiro.inode.size;*dentp = dent;ret = 0;+out:
if (dir)ext4fs_free_node(dir, &ext4fs_root->diropen);return ret;+}
+void ext4fs_closedir(struct fs_dir_stream *fs_dirs) +{
struct ext4_dir_stream *dirs = (struct ext4_dir_stream *)fs_dirs;if (!dirs)return;free(dirs->dirname);free(dirs);+}
int ext4fs_exists(const char *filename) { struct ext2fs_node *dirnode = NULL; diff --git a/fs/fs.c b/fs/fs.c index e2915e7cf79..a515905c4c9 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -232,7 +232,9 @@ static struct fstype_info fstypes[] = { .ln = fs_ln_unsupported, #endif .uuid = ext4fs_uuid,
.opendir = fs_opendir_unsupported,
.opendir = ext4fs_opendir,.readdir = ext4fs_readdir,.closedir = ext4fs_closedir, .unlink = fs_unlink_unsupported, .mkdir = fs_mkdir_unsupported, },diff --git a/include/ext4fs.h b/include/ext4fs.h index 41f9eb8bd33..fe3fb301ec8 100644 --- a/include/ext4fs.h +++ b/include/ext4fs.h @@ -27,6 +27,7 @@ #ifndef __EXT4__ #define __EXT4__ #include <ext_common.h> +#include <fs.h>
struct disk_partition;
@@ -218,4 +219,7 @@ int ext4fs_uuid(char *uuid_str); void ext_cache_init(struct ext_block_cache *cache); void ext_cache_fini(struct ext_block_cache *cache); int ext_cache_read(struct ext_block_cache *cache, lbaint_t block, int size); +int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp); +int ext4fs_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp); +void ext4fs_closedir(struct fs_dir_stream *dirs);
#endif
2.45.2
Hi Heinrich,
On Sat, 26 Oct 2024 at 08:41, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
For accessing directories from the EFI sub-system a file system must implement opendir, readdir, closedir. Provide the missing implementation.
With this patch the eficonfig command can be used to define load options for the ext4 file system.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4fs.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++- fs/fs.c | 4 +- include/ext4fs.h | 4 ++ 3 files changed, 166 insertions(+), 3 deletions(-)
with nits/questions below
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 21714149ef5..32693198aeb 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -21,17 +21,36 @@ */
#include <blk.h> +#include <div64.h> +#include <errno.h> #include <ext_common.h> #include <ext4fs.h> -#include "ext4_common.h" -#include <div64.h> #include <malloc.h> #include <part.h> #include <u-boot/uuid.h> +#include "ext4_common.h"
int ext4fs_symlinknest; struct ext_filesystem ext_fs;
+/**
- struct ext4_dir_stream - ext4 directory stream
- @parent: partition data used by fs layer.
- This field must be at the beginning of the structure.
- All other fields are private to the ext4 driver.
- @root: root directory node
- @dir: directory node
- @dirent: directory stream entry
- @fpos: file position in directory
- */
+struct ext4_dir_stream {
struct fs_dir_stream parent;char *dirname;struct fs_dirent dirent;unsigned int fpos;+};
struct ext_filesystem *get_fs(void) { return &ext_fs; @@ -205,6 +224,144 @@ int ext4fs_ls(const char *dirname) return 0; }
+int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) +{
struct ext4_dir_stream *dirs;struct ext2fs_node *dir = NULL;int ret;*dirsp = NULL;dirs = calloc(1, sizeof(struct ext4_dir_stream));if (!dirs)return -ENOMEM;dirs->dirname = strdup(dirname);if (!dirs) {free(dirs);return -ENOMEM;}ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);if (ret == 1) {ret = 0;*dirsp = (struct fs_dir_stream *)dirs;} else {ret = -ENOENT;}if (dir)ext4fs_free_node(dir, &ext4fs_root->diropen);return ret;+}
+int ext4fs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp) +{
struct ext4_dir_stream *dirs = (struct ext4_dir_stream *)fs_dirs;struct fs_dirent *dent = &dirs->dirent;struct ext2fs_node *dir = NULL;int ret;loff_t actread;struct ext2fs_node fdiro;int len;struct ext2_dirent dirent;*dentp = NULL;ret = ext4fs_find_file(dirs->dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);if (ret != 1) {ret = -ENOENT;goto out;}if (!dir->inode_read) {ret = ext4fs_read_inode(dir->data, dir->ino, &dir->inode);if (!ret) {ret = -EIO;goto out;}}if (dirs->fpos >= le32_to_cpu(dir->inode.size))return -ENOENT;memset(dent, 0, sizeof(struct fs_dirent));while (dirs->fpos < le32_to_cpu(dir->inode.size)) {ret = ext4fs_read_file(dir, dirs->fpos,sizeof(struct ext2_dirent),(char *)&dirent, &actread);if (ret < 0)return -ret;if (!dirent.direntlen)return -EIO;
Should this (and the return immediately above) free call ext4fs_free_node() ?
if (dirent.namelen)break;dirs->fpos += le16_to_cpu(dirent.direntlen);}len = min(FS_DIRENT_NAME_LEN - 1, (int)dirent.namelen);ret = ext4fs_read_file(dir, dirs->fpos + sizeof(struct ext2_dirent),len, dent->name, &actread);if (ret < 0)goto out;dent->name[len] = '\0';fdiro.data = dir->data;fdiro.ino = le32_to_cpu(dirent.inode);ret = ext4fs_read_inode(dir->data, fdiro.ino, &fdiro.inode);if (!ret) {ret = -EIO;goto out;}switch (le16_to_cpu(fdiro.inode.mode) & FILETYPE_INO_MASK) {case FILETYPE_INO_DIRECTORY:dent->type = FS_DT_DIR;break;case FILETYPE_INO_SYMLINK:dent->type = FS_DT_LNK;break;case FILETYPE_INO_REG:dent->type = FS_DT_REG;break;default:dent->type = FILETYPE_UNKNOWN;}rtc_to_tm(fdiro.inode.atime, &dent->access_time);rtc_to_tm(fdiro.inode.ctime, &dent->create_time);rtc_to_tm(fdiro.inode.mtime, &dent->change_time);dirs->fpos += le16_to_cpu(dirent.direntlen);dent->size = fdiro.inode.size;*dentp = dent;ret = 0;+out:
if (dir)ext4fs_free_node(dir, &ext4fs_root->diropen);return ret;+}
+void ext4fs_closedir(struct fs_dir_stream *fs_dirs) +{
struct ext4_dir_stream *dirs = (struct ext4_dir_stream *)fs_dirs;if (!dirs)return;free(dirs->dirname);free(dirs);+}
int ext4fs_exists(const char *filename) { struct ext2fs_node *dirnode = NULL; diff --git a/fs/fs.c b/fs/fs.c index e2915e7cf79..a515905c4c9 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -232,7 +232,9 @@ static struct fstype_info fstypes[] = { .ln = fs_ln_unsupported, #endif .uuid = ext4fs_uuid,
.opendir = fs_opendir_unsupported,
.opendir = ext4fs_opendir,.readdir = ext4fs_readdir,.closedir = ext4fs_closedir, .unlink = fs_unlink_unsupported, .mkdir = fs_mkdir_unsupported, },diff --git a/include/ext4fs.h b/include/ext4fs.h index 41f9eb8bd33..fe3fb301ec8 100644 --- a/include/ext4fs.h +++ b/include/ext4fs.h @@ -27,6 +27,7 @@ #ifndef __EXT4__ #define __EXT4__ #include <ext_common.h> +#include <fs.h>
struct disk_partition;
@@ -218,4 +219,7 @@ int ext4fs_uuid(char *uuid_str); void ext_cache_init(struct ext_block_cache *cache); void ext_cache_fini(struct ext_block_cache *cache); int ext_cache_read(struct ext_block_cache *cache, lbaint_t block, int size); +int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp); +int ext4fs_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp); +void ext4fs_closedir(struct fs_dir_stream *dirs);
Please add function comments
#endif
2.45.2
Regards, Simon
* Some of our file system drivers cannot report a file size for directories. Use a dummy value in this case. * For SetInfo the UEFI spec requires to ignore the file size field.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- lib/efi_loader/efi_file.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index c92d8ccf004..95b3c890ee9 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -864,8 +864,16 @@ static efi_status_t EFIAPI efi_file_getinfo(struct efi_file_handle *file, }
ret = efi_get_file_size(fh, &file_size); - if (ret != EFI_SUCCESS) - goto error; + if (ret != EFI_SUCCESS) { + if (!fh->isdir) + goto error; + /* + * Some file drivers don't implement fs_size() for + * directories. Use a dummy non-zero value. + */ + file_size = 4096; + ret = EFI_SUCCESS; + }
memset(info, 0, required_size);
@@ -976,14 +984,16 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file, } free(new_file_name); /* Check for truncation */ - ret = efi_get_file_size(fh, &file_size); - if (ret != EFI_SUCCESS) - goto out; - if (file_size != info->file_size) { - /* TODO: we do not support truncation */ - EFI_PRINT("Truncation not supported\n"); - ret = EFI_ACCESS_DENIED; - goto out; + if (!fh->isdir) { + ret = efi_get_file_size(fh, &file_size); + if (ret != EFI_SUCCESS) + goto out; + if (file_size != info->file_size) { + /* TODO: we do not support truncation */ + EFI_PRINT("Truncation not supported\n"); + ret = EFI_ACCESS_DENIED; + goto out; + } } /* * We do not care for the other attributes
On Sat, 26 Oct 2024 at 08:41, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
- Some of our file system drivers cannot report a file size for directories. Use a dummy value in this case.
- For SetInfo the UEFI spec requires to ignore the file size field.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
lib/efi_loader/efi_file.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, 26 Oct 2024 at 09:41, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
- Some of our file system drivers cannot report a file size for directories. Use a dummy value in this case.
- For SetInfo the UEFI spec requires to ignore the file size field.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
lib/efi_loader/efi_file.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index c92d8ccf004..95b3c890ee9 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -864,8 +864,16 @@ static efi_status_t EFIAPI efi_file_getinfo(struct efi_file_handle *file, }
ret = efi_get_file_size(fh, &file_size);
if (ret != EFI_SUCCESS)goto error;
if (ret != EFI_SUCCESS) {if (!fh->isdir)goto error;/** Some file drivers don't implement fs_size() for* directories. Use a dummy non-zero value.*/file_size = 4096;ret = EFI_SUCCESS;} memset(info, 0, required_size);@@ -976,14 +984,16 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file, } free(new_file_name); /* Check for truncation */
ret = efi_get_file_size(fh, &file_size);if (ret != EFI_SUCCESS)goto out;if (file_size != info->file_size) {/* TODO: we do not support truncation */EFI_PRINT("Truncation not supported\n");ret = EFI_ACCESS_DENIED;goto out;
if (!fh->isdir) {ret = efi_get_file_size(fh, &file_size);if (ret != EFI_SUCCESS)goto out;if (file_size != info->file_size) {/* TODO: we do not support truncation */EFI_PRINT("Truncation not supported\n");ret = EFI_ACCESS_DENIED;goto out;} } /* * We do not care for the other attributes-- 2.45.2
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Now that opendir, readir, closedir are implemented for ext4 we can use fs_ls_generic() for implementing the ls command.
Adjust the unit tests:
* fs_ls_generic() produces more spaces between file size and name. * The ext4 specific message "** Can not find directory. **\n" is not written anymore.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- fs/ext4/ext4_common.c | 29 ----------------------------- fs/ext4/ext4fs.c | 23 ----------------------- fs/fs.c | 2 +- test/py/tests/test_env.py | 2 +- test/py/tests/test_fs/test_basic.py | 5 +---- 5 files changed, 3 insertions(+), 58 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index c1e38978bb9..cc150cf824f 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2137,35 +2137,6 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, *fnode = fdiro; return 1; } - } else { - if (fdiro->inode_read == 0) { - status = ext4fs_read_inode(dir->data, - le32_to_cpu( - dirent.inode), - &fdiro->inode); - if (status == 0) { - free(fdiro); - return 0; - } - fdiro->inode_read = 1; - } - switch (type) { - case FILETYPE_DIRECTORY: - printf("<DIR> "); - break; - case FILETYPE_SYMLINK: - printf("<SYM> "); - break; - case FILETYPE_REG: - printf(" "); - break; - default: - printf("< ? > "); - break; - } - printf("%10u %s\n", - le32_to_cpu(fdiro->inode.size), - filename); } free(fdiro); } diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 32693198aeb..dfecfa0b4e8 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -201,29 +201,6 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, return 0; }
-int ext4fs_ls(const char *dirname) -{ - struct ext2fs_node *dirnode = NULL; - int status; - - if (dirname == NULL) - return 0; - - status = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dirnode, - FILETYPE_DIRECTORY); - if (status != 1) { - printf("** Can not find directory. **\n"); - if (dirnode) - ext4fs_free_node(dirnode, &ext4fs_root->diropen); - return 1; - } - - ext4fs_iterate_dir(dirnode, NULL, NULL, NULL); - ext4fs_free_node(dirnode, &ext4fs_root->diropen); - - return 0; -} - int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs; diff --git a/fs/fs.c b/fs/fs.c index a515905c4c9..1afa0fbeaed 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -220,7 +220,7 @@ static struct fstype_info fstypes[] = { .null_dev_desc_ok = false, .probe = ext4fs_probe, .close = ext4fs_close, - .ls = ext4fs_ls, + .ls = fs_ls_generic, .exists = ext4fs_exists, .size = ext4fs_size, .read = ext4_read_file, diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py index 00bcccd65ff..4471db7d9cb 100644 --- a/test/py/tests/test_env.py +++ b/test/py/tests/test_env.py @@ -488,7 +488,7 @@ def test_env_ext4(state_test_env): assert 'Loading Environment from EXT4... OK' in response
response = c.run_command('ext4ls host 0:0') - assert '8192 uboot.env' in response + assert '8192 uboot.env' in response
response = c.run_command('env info') assert 'env_valid = valid' in response diff --git a/test/py/tests/test_fs/test_basic.py b/test/py/tests/test_fs/test_basic.py index 71f3e86fb18..b5f4704172a 100644 --- a/test/py/tests/test_fs/test_basic.py +++ b/test/py/tests/test_fs/test_basic.py @@ -33,10 +33,7 @@ class TestFsBasic(object): # In addition, test with a nonexistent directory to see if we crash. output = u_boot_console.run_command( '%sls host 0:0 invalid_d' % fs_type) - if fs_type == 'ext4': - assert('Can not find directory' in output) - else: - assert('' == output) + assert('' == output)
def test_fs2(self, u_boot_console, fs_obj_basic): """
On Sat, 26 Oct 2024 at 08:41, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Now that opendir, readir, closedir are implemented for ext4 we can use fs_ls_generic() for implementing the ls command.
Adjust the unit tests:
- fs_ls_generic() produces more spaces between file size and name.
- The ext4 specific message "** Can not find directory. **\n" is not written anymore.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
fs/ext4/ext4_common.c | 29 ----------------------------- fs/ext4/ext4fs.c | 23 ----------------------- fs/fs.c | 2 +- test/py/tests/test_env.py | 2 +- test/py/tests/test_fs/test_basic.py | 5 +---- 5 files changed, 3 insertions(+), 58 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Hi Tom
On Sun, Nov 3, 2024 at 12:26 AM Tom Rini trini@konsulko.com wrote:
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Am I sleeping?
int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL; struct ext2fs_node *dir = NULL; int ret;
*dirsp = NULL;
dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM;
dirs->dirname = strdup(dirname); if (!dirs->dirname) { free(dirs); return -ENOMEM; }
ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir, FILETYPE_DIRECTORY);
if (ret == 1) { ret = 0; *dirsp = (struct fs_dir_stream *)dirs; } else { ret = -ENOENT; free(dirs->dirname); free(dirs); }
if (dir) ext4fs_free_node(dir, &ext4fs_root->diropen);
return ret; }
Should not be like this?
-- Tom
On Sun, Nov 03, 2024 at 12:36:38AM +0100, Michael Nazzareno Trimarchi wrote:
Hi Tom
On Sun, Nov 3, 2024 at 12:26 AM Tom Rini trini@konsulko.com wrote:
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Am I sleeping?
... I always forget b4 compares with checked out branch, that should have been master.
int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL; struct ext2fs_node *dir = NULL; int ret;
*dirsp = NULL; dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM; dirs->dirname = strdup(dirname); if (!dirs->dirname) { free(dirs); return -ENOMEM; } ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);
if (ret == 1) { ret = 0; *dirsp = (struct fs_dir_stream *)dirs; } else { ret = -ENOENT; free(dirs->dirname); free(dirs); } if (dir) ext4fs_free_node(dir, &ext4fs_root->diropen); return ret;}
Should not be like this?
Please elaborate?
Hi
Il dom 3 nov 2024, 00:53 Tom Rini trini@konsulko.com ha scritto:
On Sun, Nov 03, 2024 at 12:36:38AM +0100, Michael Nazzareno Trimarchi wrote:
Hi Tom
On Sun, Nov 3, 2024 at 12:26 AM Tom Rini trini@konsulko.com wrote:
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Am I sleeping?
... I always forget b4 compares with checked out branch, that should have been master.
int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL; struct ext2fs_node *dir = NULL; int ret;
*dirsp = NULL; dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM; dirs->dirname = strdup(dirname); if (!dirs->dirname) { free(dirs); return -ENOMEM; } ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);
if (ret == 1) { ret = 0; *dirsp = (struct fs_dir_stream *)dirs; } else { ret = -ENOENT; free(dirs->dirname); free(dirs);
I have add in this path the free of dirs, because according to the code I can see from this email is not reference anymore and anyway according to what I have commented already moving allocation of dirs later make the code a bit simpler.
Michael
}if (dir) ext4fs_free_node(dir, &ext4fs_root->diropen); return ret;}
Should not be like this?
Please elaborate?
-- Tom
Hi Tom
On Sun, Nov 3, 2024 at 9:21 AM Michael Nazzareno Trimarchi michael@amarulasolutions.com wrote:
Hi
Il dom 3 nov 2024, 00:53 Tom Rini trini@konsulko.com ha scritto:
On Sun, Nov 03, 2024 at 12:36:38AM +0100, Michael Nazzareno Trimarchi wrote:
Hi Tom
On Sun, Nov 3, 2024 at 12:26 AM Tom Rini trini@konsulko.com wrote:
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Am I sleeping?
... I always forget b4 compares with checked out branch, that should have been master.
int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL; struct ext2fs_node *dir = NULL; int ret;
*dirsp = NULL; dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM; dirs->dirname = strdup(dirname); if (!dirs->dirname) { free(dirs); return -ENOMEM; } ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);
if (ret == 1) { ret = 0; *dirsp = (struct fs_dir_stream *)dirs; } else { ret = -ENOENT; free(dirs->dirname); free(dirs);I have add in this path the free of dirs, because according to the code I can see from this email is not reference anymore and anyway according to what I have commented already moving allocation of dirs later make the code a bit simpler.
There are few leaks in this function. I have fixed, can you take a look again on what you merge?
Michael
Michael
} if (dir) ext4fs_free_node(dir, &ext4fs_root->diropen); return ret;}
Should not be like this?
Please elaborate?
-- Tom
On 11/3/24 00:36, Michael Nazzareno Trimarchi wrote:
Hi Tom
On Sun, Nov 3, 2024 at 12:26 AM Tom Rini trini@konsulko.com wrote:
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Am I sleeping?
int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL;
Thank you for your review.
We should not set a value that we do not use.
struct ext2fs_node *dir = NULL; int ret; *dirsp = NULL; dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM; dirs->dirname = strdup(dirname); if (!dirs->dirname) {
Yes this line in my code is incorrect.
free(dirs); return -ENOMEM; } ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);
if (ret == 1) { ret = 0; *dirsp = (struct fs_dir_stream *)dirs; } else { ret = -ENOENT; free(dirs->dirname); free(dirs);
Yes, we should avoid a memory leak here.
I will send a patch.
Best regards
Heinrich
} if (dir) ext4fs_free_node(dir, &ext4fs_root->diropen); return ret;}
Should not be like this?
-- Tom
Hi
On Thu, Nov 7, 2024 at 10:27 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/3/24 00:36, Michael Nazzareno Trimarchi wrote:
Hi Tom
On Sun, Nov 3, 2024 at 12:26 AM Tom Rini trini@konsulko.com wrote:
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Am I sleeping?
int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL;
Thank you for your review.
We should not set a value that we do not use.
struct ext2fs_node *dir = NULL; int ret; *dirsp = NULL; dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM; dirs->dirname = strdup(dirname); if (!dirs->dirname) {Yes this line in my code is incorrect.
We need to use valgrind in unit test or we need some code analys to automate this process. We are always looking for memory leaks in uboot in exit path or we need to have alloction funciton that automatic free the memory when we go in error path
Michael
free(dirs); return -ENOMEM; } ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);
if (ret == 1) { ret = 0; *dirsp = (struct fs_dir_stream *)dirs; } else { ret = -ENOENT; free(dirs->dirname); free(dirs);Yes, we should avoid a memory leak here.
I will send a patch.
Best regards
Heinrich
} if (dir) ext4fs_free_node(dir, &ext4fs_root->diropen); return ret;}
Should not be like this?
-- Tom
On 11/7/24 10:31, Michael Nazzareno Trimarchi wrote:
Hi
On Thu, Nov 7, 2024 at 10:27 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/3/24 00:36, Michael Nazzareno Trimarchi wrote:
Hi Tom
On Sun, Nov 3, 2024 at 12:26 AM Tom Rini trini@konsulko.com wrote:
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Am I sleeping?
int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL;
Thank you for your review.
We should not set a value that we do not use.
struct ext2fs_node *dir = NULL; int ret; *dirsp = NULL; dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM; dirs->dirname = strdup(dirname); if (!dirs->dirname) {Yes this line in my code is incorrect.
We need to use valgrind in unit test or we need some code analys to automate this process. We are always looking for memory leaks in uboot in exit path or we need to have alloction funciton that automatic free the memory when we go in error path
Coverity probably would have reported this. But I think we only run it on tags.
This article describes how Valgrind could be used:
https://developers.redhat.com/articles/2022/03/23/use-valgrind-memcheck-cust...
Both malloc and LMB would be candidates.
Best regards
Heinrich
Michael
free(dirs); return -ENOMEM; } ret = ext4fs_find_file(dirname, &ext4fs_root->diropen, &dir,FILETYPE_DIRECTORY);
if (ret == 1) { ret = 0; *dirsp = (struct fs_dir_stream *)dirs; } else { ret = -ENOENT; free(dirs->dirname); free(dirs);Yes, we should avoid a memory leak here.
I will send a patch.
Best regards
Heinrich
} if (dir) ext4fs_free_node(dir, &ext4fs_root->diropen); return ret;}
Should not be like this?
-- Tom
On Thu, Nov 07, 2024 at 10:47:38AM +0100, Heinrich Schuchardt wrote:
On 11/7/24 10:31, Michael Nazzareno Trimarchi wrote:
Hi
On Thu, Nov 7, 2024 at 10:27 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/3/24 00:36, Michael Nazzareno Trimarchi wrote:
Hi Tom
On Sun, Nov 3, 2024 at 12:26 AM Tom Rini trini@konsulko.com wrote:
On Sat, 26 Oct 2024 08:40:43 +0200, Heinrich Schuchardt wrote:
With this series opendir, readdir, closedir are implemented for ext4. These functions are needed for the UEFI sub-system to interact with the ext4 file system.
To reduce code growth the functions are reused to implement the ls command for ext4.
[...]
Applied to local tree/v2-tidy-test-dir, thanks!
Am I sleeping?
int ext4fs_opendir(const char *dirname, struct fs_dir_stream **dirsp) { struct ext4_dir_stream *dirs = NULL;
Thank you for your review.
We should not set a value that we do not use.
struct ext2fs_node *dir = NULL; int ret; *dirsp = NULL; dirs = calloc(1, sizeof(struct ext4_dir_stream)); if (!dirs) return -ENOMEM; dirs->dirname = strdup(dirname); if (!dirs->dirname) {Yes this line in my code is incorrect.
We need to use valgrind in unit test or we need some code analys to automate this process. We are always looking for memory leaks in uboot in exit path or we need to have alloction funciton that automatic free the memory when we go in error path
Coverity probably would have reported this. But I think we only run it on tags.
A challenge with Coverity is that for the free plan we're limited to basically one run per day, and there's not a good way to handle that with CI. I am trying to get in the habit of running a scan after I push my last merge for the day but that's (a) manual and then runs in to (b) doesn't handle branching as I also didn't see a way for the dashboard to handle both "master" and "next", on the free plan.
That said, if anyone out there is very good at Coverity stuff and would like to volunteer to help organize things better, it would be much appreciated!
participants (6)
-
Heinrich Schuchardt -
Heinrich Schuchardt -
Ilias Apalodimas -
Michael Nazzareno Trimarchi -
Simon Glass -
Tom Rini