[PATCH 0/6] add support for renaming to EFI_FILE_PROTOCOL.SetInfo()

[PATCH 0/6] add support for renaming to EFI_FILE_PROTOCOL.SetInfo()

[Gabriel Dalimonte]

This series adds support for file renaming to EFI_FILE_PROTOCOL.SetInfo().
One of the use cases for renaming in EFI is to facilitate boot loader
boot counting.

No existing filesystems in U-Boot currently include file renaming,
resulting in support for renaming at the filesystem level and a
concrete implementation for the FAT filesystem.


Gabriel Dalimonte (6):
  fs: fat: factor out dentry link create/delete
  fs: add rename infrastructure
  fs: fat: add rename
  fs: fat: update parent dirs metadata on rename
  efi_loader: move path out of file_handle
  efi_loader: support file rename in SetInfo()

 cmd/fat.c                               |  14 +
 fs/fat/fat_write.c                      | 448 +++++++++++++++++++++---
 fs/fs.c                                 |  48 +++
 include/fat.h                           |   1 +
 include/fs.h                            |  13 +
 lib/efi_loader/efi_file.c               |  61 +++-
 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 +++++++++++++++++++
 9 files changed, 1009 insertions(+), 65 deletions(-)
 create mode 100644 test/py/tests/test_fs/test_rename.py

-- 
2.34.1

[PATCH 1/6] fs: fat: factor out dentry link create/delete

[Gabriel Dalimonte]

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

 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
+ *
+ * @itr:	directory iterator
+ * @basename:	name of the new entry
+ * @clust:	cluster number the new directory entry should point to
+ * @size:	file size
+ * @attr:	file attributes
+ * 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 */
+	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 */
 			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;
 	}
-- 
2.34.1

Re: [PATCH 1/6] fs: fat: factor out dentry link create/delete

[Heinrich Schuchardt]

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?

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;
> 	}

Re: [PATCH 1/6] fs: fat: factor out dentry link create/delete

[Gabriel D'Alimonte]

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-L996
[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/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/Info.c#L389-L442
[6] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/Flush.c#L245-L249
[7] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/DirectoryManage.c#L196-L220
[8] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/ReadWrite.c#L507-L510
[9] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429742/FatPkg/EnhancedFatDxe/DirectoryManage.c
[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;
> >       }
>

[PATCH 2/6] fs: add rename infrastructure

[Gabriel Dalimonte]

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

 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);
+		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
-- 
2.34.1

Re: [PATCH 2/6] fs: add rename infrastructure

[Heinrich Schuchardt]

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

Re: [PATCH 2/6] fs: add rename infrastructure

[Heinrich Schuchardt]

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?

>> + *
>> + * @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?

I would prefer a real move function that copies the old file to the
pre-existing target directory.

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
>

Re: [PATCH 2/6] fs: add rename infrastructure

[Gabriel D'Alimonte]

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
> >
>

[PATCH 3/6] fs: fat: add rename

[Gabriel Dalimonte]

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.

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
+ *
+ * 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)) {
+			ret = -EIO;
+			break;
+		}
+	}
+
+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;
+	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");
+		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);
+		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);
+		ret = -ENOENT;
+		goto exit;
+	}
+
+	if (!find_directory_entry(old_itr, old_basename)) {
+		log_err("%s: doesn't exist (%d)\n", old_basename, -ENOENT);
+		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);
+		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)
-- 
2.34.1

Re: [PATCH 3/6] fs: fat: add rename

[Heinrich Schuchardt]

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)

[PATCH 4/6] fs: fat: update parent dirs metadata on rename

[Gabriel Dalimonte]

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.

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);
-- 
2.34.1

Re: [PATCH 4/6] fs: fat: update parent dirs metadata on rename

[Heinrich Schuchardt]

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);

Re: [PATCH 4/6] fs: fat: update parent dirs metadata on rename

