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

[Heinrich Schuchardt]

On 2/8/25 22:20, Heinrich Schuchardt wrote:
> 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;
>> +    }

Please, do not iterate multiple times over the same paths. You already
call far_itr_resolve() below.

Best regards

Heinrich


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