On Wed, Jan 22, 2025 at 4:18 AM Heinrich Schuchardt <[email protected]> wrote: > > On 22.01.25 09:10, Heinrich Schuchardt wrote: > > On 22.01.25 06:32, Gabriel Dalimonte wrote: > >> Signed-off-by: Gabriel Dalimonte <[email protected]> > >> --- > > > > Please, add a commit message. > > > >> > >> fs/fs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > >> include/fs.h | 13 +++++++++++++ > >> 2 files changed, 60 insertions(+) > >> > >> diff --git a/fs/fs.c b/fs/fs.c > >> index 99ddcc5e37..160a43c957 100644 > >> --- a/fs/fs.c > >> +++ b/fs/fs.c > >> @@ -143,6 +143,12 @@ static inline int fs_mkdir_unsupported(const char > >> *dirname) > >> return -1; > >> } > >> > >> +static inline int fs_rename_unsupported(const char *old_path, > >> + const char *new_path) > >> +{ > >> + return -1; > >> +} > >> + > >> struct fstype_info { > >> int fstype; > >> char *name; > >> @@ -183,6 +189,7 @@ struct fstype_info { > >> int (*unlink)(const char *filename); > >> int (*mkdir)(const char *dirname); > >> int (*ln)(const char *filename, const char *target); > >> + int (*rename)(const char *old_path, const char *new_path); > >> }; > >> > >> static struct fstype_info fstypes[] = { > >> @@ -206,6 +213,7 @@ static struct fstype_info fstypes[] = { > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> #endif > >> + .rename = fs_rename_unsupported, > >> .uuid = fat_uuid, > >> .opendir = fat_opendir, > >> .readdir = fat_readdir, > >> @@ -238,6 +246,7 @@ static struct fstype_info fstypes[] = { > >> .closedir = ext4fs_closedir, > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> + .rename = fs_rename_unsupported, > >> }, > >> #endif > >> #if IS_ENABLED(CONFIG_SANDBOX) && !IS_ENABLED(CONFIG_XPL_BUILD) > >> @@ -257,6 +266,7 @@ static struct fstype_info fstypes[] = { > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> .ln = fs_ln_unsupported, > >> + .rename = fs_rename_unsupported, > >> }, > >> #endif > >> #if CONFIG_IS_ENABLED(SEMIHOSTING) > >> @@ -276,6 +286,7 @@ static struct fstype_info fstypes[] = { > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> .ln = fs_ln_unsupported, > >> + .rename = fs_rename_unsupported, > >> }, > >> #endif > >> #ifndef CONFIG_XPL_BUILD > >> @@ -296,6 +307,7 @@ static struct fstype_info fstypes[] = { > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> .ln = fs_ln_unsupported, > >> + .rename = fs_rename_unsupported, > >> }, > >> #endif > >> #endif > >> @@ -317,6 +329,7 @@ static struct fstype_info fstypes[] = { > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> .ln = fs_ln_unsupported, > >> + .rename = fs_rename_unsupported, > >> }, > >> #endif > >> #endif > >> @@ -339,6 +352,7 @@ static struct fstype_info fstypes[] = { > >> .ln = fs_ln_unsupported, > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> + .rename = fs_rename_unsupported, > >> }, > >> #endif > >> #if IS_ENABLED(CONFIG_FS_EROFS) > >> @@ -360,6 +374,7 @@ static struct fstype_info fstypes[] = { > >> .ln = fs_ln_unsupported, > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> + .rename = fs_rename_unsupported, > >> }, > >> #endif > >> { > >> @@ -378,6 +393,7 @@ static struct fstype_info fstypes[] = { > >> .unlink = fs_unlink_unsupported, > >> .mkdir = fs_mkdir_unsupported, > >> .ln = fs_ln_unsupported, > >> + .rename = fs_rename_unsupported, > >> }, > >> }; > >> > >> @@ -713,6 +729,22 @@ int fs_ln(const char *fname, const char *target) > >> return ret; > >> } > >> > >> +int fs_rename(const char *old_path, const char *new_path) > >> +{ > >> + struct fstype_info *info = fs_get_info(fs_type); > >> + int ret; > >> + > >> + ret = info->rename(old_path, new_path); > >> + > >> + if (ret < 0) { > >> + log_err("** Unable to rename %s -> %s **\n", old_path, > >> new_path); > > > > We should not write messages by default. This will may lead to output > > layout in EFI applications looking wrong. > > > > log_debug() would be adequate. > > > > We don't need the "** " first characters. > > > > Otherwise the patch looks good to me. > > > > Best regards > > > > Heinrich > > > >> + ret = -1; > >> + } > >> + fs_close(); > >> + > >> + return ret; > >> +} > >> + > >> int do_size(struct cmd_tbl *cmdtp, int flag, int argc, char *const > >> argv[], > >> int fstype) > >> { > >> @@ -975,6 +1007,21 @@ int do_ln(struct cmd_tbl *cmdtp, int flag, int > >> argc, char *const argv[], > >> return 0; > >> } > >> > >> +int do_rename(struct cmd_tbl *cmdtp, int flag, int argc, char *const > >> argv[], > >> + int fstype) > >> +{ > >> + if (argc != 5) > >> + return CMD_RET_USAGE; > >> + > >> + if (fs_set_blk_dev(argv[1], argv[2], fstype)) > >> + return 1; > >> + > >> + if (fs_rename(argv[3], argv[4])) > >> + return 1; > >> + > >> + return 0; > >> +} > >> + > >> int do_fs_types(struct cmd_tbl *cmdtp, int flag, int argc, char * > >> const argv[]) > >> { > >> struct fstype_info *drv = fstypes; > >> diff --git a/include/fs.h b/include/fs.h > >> index 2474880385..24b121f55d 100644 > >> --- a/include/fs.h > >> +++ b/include/fs.h > >> @@ -270,6 +270,17 @@ int fs_unlink(const char *filename); > >> */ > >> int fs_mkdir(const char *filename); > >> > >> +/** > >> + * fs_rename - rename a file or directory > > Shouldn't this function be called fs_move() and allow moving a file from > one directory to another?
I went with fs_rename() since the implementation mirrors the POSIX specification. [1] > > >> + * > >> + * @old_path: existing path of the file/directory to rename > >> + * @new_path: new path of the file/directory. If this points to an > >> existing > >> + * file or empty directory, the existing file/directory will be > >> unlinked. > > What happens if the directory is non-empty? As per [1], the function aborts the rename (without making any changes). Answering this question made me realize I'm erroneously returning EINVAL in this condition instead of the correct EEXIST or ENOTEMPTY. Thanks. :) > > I would prefer a real move function that copies the old file to the > pre-existing target directory. To clarify: Is this for changing fs_rename() to fs_move() (or adding fs_move() in addition to fs_rename()) or changing the command (fat)rename to mv? Thanks, Gabriel [1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html > > Best regards > > Heinrich > > >> + * > >> + * Return: 0 on success, -1 on error conditions > >> + */ > >> +int fs_rename(const char *old_path, const char *new_path); > >> + > >> /* > >> * Common implementation for various filesystem commands, optionally > >> limited > >> * to a specific filesystem type via the fstype parameter. > >> @@ -290,6 +301,8 @@ int do_mkdir(struct cmd_tbl *cmdtp, int flag, int > >> argc, char *const argv[], > >> int fstype); > >> int do_ln(struct cmd_tbl *cmdtp, int flag, int argc, char *const > >> argv[], > >> int fstype); > >> +int do_rename(struct cmd_tbl *cmdtp, int flag, int argc, char *const > >> argv[], > >> + int fstype); > >> > >> /* > >> * Determine the UUID of the specified filesystem and print it. > >> Optionally it is > > >
