Hi!

Thank you for the reviews and feedback.

On Wed, Jan 22, 2025 at 2:08 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

Am 22. Januar 2025 06:32:26 MEZ schrieb Gabriel Dalimonte gabriel.dalimonte@gmail.com:

...

Signed-off-by: Gabriel Dalimonte gabriel.dalimonte@gmail.com

Hello Gabriel,

Thank you for implementing this.

Please, add a commit message to each patch.

...

---

fs/fat/fat_write.c | 122 ++++++++++++++++++++++++--------------------- 1 file changed, 66 insertions(+), 56 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index ea877ee917..b86e78abc0 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1215,6 +1215,44 @@ static void fill_dentry(fsdata *mydata, dir_entry *dentptr, memcpy(&dentptr->nameext, shortname, SHORT_NAME_SIZE); }

+/**

• • create_link() - inserts a directory entry for a cluster

... for a file or directory

...

• • @itr: directory iterator
• • @basename: name of the new entry

@basename: file name

...

• • @clust: cluster number the new directory entry should point to

... point to. Use 0 if no cluster is assigned yet.

...

• • @size: file size
• • @attr: file attributes

Do we need to keep the creation timestamp when moving files? And only update the change timestamp?

Should the archive flag be set when moving files?

What does EDK II or Linux in these cases?

On Linux (6.12) [1] it looks like the following happens with respect to timestamps: 1. If new_path is not overwriting an existing file, the parent directory of new_path has its modification time updated. (via `vfat_add_entry` [2] ) 2. old_path's parent directory has access, and modification time updated via `vfat_remove_entries` [3] and `vfat_update_dir_metadata` [4]. 3. The moved file maintains its timestamps and does not update them.

EDK II (branch edk2-stable202411) [5] updates the last modification and last access times on the parent directories (via `FatRemoveDirEnt`/`FatCreateDirEnt` [9] -> `FatStoreDirEnt` [9] -> `FatAccessEntry` [9] -> `FatAccessOFile` [8] -- marking the files dirty). It updates the last modification and last access times (via FatOFileFlush [6] -- called on line 469 in [5]) while preserving the creation time on the moved file (via FatCloneDirEnt [7]). The implementation of `FatOFileFlush` results in timestamps being updated up the file tree to the root as `FatOFileFlush` calls `FatStoreDirEnt` marking the subsequent parent directories as dirty.

Linux appears to not touch the ATTR_ARCH attribute on file moves and treats ATTR_ARCH and ATTR_DIR as mutually exclusive [10]. On the other hand, EDK II directly sets the archive bit on the file. The flush operation [6] will set it on each directory up to the root, from my understanding.

Is there a preference on either implementation? Or are there constraints in U-Boot that make either of these timestamp and archive attribute implementations unsuitable to match?

Thanks, Gabriel

[1] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L959-L99... [2] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L682 [3] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/dir.c#L1083 [4] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L924 [5] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [6] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [7] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [8] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [9] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [10] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L643

Maybe you could describe these design considerations in the commit message.

Please, document which fields and flags are implicitly set in the function description.

...

• • Return: 0 for success
• */

+static int create_link(fat_itr *itr, char *basename, __u32 clust, __u32 size,

•                 __u8 attr)
  

+{

•  char shortname[SHORT_NAME_SIZE];
  

•  int ndent;
  

•  int ret;
  

•  /* Check if long name is needed */
  

•  ndent = set_name(itr, basename, shortname);
  

•  if (ndent < 0)
  

•          return ndent;
  

•  ret = fat_find_empty_dentries(itr, ndent);
  

•  if (ret)
  

•          return ret;
  

•  if (ndent > 1) {
  

•          /* Set long name entries */
  

•          ret = fill_dir_slot(itr, basename, shortname);
  

•          if (ret)
  

•                  return ret;
  

•  }
  

•  /* Add attribute as archive */
  

Could this be changed to "Add archive attribute"?.

...

•  fill_dentry(itr->fsdata, itr->dent, shortname, clust, size,
  

•              attr | ATTR_ARCH);
  

•  return 0;
  

+}

/**

• find_directory_entry() - find a directory entry by filename

@@ -1420,35 +1458,15 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer, /* Update change date */ dentry_set_time(retdent); } else {

•          /* Create a new file */
  

•          char shortname[SHORT_NAME_SIZE];
  

•          int ndent;
  

•         if (pos) {
                  /* No hole allowed */
  

This is another deficiency of our FAT driver worth looking into in future.

Best regards

Heinrich

...

                  ret = -EINVAL;
                  goto exit;
          }

•          /* Check if long name is needed */
  

•          ndent = set_name(itr, basename, shortname);
  

•          if (ndent < 0) {
  

•                  ret = ndent;
  

•                  goto exit;
  

•          }
  

•          ret = fat_find_empty_dentries(itr, ndent);
  

•          ret = create_link(itr, basename, 0, size, 0);
          if (ret)
                  goto exit;
  

•          if (ndent > 1) {
  

•                  /* Set long name entries */
  

•                  ret = fill_dir_slot(itr, basename, shortname);
  

•                  if (ret)
  

•                          goto exit;
  

•          }
  

•          /* Set short name entry */
  

•          fill_dentry(itr->fsdata, itr->dent, shortname, 0, size,
  

•                      ATTR_ARCH);
  
          retdent = itr->dent;
  }
  

