[PATCH v3 4/6] fs: fat: Support basic file rename

[Heinrich Schuchardt]

On 2/7/25 13:45, Daniel Venzin wrote:
> This implementation currently does not support
> moving files between directories.

Why would we make such a restriction?

>
> Signed-off-by: Daniel Venzin <Daniel.Venzin at mt.com>
>
> ---
>
> Changes in v3:
> - Abort the rename if the destination is a path instead of a file
>
> Changes in v2:
> - Separate the rename implementation from changes in the filesystem layer
> - Eliminate code duplication with delete_dentry_long(..)
> - Ensure that the source file exists, but not the destination file
> - Free the fatbuf if it has been reinitialized with fat_itr_root
> - Delete orphaned FAT entry after copying file metadata
>
>   fs/fat/fat_write.c | 125 +++++++++++++++++++++++++++++++++++++++++++++
>   fs/fs.c            |   2 +-
>   include/fat.h      |   1 +
>   3 files changed, 127 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index c78b80e0b7a..6e4ff7c57d4 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -1821,3 +1821,128 @@ exit:
>   	free(dotdent);
>   	return ret;
>   }
> +
> +int file_fat_rename(const char *path, const char *new_fname)

Below you are expecting that new_fname is a path not only a file name.

%s/new_fname/new_path/

> +{
> +	fsdata datablock = { .fatbuf = NULL, };
> +	fat_itr *itr = NULL;
> +
> +	dir_entry *dentry;
> +
> +	char new_path[VFAT_MAXLEN_BYTES];
> +	char *new_fname_copy = NULL, *new_fname_dirname, *new_fname_basename;
> +	char *path_copy = NULL, *path_dirname, *path_basename;
> +
> +	loff_t actwrite;
> +
> +	__u16 starthi, start;
> +	__u32 size, entry;
> +
> +	int ret = -1;
> +
> +	new_fname_copy = strdup(new_fname);
> +	if (!new_fname_copy)
> +		goto exit;
> +
> +	split_filename(new_fname_copy, &new_fname_dirname, &new_fname_basename);
> +
> +	if (strlen(new_fname_dirname) > 1 ||
> +		(strcmp(new_fname_dirname, "/") != 0 &&
> +		 strcmp(new_fname_dirname, ".") != 0)) {

No clue, why you expect "/" or "." at the beginning of a path.
In U-Boot file paths are always relative to the root directory.

Shouldn't fat_itr_resolve() take care of checking the validity of the path?

> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	path_copy = strdup(path);
> +	if (!path_copy)
> +		goto exit;
> +
> +	split_filename(path_copy, &path_dirname, &path_basename);
> +	snprintf(new_path, sizeof(new_path), "%s/%s", path_dirname, new_fname_basename);

You already determined new_fname_dirname. You must not ignore it.
You should delete above statement.

> +
> +	if (!fat_exists(path)) {
> +		ret = -ENOENT;
> +		goto exit;
> +	}
> +
> +	if (fat_exists(new_path)) {
> +		ret = -EEXIST;
> +		goto exit;
> +	}
> +
> +	ret = file_fat_write(new_path, &actwrite, 0, 0, &actwrite);
> +	if (ret)
> +		goto exit;
> +
> +	itr = malloc_cache_aligned(sizeof(*itr));
> +	if (!itr) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	ret = fat_itr_root(itr, &datablock);
> +	if (ret)
> +		goto exit;
> +
> +	ret = fat_itr_resolve(itr, path, TYPE_FILE);

Why don't we allow renaming of directories?

TYPE_FILE | TYPE_DIR

> +	if (ret)
> +		goto exit;
> +
> +	dentry = itr->dent;
> +
> +	starthi = dentry->starthi;
> +	start = dentry->start;
> +	size = dentry->size;
> +
> +	free(datablock.fatbuf);
> +	datablock.fatbuf = NULL;
> +
> +	ret = fat_itr_root(itr, &datablock);
> +	if (ret)
> +		goto exit;
> +
> +	ret = fat_itr_resolve(itr, new_path, TYPE_FILE);

This would not detect if new_path points to an existing or non-existing
directory.

Check that new_fname_dirname exists and then use the same itr to check
that new_fname_dirname/new_basename does not exist.

This avoids iterating once here and once in fat_rename().

> +	if (ret)
> +		goto exit;

We don't want to overwrite existing files or directories. Don't we
expect -ENOENT or -ENOTDIR here?

> +
> +	dentry = itr->dent;
> +
> +	fsdata *mydata = itr->fsdata;
> +
> +	entry = START(dentry);
> +
> +	dentry->starthi = starthi;
> +	dentry->start = start;
> +	dentry->size = size;
> +
> +	ret = flush_dir(itr);
> +	if (ret)
> +		goto exit;
> +
> +	clear_fatent(mydata, entry);
> +	if (flush_dirty_fat_buffer(mydata) < 0) {
> +		printf("Error: flush fat buffer\n");
> +		ret = -EIO;
> +		goto exit;
> +	}
> +
> +	free(datablock.fatbuf);
> +	datablock.fatbuf = NULL;
> +
> +	ret = fat_itr_root(itr, &datablock);
> +	if (ret)
> +		goto exit;
> +
> +	ret = fat_itr_resolve(itr, path, TYPE_FILE);
> +	if (ret)
> +		goto exit;

Why don't you use separate variables for the old and the new itr value?
Then you would not have to resolve the old path twice.

Best regards

Heinrich

> +
> +	ret = delete_dentry_long(itr, FATENT_CLEAR_SKIP);> +
> +exit:
> +	free(new_fname_copy);
> +	free(path_copy);
> +	free(datablock.fatbuf);
> +	free(itr);
> +	return ret;
> +}
> diff --git a/fs/fs.c b/fs/fs.c
> index b746d05ebcd..3576d5c6644 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -208,7 +208,7 @@ static struct fstype_info fstypes[] = {
>   		.write = file_fat_write,
>   		.unlink = fat_unlink,
>   		.mkdir = fat_mkdir,
> -		.rename = fs_rename_unsupported,
> +		.rename = file_fat_rename,
>   #else
>   		.write = fs_write_unsupported,
>   		.unlink = fs_unlink_unsupported,
> diff --git a/include/fat.h b/include/fat.h
> index 3dce99a23cf..415ebd7b7c8 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -207,6 +207,7 @@ 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_mkdir(const char *dirname);
> +int file_fat_rename(const char *path, const char *new_filename);
>   void fat_close(void);
>   void *fat_next_cluster(fat_itr *itr, unsigned int *nbytes);
>