[Gabriel D'Alimonte]

On Wed, Jan 22, 2025 at 4:00 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> 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."

That was the line I was referring to in the message. My apologies for
not including a direct link in the commit message.

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

I interpreted that to mean only the immediate parent directories of
`old_path` and `new_path` are to be updated. With the rationale for
updating the timestamp and archive attribute on the moved file coming
from the application usage section stating:
"Some implementations mark for update the last file status change
timestamp of renamed files and some do not."

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

That is a good question. I think skipping timestamp updates in the
absence of an RTC makes sense rather than having a timestamp jump
backwards by (likely) several years.

Thanks,

Gabriel

>
> 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);
>

[PATCH 5/6] efi_loader: move path out of file_handle

[Gabriel Dalimonte]

In order to support renaming via SetInfo(), path must allow for longer
values than what was originally present when file_handle was allocated.

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

 lib/efi_loader/efi_file.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index 95b3c890ee..e72bc36aca 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -38,7 +38,7 @@ struct file_handle {
 	struct fs_dir_stream *dirs;
 	struct fs_dirent *dent;
 
-	char path[0];
+	char *path;
 };
 #define to_fh(x) container_of(x, struct file_handle, base)
 
@@ -176,6 +176,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
 		u64 attributes)
 {
 	struct file_handle *fh;
+	char *path;
 	char f0[MAX_UTF8_PER_UTF16] = {0};
 	int plen = 0;
 	int flen = 0;
@@ -192,11 +193,13 @@ static struct efi_file_handle *file_open(struct file_system *fs,
 		plen = strlen(parent->path) + 1;
 	}
 
+	fh = calloc(1, sizeof(*fh));
 	/* +2 is for null and '/' */
-	fh = calloc(1, sizeof(*fh) + plen + (flen * MAX_UTF8_PER_UTF16) + 2);
-	if (!fh)
-		return NULL;
+	path = calloc(1, plen + (flen * MAX_UTF8_PER_UTF16) + 2);
+	if (!fh || !path)
+		goto error;
 
+	fh->path = path;
 	fh->open_mode = open_mode;
 	fh->base = efi_file_handle_protocol;
 	fh->fs = fs;
@@ -243,6 +246,7 @@ static struct efi_file_handle *file_open(struct file_system *fs,
 	return &fh->base;
 
 error:
+	free(fh->path);
 	free(fh);
 	return NULL;
 }
@@ -366,6 +370,7 @@ out:
 static efi_status_t file_close(struct file_handle *fh)
 {
 	fs_closedir(fh->dirs);
+	free(fh->path);
 	free(fh);
 	return EFI_SUCCESS;
 }
-- 
2.34.1

[PATCH 6/6] efi_loader: support file rename in SetInfo()

[Gabriel Dalimonte]

Following the UEFI specification. The specification did not seem to
delineate if file_name was explicitly a file name only, or could
include paths to move the file to a different directory. The more
generous interpretation of supporting paths was selected.

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

 lib/efi_loader/efi_file.c | 48 +++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index e72bc36aca..f3d643a057 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -981,11 +981,49 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
 		pos = new_file_name;
 		utf16_utf8_strcpy(&pos, info->file_name);
 		if (strcmp(new_file_name, filename)) {
-			/* TODO: we do not support renaming */
-			EFI_PRINT("Renaming not supported\n");
-			free(new_file_name);
-			ret = EFI_ACCESS_DENIED;
-			goto out;
+			int dlen;
+			int rv;
+			char *new_path;
+
+			if (set_blk_dev(fh)) {
+				free(new_file_name);
+				ret = EFI_DEVICE_ERROR;
+				goto out;
+			}
+			dlen = filename - fh->path;
+			new_path = calloc(1, dlen + strlen(new_file_name) + 1);
+			if (!new_path) {
+				free(new_file_name);
+				ret = EFI_OUT_OF_RESOURCES;
+				goto out;
+			}
+			memcpy(new_path, fh->path, dlen);
+			strcpy(new_path + dlen, new_file_name);
+			sanitize_path(new_path);
+			rv = fs_exists(new_path);
+			if (rv) {
+				free(new_path);
+				free(new_file_name);
+				ret = EFI_ACCESS_DENIED;
+				goto out;
+			}
+			/* fs_exists() calls fs_close(), so open file system again */
+			if (set_blk_dev(fh)) {
+				free(new_path);
+				free(new_file_name);
+				ret = EFI_DEVICE_ERROR;
+				goto out;
+			}
+			rv = fs_rename(fh->path, new_path);
+			if (rv) {
+				free(new_path);
+				free(new_file_name);
+				ret = EFI_ACCESS_DENIED;
+				goto out;
+			}
+			free(fh->path);
+			fh->path = new_path;
+			ret = EFI_SUCCESS;
 		}
 		free(new_file_name);
 		/* Check for truncation */
-- 
2.34.1

Re: [PATCH 6/6] efi_loader: support file rename in SetInfo()

[Ilias Apalodimas]

Hi Gabriel

On Wed, 22 Jan 2025 at 07:34, Gabriel Dalimonte
<gabriel.dalimonte@gmail.com> wrote:
>
> Following the UEFI specification. The specification did not seem to
> delineate if file_name was explicitly a file name only, or could
> include paths to move the file to a different directory. The more
> generous interpretation of supporting paths was selected.
>
> Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte@gmail.com>
> ---
>
>  lib/efi_loader/efi_file.c | 48 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index e72bc36aca..f3d643a057 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -981,11 +981,49 @@ static efi_status_t EFIAPI efi_file_setinfo(struct efi_file_handle *file,
>                 pos = new_file_name;
>                 utf16_utf8_strcpy(&pos, info->file_name);
>                 if (strcmp(new_file_name, filename)) {
> -                       /* TODO: we do not support renaming */
> -                       EFI_PRINT("Renaming not supported\n");
> -                       free(new_file_name);
> -                       ret = EFI_ACCESS_DENIED;
> -                       goto out;
> +                       int dlen;
> +                       int rv;
> +                       char *new_path;
> +
> +                       if (set_blk_dev(fh)) {
> +                               free(new_file_name);
> +                               ret = EFI_DEVICE_ERROR;
> +                               goto out;
> +                       }
> +                       dlen = filename - fh->path;
> +                       new_path = calloc(1, dlen + strlen(new_file_name) + 1);
> +                       if (!new_path) {
> +                               free(new_file_name);
> +                               ret = EFI_OUT_OF_RESOURCES;
> +                               goto out;

It's not too clear from the patch context but, instead of keeping
track of new_file_name/new_path can you move it to 'out' and init
those variables to NULL?

> +                       }
> +                       memcpy(new_path, fh->path, dlen);
> +                       strcpy(new_path + dlen, new_file_name);
> +                       sanitize_path(new_path);
> +                       rv = fs_exists(new_path);
> +                       if (rv) {
> +                               free(new_path);
> +                               free(new_file_name);

I think we should add some kind of an error message here and in the fails below.

Otherwise looks good.
Thanks
/Ilias

> +                               ret = EFI_ACCESS_DENIED;
> +                               goto out;
> +                       }
> +                       /* fs_exists() calls fs_close(), so open file system again */
> +                       if (set_blk_dev(fh)) {
> +                               free(new_path);
> +                               free(new_file_name);
> +                               ret = EFI_DEVICE_ERROR;
> +                               goto out;
> +                       }
> +                       rv = fs_rename(fh->path, new_path);
> +                       if (rv) {
> +                               free(new_path);
> +                               free(new_file_name);
> +                               ret = EFI_ACCESS_DENIED;
> +                               goto out;
> +                       }
> +                       free(fh->path);
> +                       fh->path = new_path;
> +                       ret = EFI_SUCCESS;
>                 }
>                 free(new_file_name);
>                 /* Check for truncation */
> --
> 2.34.1
>