@@ -1564,6 +1582,31 @@ static int delete_long_name(fat_itr *itr) return 0; }

+/**

• • delete_dentry_link() - deletes a directory entry, but not the cluster chain
• • it points to
• • @itr: the first directory entry (if a longname) to remove
• • Return: 0 for success
• */

+static int delete_dentry_link(fat_itr *itr) +{

•  itr->dent = itr->dent_start;
  

•  itr->remaining = itr->dent_rem;
  

•  /* Delete long name */
  

•  if ((itr->dent->attr & ATTR_VFAT) == ATTR_VFAT &&
  

•      (itr->dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {
  

•          int ret;
  

•          ret = delete_long_name(itr);
  

•          if (ret)
  

•                  return ret;
  

•  }
  

•  /* Delete short name */
  

•  delete_single_dentry(itr);
  

•  return flush_dir(itr);
  

+}

/**

• delete_dentry_long() - remove directory entry

@@ -1589,21 +1632,7 @@ static int delete_dentry_long(fat_itr *itr) if (ret) return ret; }

•  itr->dent = itr->dent_start;
  

•  itr->remaining = itr->dent_rem;
  

•  dent = itr->dent_start;
  

•  /* Delete long name */
  

•  if ((dent->attr & ATTR_VFAT) == ATTR_VFAT &&
  

•      (dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {
  

•          int ret;
  

•          ret = delete_long_name(itr);
  

•          if (ret)
  

•                  return ret;
  

•  }
  

•  /* Delete short name */
  

•  delete_single_dentry(itr);
  

•  return flush_dir(itr);
  

•  return delete_dentry_link(itr);
  

}

int fat_unlink(const char *filename) @@ -1725,9 +1754,6 @@ int fat_mkdir(const char *dirname) ret = -EEXIST; goto exit; } else {

•          char shortname[SHORT_NAME_SIZE];
  

•          int ndent;
  

•         if (itr->is_root) {
                  /* root dir cannot have "." or ".." */
                  if (!strcmp(l_dirname, ".") ||
  

@@ -1737,25 +1763,9 @@ int fat_mkdir(const char *dirname) } }

•          /* Check if long name is needed */
  

•          ndent = set_name(itr, basename, shortname);
  

•          if (ndent < 0) {
  

•                  ret = ndent;
  

•                  goto exit;
  

•          }
  

•          ret = fat_find_empty_dentries(itr, ndent);
  

•          ret = create_link(itr, basename, 0, 0, ATTR_DIR);
          if (ret)
                  goto exit;
  

•          if (ndent > 1) {
  

•                  /* Set long name entries */
  

•                  ret = fill_dir_slot(itr, basename, shortname);
  

•                  if (ret)
  

•                          goto exit;
  

•          }
  

•          /* Set attribute as archive for regular file */
  

•          fill_dentry(itr->fsdata, itr->dent, shortname, 0, 0,
  

•                      ATTR_DIR | ATTR_ARCH);
  
          retdent = itr->dent;
  }
  

On 22.01.25 06:32, Gabriel Dalimonte wrote:

Signed-off-by: Gabriel Dalimonte gabriel.dalimonte@gmail.com

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
• • @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.
• • 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

On Wed, Jan 22, 2025 at 4:18 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 22.01.25 09:10, Heinrich Schuchardt wrote:

...

On 22.01.25 06:32, Gabriel Dalimonte wrote:

...

Signed-off-by: Gabriel Dalimonte gabriel.dalimonte@gmail.com

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

On 22.01.25 06:32, Gabriel Dalimonte wrote:

The implementation roughly follows the POSIX specification for rename(). The ordering of operations attempting to minimize the chance for data loss in unexpected circumstances.

The fatrename command is added mostly for the purpose of testing through the sandbox build.

We should not introduce a FAT specific commands. A file-system specific command like 'fatload' does not offer a user benefit over a generic command like 'load'.

Instead, please, create a 'mv' command that will use whatever file-system is on the partition.

Please, add documentation for every new command in doc/usage/cmd/.

The FAT file system is used in SPL where we need to avoid binary size increases.

Please, create a Kconfig symbol for renaming support.

Please, don't add the rename field in fs/fs.c to the driver if the Kconfig symbol is not set or if in SPL.

This should be enough for the linker to eliminate the rename support from the FAT driver if not needed.

Signed-off-by: Gabriel Dalimonte gabriel.dalimonte@gmail.com

cmd/fat.c | 14 + fs/fat/fat_write.c | 284 ++++++++++++++++++ fs/fs.c | 3 +- include/fat.h | 1 + test/py/tests/test_fs/conftest.py | 121 ++++++++ test/py/tests/test_fs/fstest_helpers.py | 2 + test/py/tests/test_fs/test_rename.py | 366 ++++++++++++++++++++++++ 7 files changed, 790 insertions(+), 1 deletion(-) create mode 100644 test/py/tests/test_fs/test_rename.py

diff --git a/cmd/fat.c b/cmd/fat.c index 5b7484dc1a..56e6bd0f89 100644 --- a/cmd/fat.c +++ b/cmd/fat.c @@ -132,4 +132,18 @@ U_BOOT_CMD( "<interface> [<dev[:part]>] <directory>\n" " - create a directory in 'dev' on 'interface'" );

+static int do_fat_rename(struct cmd_tbl *cmdtp, int flag, int argc,

• 	 char *const argv[])
  

+{

• return do_rename(cmdtp, flag, argc, argv, FS_TYPE_FAT);

+}

+U_BOOT_CMD(

• fatrename, 5, 1, do_fat_rename,
• "rename a file/directory",
• "<interface> [<dev[:part]>] <old_path> <new_path>\n"
• " - renames a file/directory in 'dev' on 'interface' from 'old_path'\n"
• " to 'new_path'"

+); #endif diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index b86e78abc0..f9f7051e30 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1823,3 +1823,287 @@ exit: free(dotdent); return ret; }

+/**

• • fat_itr_parent() - modifies the iterator to the parent directory of the
• • current iterator.
• • @itr: iterator positioned anywhere in a directory
• • @Return: 0 if the iterator is in the parent directory, -errno otherwise
• */

+static int fat_itr_parent(fat_itr *itr) +{

• int ret;
• if (itr->is_root)

• return -EIO;
  

• /* ensure iterator is at the first directory entry */
• ret = fat_move_to_cluster(itr, itr->start_clust);
• if (ret)

• return ret;
  

• return fat_itr_resolve(itr, "..", TYPE_DIR);

+}

+/**

• • check_path_prefix() - errors if a proposed path contains another path

From the description it remains unclear to me what this checks.

• • note: the iterator may be pointing to any directory entry in the directory
• • @prefix_clust: start cluster of the final directory in the prefix path
• • @path_itr: iterator of the path to check.
• • Return: -errno on error, 0 on success
• */

+static int check_path_prefix(loff_t prefix_clust, fat_itr *path_itr) +{

• fat_itr *itr = NULL;
• fsdata fsdata = { .fatbuf = NULL, }, *mydata = &fsdata;
• int ret;
• itr = malloc_cache_aligned(sizeof(fat_itr));
• if (!itr) {

• debug("Error: allocating memory\n");
  

• ret = -ENOMEM;
  

• goto exit;
  

• }
• /* duplicate fsdata */
• *itr = *path_itr;
• fsdata = *itr->fsdata;
• /* allocate local fat buffer */
• fsdata.fatbuf = malloc_cache_aligned(FATBUFSIZE);
• if (!fsdata.fatbuf) {

• debug("Error: allocating memory\n");
  

• ret = -ENOMEM;
  

• goto exit;
  

• }
• fsdata.fatbufnum = -1;
• itr->fsdata = &fsdata;
• /* ensure iterator is at the first directory entry */
• ret = fat_move_to_cluster(itr, itr->start_clust);
• if (ret)

• goto exit;
  

• while (1) {

• if (prefix_clust == itr->start_clust) {
  

• 	ret = -EINVAL;
  

• 	break;
  

• }
  

• if (itr->is_root) {
  

• 	ret = 0;
  

• 	break;
  

• }
  

• /* Should not occur in a well-formed FAT filesystem besides the root */
  

• if (fat_itr_parent(itr)) {
  

This deserves a log_debug() message pointing to the file-system corruption.

• 	ret = -EIO;
  

• 	break;
  

Somebody adding code after the loop might not see this. Therefore I would prefer `goto exit;` here.

• }
  

• }

+exit:

• free(fsdata.fatbuf);
• free(itr);
• return ret;

+}

+/**

• • fat_rename - rename/move a file or directory
• • @old_path: path to the existing file/directory
• • @new_path: new path/name for the rename/move
• • Return: 0 on success, -errno otherwise
• */

+int fat_rename(const char *old_path, const char *new_path) +{

• fat_itr *old_itr = NULL, *new_itr = NULL;
• fsdata old_datablock = { .fatbuf = NULL, };
• fsdata new_datablock = { .fatbuf = NULL, };
• /* used for START macro */
• fsdata *mydata = &old_datablock;
• int ret = -EIO, is_old_dir;
• char *old_path_copy, *old_dirname, *old_basename;
• char *new_path_copy, *new_dirname, *new_basename;

All of these cannot be longer than 256 characters.

Can we allocate the on the stack.

• char l_new_basename[VFAT_MAXLEN_BYTES];
• __u32 old_clust;
• dir_entry *found_existing;
• /* only set if found_existing != NULL */
• __u32 new_clust;
• old_path_copy = strdup(old_path);
• new_path_copy = strdup(new_path);
• old_itr = malloc_cache_aligned(sizeof(fat_itr));
• new_itr = malloc_cache_aligned(sizeof(fat_itr));
• if (!old_path_copy || !new_path_copy || !old_itr || !new_itr) {

• printf("Error: out of memory\n");
  

log_debug()

Please, do not write messages in the FAT driver. log_debug() is ok.

• ret = -ENOMEM;
  

• goto exit;
  

• }
• split_filename(old_path_copy, &old_dirname, &old_basename);
• split_filename(new_path_copy, &new_dirname, &new_basename);
• if (normalize_longname(l_new_basename, new_basename)) {

• printf("FAT: illegal filename (%s)\n", new_basename);
  

log_debug()

• ret = -EINVAL;
  

• goto exit;
  

• }
• if (!strcmp(old_basename, ".") || !strcmp(old_basename, "..") ||

•    !strcmp(old_basename, "") || !strcmp(l_new_basename, ".") ||
  

•    !strcmp(l_new_basename, "..") || !strcmp(l_new_basename, "")) {
  

• ret = -EINVAL;
  

• goto exit;
  

• }
• /* checking for old_path == new_path is deferred until they're resolved */
• /* resolve old_path */
• ret = fat_itr_root(old_itr, &old_datablock);
• if (ret)

• goto exit;
  

• ret = fat_itr_resolve(old_itr, old_dirname, TYPE_DIR);
• if (ret) {

• printf("%s: doesn't exist (%d)\n", old_dirname, ret);
  

"%s doesn't exist (%d)\n"

log_debug()

• ret = -ENOENT;
  

• goto exit;
  

• }
• if (!find_directory_entry(old_itr, old_basename)) {

• log_err("%s: doesn't exist (%d)\n", old_basename, -ENOENT);
  

"%s doesn't exist (%d)\n"

log_debug()

• ret = -ENOENT;
  

• goto exit;
  

• }
• /* store clust old_path points to, to relink later */
• total_sector = old_datablock.total_sect;
• old_clust = START(old_itr->dent);
• is_old_dir = fat_itr_isdir(old_itr);
• /* resolve new_path*/
• ret = fat_itr_root(new_itr, &new_datablock);
• if (ret)

• goto exit;
  

• ret = fat_itr_resolve(new_itr, new_dirname, TYPE_DIR);
• if (ret) {

• printf("%s: doesn't exist (%d)\n", new_dirname, ret);
  

"%s doesn't exist (%d)\n"

log_debug()

Best regards

Heinrich

• ret = -ENOENT;
  

• goto exit;
  

• }
• found_existing = find_directory_entry(new_itr, l_new_basename);
• if (found_existing) {

• /* store cluster of new_path since it may need to be deleted */
  

• new_clust = START(new_itr->dent);
  

• /* old_path is new_path, noop */
  

• if (old_clust == new_clust) {
  

• 	ret = 0;
  

• 	goto exit;
  

• }
  

• if (fat_itr_isdir(new_itr) != is_old_dir) {
  

• 	if (is_old_dir)
  

• 		ret = -ENOTDIR;
  

• 	else
  

• 		ret = -EISDIR;
  

• 	goto exit;
  

• }
  

• }
• if (is_old_dir) {

• ret = check_path_prefix(old_clust, new_itr);
  

• if (ret)
  

• 	goto exit;
  

• }
• /* create/update dentry to point to old_path's data cluster */
• if (found_existing) {

• struct nameext new_name = new_itr->dent->nameext;
  

• __u8 lcase = new_itr->dent->lcase;
  

• if (is_old_dir) {
  

• 	int n_entries = fat_dir_entries(new_itr);
  

• 	if (n_entries < 0) {
  

• 		ret = n_entries;
  

• 		goto exit;
  

• 	}
  

• 	if (n_entries > 2) {
  

• 		printf("Error: directory is not empty: %d\n",
  

• 		       n_entries);
  

• 		ret = -EINVAL;
  

• 		goto exit;
  

• 	}
  

• }
  

• *new_itr->dent = *old_itr->dent;
  

• new_itr->dent->nameext = new_name;
  

• new_itr->dent->lcase = lcase;
  

• } else {

• /* reset iterator to the start of the directory */
  

• ret = fat_move_to_cluster(new_itr, new_itr->start_clust);
  

• if (ret)
  

• 	goto exit;
  

• ret = create_link(new_itr, l_new_basename, old_clust,
  

• 		  old_itr->dent->size, old_itr->dent->attr);
  

• if (ret)
  

• 	goto exit;
  

• }
• ret = flush_dir(new_itr);
• if (ret)

• goto exit;
  

• /* with new_path data cluster unreferenced, clear it */
• if (found_existing) {

• ret = clear_fatent(&new_datablock, new_clust);
  

• if (ret)
  

• 	goto exit;
  

• }
• /* update moved directory so the parent is new_path */
• if (is_old_dir) {

• __u32 clust = new_itr->start_clust;
  

• dir_entry *dent;
  

• fat_itr_child(new_itr, new_itr);
  

• dent = find_directory_entry(new_itr, "..");
  

• if (!dent) {
  

• 	printf("Error: Directory missing parent entry (..)\n");
  

• 	ret = -EIO;
  

• 	goto exit;
  

• }
  

• set_start_cluster(&new_datablock, dent, clust);
  

• ret = flush_dir(new_itr);
  

• if (ret)
  

• 	goto exit;
  

• }
• /* refresh old in case write happened to the same block. */
• ret = fat_move_to_cluster(old_itr, old_itr->dent_clust);
• if (ret)

• goto exit;
  

• ret = delete_dentry_link(old_itr);
• if (ret)

• goto exit;
  

+exit:

• free(new_datablock.fatbuf);
• free(old_datablock.fatbuf);
• free(new_itr);
• free(old_itr);
• free(new_path_copy);
• free(old_path_copy);
• return ret;

+} diff --git a/fs/fs.c b/fs/fs.c index 160a43c957..424adfbbfb 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -208,12 +208,13 @@ static struct fstype_info fstypes[] = { .write = file_fat_write, .unlink = fat_unlink, .mkdir = fat_mkdir,

• .rename = fat_rename,
  

  #else .write = fs_write_unsupported, .unlink = fs_unlink_unsupported, .mkdir = fs_mkdir_unsupported,

-#endif .rename = fs_rename_unsupported, +#endif .uuid = fat_uuid, .opendir = fat_opendir, .readdir = fat_readdir, diff --git a/include/fat.h b/include/fat.h index 3dce99a23c..ca97880de1 100644 --- a/include/fat.h +++ b/include/fat.h @@ -206,6 +206,7 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp); int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp); void fat_closedir(struct fs_dir_stream *dirs); int fat_unlink(const char *filename); +int fat_rename(const char *old_path, const char *new_path); int fat_mkdir(const char *dirname); void fat_close(void); void *fat_next_cluster(fat_itr *itr, unsigned int *nbytes); diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py index af2adaf164..9e188f2228 100644 --- a/test/py/tests/test_fs/conftest.py +++ b/test/py/tests/test_fs/conftest.py @@ -18,6 +18,7 @@ supported_fs_fat = ['fat12', 'fat16'] supported_fs_mkdir = ['fat12', 'fat16', 'fat32'] supported_fs_unlink = ['fat12', 'fat16', 'fat32'] supported_fs_symlink = ['ext4'] +supported_fs_rename = ['fat12', 'fat16', 'fat32']

# # Filesystem test specific setup @@ -55,6 +56,7 @@ def pytest_configure(config): global supported_fs_mkdir global supported_fs_unlink global supported_fs_symlink

• global supported_fs_rename

  def intersect(listA, listB): return [x for x in listA if x in listB]

@@ -68,6 +70,7 @@ def pytest_configure(config): supported_fs_mkdir = intersect(supported_fs, supported_fs_mkdir) supported_fs_unlink = intersect(supported_fs, supported_fs_unlink) supported_fs_symlink = intersect(supported_fs, supported_fs_symlink)

•    supported_fs_rename =  intersect(supported_fs, supported_fs_rename)
  
  

  def pytest_generate_tests(metafunc): """Parametrize fixtures, fs_obj_xxx

@@ -99,6 +102,9 @@ def pytest_generate_tests(metafunc): if 'fs_obj_symlink' in metafunc.fixturenames: metafunc.parametrize('fs_obj_symlink', supported_fs_symlink, indirect=True, scope='module')

• if 'fs_obj_rename' in metafunc.fixturenames:

•    metafunc.parametrize('fs_obj_rename', supported_fs_rename,
  

•        indirect=True, scope='module')
  
  

  # # Helper functions

@@ -527,6 +533,121 @@ def fs_obj_symlink(request, u_boot_config): call('rm -rf %s' % scratch_dir, shell=True) call('rm -f %s' % fs_img, shell=True)

+# +# Fixture for rename test +# +@pytest.fixture() +def fs_obj_rename(request, u_boot_config):

• """Set up a file system to be used in rename tests.
• Args:

•    request: Pytest request object.
  

•    u_boot_config: U-Boot configuration.
  

• Return:

•    A fixture for rename tests, i.e. a triplet of file system type,
  

•    volume file name, and dictionary of test identifier and md5val.
  

• """
• def new_rand_file(path):

•    check_call('dd if=/dev/urandom of=%s bs=1K count=1' % path, shell=True)
  

• def file_hash(path):

•    out = check_output(
  

•        'dd if=%s bs=1K skip=0 count=1 2> /dev/null | md5sum' % path,
  

•        shell=True
  

•    )
  

•    return out.decode().split()[0]
  

• fs_type = request.param
• fs_img = ''
• fs_ubtype = fstype_to_ubname(fs_type)
• check_ubconfig(u_boot_config, fs_ubtype)
• mount_dir = u_boot_config.persistent_data_dir + '/scratch'
• try:

•    check_call('mkdir -p %s' % mount_dir, shell=True)
  

• except CalledProcessError as err:

•    pytest.skip('Preparing mount folder failed for filesystem: ' + fs_type + '. {}'.format(err))
  

•    call('rm -f %s' % fs_img, shell=True)
  

•    return
  

• try:

•    md5val = {}
  

•    # Test Case 1
  

•    check_call('mkdir %s/test1' % mount_dir, shell=True)
  

•    new_rand_file('%s/test1/file1' % mount_dir)
  

•    md5val['test1'] = file_hash('%s/test1/file1' % mount_dir)
  

•    # Test Case 2
  

•    check_call('mkdir %s/test2' % mount_dir, shell=True)
  

•    new_rand_file('%s/test2/file1' % mount_dir)
  

•    new_rand_file('%s/test2/file_exist' % mount_dir)
  

•    md5val['test2'] = file_hash('%s/test2/file1' % mount_dir)
  

•    # Test Case 3
  

•    check_call('mkdir -p %s/test3/dir1' % mount_dir, shell=True)
  

•    new_rand_file('%s/test3/dir1/file1' % mount_dir)
  

•    md5val['test3'] = file_hash('%s/test3/dir1/file1' % mount_dir)
  

•    # Test Case 4
  

•    check_call('mkdir -p %s/test4/dir1' % mount_dir, shell=True)
  

•    check_call('mkdir %s/test4/dir2' % mount_dir, shell=True)
  

•    new_rand_file('%s/test4/dir1/file1' % mount_dir)
  

•    md5val['test4'] = file_hash('%s/test4/dir1/file1' % mount_dir)
  

•    # Test Case 5
  

•    check_call('mkdir -p %s/test5/dir1' % mount_dir, shell=True)
  

•    new_rand_file('%s/test5/file2' % mount_dir)
  

•    md5val['test5'] = file_hash('%s/test5/file2' % mount_dir)
  

•    # Test Case 6
  

•    check_call('mkdir -p %s/test6/dir2' % mount_dir, shell=True)
  

•    new_rand_file('%s/test6/file1' % mount_dir)
  

•    md5val['test6'] = file_hash('%s/test6/file1' % mount_dir)
  

•    # Test Case 7
  

•    check_call('mkdir -p %s/test7/dir1' % mount_dir, shell=True)
  

•    check_call('mkdir -p %s/test7/dir2' % mount_dir, shell=True)
  

•    new_rand_file('%s/test7/dir2/file1' % mount_dir)
  

•    md5val['test7'] = file_hash('%s/test7/dir2/file1' % mount_dir)
  

•    # Test Case 8
  

•    check_call('mkdir -p %s/test8/dir1' % mount_dir, shell=True)
  

•    new_rand_file('%s/test8/dir1/file1' % mount_dir)
  

•    md5val['test8'] = file_hash('%s/test8/dir1/file1' % mount_dir)
  

•    # Test Case 9
  

•    check_call('mkdir -p %s/test9/dir1/nested/inner' % mount_dir, shell=True)
  

•    new_rand_file('%s/test9/dir1/nested/inner/file1' % mount_dir)
  

•    # Test Case 10
  

•    check_call('mkdir -p %s/test10' % mount_dir, shell=True)
  

•    new_rand_file('%s/test10/file1' % mount_dir)
  

•    md5val['test10'] = file_hash('%s/test10/file1' % mount_dir)
  

•    # Test Case 11
  

•    check_call('mkdir -p %s/test11/dir1' % mount_dir, shell=True)
  

•    new_rand_file('%s/test11/dir1/file1' % mount_dir)
  

•    md5val['test11'] = file_hash('%s/test11/dir1/file1' % mount_dir)
  

•    try:
  

•        # 128MiB volume
  

•        fs_img = fs_helper.mk_fs(u_boot_config, fs_type, 0x8000000, '128MB', mount_dir)
  

•    except CalledProcessError as err:
  

•        pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))
  

•        return
  

• except CalledProcessError:

•    pytest.skip('Setup failed for filesystem: ' + fs_type)
  

•    return
  

• else:

•    yield [fs_ubtype, fs_img, md5val]
  

• finally:

•    call('rm -rf %s' % mount_dir, shell=True)
  

•    call('rm -f %s' % fs_img, shell=True)
  

• # # Fixture for fat test #

diff --git a/test/py/tests/test_fs/fstest_helpers.py b/test/py/tests/test_fs/fstest_helpers.py index faec298248..c1447b4d43 100644 --- a/test/py/tests/test_fs/fstest_helpers.py +++ b/test/py/tests/test_fs/fstest_helpers.py @@ -9,5 +9,7 @@ def assert_fs_integrity(fs_type, fs_img): try: if fs_type == 'ext4': check_call('fsck.ext4 -n -f %s' % fs_img, shell=True)

•    elif fs_type in ['fat12', 'fat16', 'fat32']:
  

•        check_call('fsck.fat -n %s' % fs_img, shell=True)
  except CalledProcessError:
      raise
  

diff --git a/test/py/tests/test_fs/test_rename.py b/test/py/tests/test_fs/test_rename.py new file mode 100644 index 0000000000..1a849da910 --- /dev/null +++ b/test/py/tests/test_fs/test_rename.py @@ -0,0 +1,366 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright 2025 Gabriel Dalimonte gabriel.dalimonte@gmail.com +# +# U-Boot File System:rename Test

+import pytest

+from fstest_defs import * +from fstest_helpers import assert_fs_integrity

+@pytest.mark.boardspec('sandbox') +@pytest.mark.slow +class TestRename(object):

• def test_rename1(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 1 - rename a file
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 1 - rename a file'):
  

•        d = 'test1'
  

•        src = '%s/file1' % d
  

•        dst = '%s/file2' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        assert('' == ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s' % (fs_type, ADDR, dst),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('file1' not in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test1'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename2(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 2 - rename a file to an existing file
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 2 - rename a file to an existing file'):
  

•        d = 'test2'
  

•        src = '%s/file1' % d
  

•        dst = '%s/file_exist' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        assert('' == ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s' % (fs_type, ADDR, dst),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('file1' not in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test2'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename3(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 3 - rename a directory
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 3 - rename a directory'):
  

•        d = 'test3'
  

•        src = '%s/dir1' % d
  

•        dst = '%s/dir2' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        assert('' == ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s/file1' % (fs_type, ADDR, dst),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('dir1' not in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test3'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename4(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 4 - rename a directory to an existing directory
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 4 - rename a directory to an existing directory'):
  

•        d = 'test4'
  

•        src = '%s/dir1' % d
  

•        dst = '%s/dir2' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        assert('' == ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s/file1' % (fs_type, ADDR, dst),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('dir1' not in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test4'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename5(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 5 - rename a directory to an existing file
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 5 - rename a directory to an existing file'):
  

•        d = 'test5'
  

•        src = '%s/dir1' % d
  

•        dst = '%s/file2' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        #assert('' == ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('dir1' in ''.join(output))
  

•        assert('file2' in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s' % (fs_type, ADDR, dst),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test5'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename6(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 6 - rename a file to an existing empty directory
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 6 - rename a file to an existing empty directory'):
  

•        d = 'test6'
  

•        src = '%s/file1' % d
  

•        dst = '%s/dir2' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        #assert('' == ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s' % (fs_type, ADDR, src),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('dir2' in ''.join(output))
  

•        assert('file1' in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test6'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename7(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 7 - rename a directory to a non-empty directory should fail
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 7 - rename a directory to a non-empty directory should fail'):
  

•        d = 'test7'
  

•        src = '%s/dir1' % d
  

•        dst = '%s/dir2' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        assert('directory is not empty' in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s/file1' % (fs_type, ADDR, dst),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('dir1' in ''.join(output))
  

•        assert('dir2' in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test7'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename8(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 8 - rename a directory inside itself
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 8 - rename a directory inside itself'):
  

•        d = 'test8'
  

•        src = '%s/dir1' % d
  

•        dst = '%s/dir1/dir1' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s/file1' % (fs_type, ADDR, src),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('dir1' in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, src),
  

•        ])
  

•        assert('file1' in ''.join(output))
  

•        assert('dir1' not in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test8'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename9(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 9 - rename a directory inside itself with backtracks
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 9 - rename a directory inside itself with backtracks'):
  

•        d = 'test9'
  

•        src = '%s/dir1/nested' % d
  

•        dst = '%s/dir1/nested/inner/./../../../dir1/nested/inner/another' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, dst),
  

•        ])
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s/dir1' % (fs_type, d),
  

•        ])
  

•        assert('nested' in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, src),
  

•        ])
  

•        assert('inner' in ''.join(output))
  

•        assert('nested' not in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename10(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 10 - rename a file to itself
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 10 - rename a file to itself'):
  

•        d = 'test10'
  

•        src = '%s/file1' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, src),
  

•        ])
  

•        assert('' == ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s' % (fs_type, ADDR, src),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('file1' in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test10'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

• def test_rename11(self, u_boot_console, fs_obj_rename):

•    """
  

•    Test Case 11 - rename a directory to itself
  

•    """
  

•    fs_type, fs_img, md5val = fs_obj_rename
  

•    with u_boot_console.log.section('Test Case 11 - rename a directory to itself'):
  

•        d = 'test11'
  

•        src = '%s/dir1' % d
  

•        output = u_boot_console.run_command_list([
  

•            'host bind 0 %s' % fs_img,
  

•            'setenv filesize',
  

•            '%srename host 0:0 %s %s ' % (fs_type, src, src),
  

•        ])
  

•        assert('' == ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            '%sload host 0:0 %x /%s/file1' % (fs_type, ADDR, src),
  

•            'printenv filesize'])
  

•        assert('filesize=400' in output)
  

•        output = u_boot_console.run_command_list([
  

•            '%sls host 0:0 %s' % (fs_type, d),
  

•        ])
  

•        assert('dir1' in ''.join(output))
  

•        output = u_boot_console.run_command_list([
  

•            'md5sum %x $filesize' % ADDR,
  

•            'setenv filesize'])
  

•        assert(md5val['test11'] in ''.join(output))
  

•        assert_fs_integrity(fs_type, fs_img)
  

On 22.01.25 06:32, Gabriel Dalimonte wrote:

None of the existing fat code appears to update parent dir properties however, the POSIX specification for rename() mentions that the last modified timestamp should be updated. In addition, I believe the ATTR_ARCH attribute should also be reset on the parent directories as the content of those directories has changed.

Could you, please, provide a link to the referenced specification.

E.g. https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html

Here I find: "Upon successful completion, rename() shall mark for update the last data modification and last file status change timestamps of the parent directory of each file."

Does this mean that if we move a directory we would have to update the fields of all moved non-empty sub-directories?

On many boards we don't have a real time clock. Should we skip updating if we have no RTC?

If we want to update parent directories, we should do this for file creation, too. The update function could be called in the function where we create directory entries.

Best regards

Heinrich

Signed-off-by: Gabriel Dalimonte gabriel.dalimonte@gmail.com

fs/fat/fat_write.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index f9f7051e30..2559e38d5b 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1912,6 +1912,42 @@ exit: return ret; }

+/**

• • update_parent_dir_props - updates the modified time and resets the archive
• • attribute for the parent directory
• • @itr: iterator positioned anywhere in a directory whose parent should be
• • updated
• • @Return: 0 for success
• */

+static int update_parent_dir_props(fat_itr *itr) +{

• int ret = 0;
• __u32 target_clust = itr->start_clust;
• /* For START macro */
• fsdata *mydata = itr->fsdata;
• if (!itr->is_root) {

• ret = fat_itr_parent(itr);
  

• if (ret)
  

• 	return ret;
  

• while (fat_itr_next(itr)) {
  

• 	if (START(itr->dent) == target_clust)
  

• 		goto update;
  

• }
  

• /* dent not found */
  

• return -EIO;
  

+update:

• dentry_set_time(itr->dent);
  

• itr->dent->attr |= ATTR_ARCH;
  

• ret = flush_dir(itr);
  

• }
• return ret;

+}

• /**
  • fat_rename - rename/move a file or directory

@@ -2086,7 +2122,12 @@ int fat_rename(const char *old_path, const char *new_path) ret = flush_dir(new_itr); if (ret) goto exit;

• /* restore directory location to update parent props below */
  

• fat_itr_child(new_itr, new_itr);
  

  }

• ret = update_parent_dir_props(new_itr);

• if (ret)

• goto exit;
  
  

  /* refresh old in case write happened to the same block. */ ret = fat_move_to_cluster(old_itr, old_itr->dent_clust);

@@ -2097,6 +2138,7 @@ int fat_rename(const char *old_path, const char *new_path) if (ret) goto exit;

• ret = update_parent_dir_props(old_itr); exit: free(new_datablock.fatbuf); free(old_datablock.fatbuf);