Hi! Thank you for the reviews and feedback.
On Wed, Jan 22, 2025 at 2:08 AM Heinrich Schuchardt <[email protected]> wrote: > > Am 22. Januar 2025 06:32:26 MEZ schrieb Gabriel Dalimonte > <[email protected]>: > >Signed-off-by: Gabriel Dalimonte <[email protected]> > > 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; > > } >
