Hi!
Thank you for the reviews and feedback.
On Wed, Jan 22, 2025 at 2:08 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 22. Januar 2025 06:32:26 MEZ schrieb Gabriel Dalimonte gabriel.dalimonte@gmail.com:
Signed-off-by: Gabriel Dalimonte gabriel.dalimonte@gmail.com
Hello Gabriel,
Thank you for implementing this.
Please, add a commit message to each patch.
fs/fat/fat_write.c | 122 ++++++++++++++++++++++++--------------------- 1 file changed, 66 insertions(+), 56 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index ea877ee917..b86e78abc0 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1215,6 +1215,44 @@ static void fill_dentry(fsdata *mydata, dir_entry *dentptr, memcpy(&dentptr->nameext, shortname, SHORT_NAME_SIZE); }
+/**
- create_link() - inserts a directory entry for a cluster
... for a file or directory
- @itr: directory iterator
- @basename: name of the new entry
@basename: file name
- @clust: cluster number the new directory entry should point to
... point to. Use 0 if no cluster is assigned yet.
- @size: file size
- @attr: file attributes
Do we need to keep the creation timestamp when moving files? And only update the change timestamp?
Should the archive flag be set when moving files?
What does EDK II or Linux in these cases?
On Linux (6.12) [1] it looks like the following happens with respect to timestamps: 1. If new_path is not overwriting an existing file, the parent directory of new_path has its modification time updated. (via `vfat_add_entry` [2] ) 2. old_path's parent directory has access, and modification time updated via `vfat_remove_entries` [3] and `vfat_update_dir_metadata` [4]. 3. The moved file maintains its timestamps and does not update them.
EDK II (branch edk2-stable202411) [5] updates the last modification and last access times on the parent directories (via `FatRemoveDirEnt`/`FatCreateDirEnt` [9] -> `FatStoreDirEnt` [9] -> `FatAccessEntry` [9] -> `FatAccessOFile` [8] -- marking the files dirty). It updates the last modification and last access times (via FatOFileFlush [6] -- called on line 469 in [5]) while preserving the creation time on the moved file (via FatCloneDirEnt [7]). The implementation of `FatOFileFlush` results in timestamps being updated up the file tree to the root as `FatOFileFlush` calls `FatStoreDirEnt` marking the subsequent parent directories as dirty.
Linux appears to not touch the ATTR_ARCH attribute on file moves and treats ATTR_ARCH and ATTR_DIR as mutually exclusive [10]. On the other hand, EDK II directly sets the archive bit on the file. The flush operation [6] will set it on each directory up to the root, from my understanding.
Is there a preference on either implementation? Or are there constraints in U-Boot that make either of these timestamp and archive attribute implementations unsuitable to match?
Thanks, Gabriel
[1] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L959-L99... [2] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L682 [3] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/dir.c#L1083 [4] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L924 [5] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [6] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [7] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [8] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [9] https://github.com/tianocore/edk2/blob/0f3867fa6ef0553e26c42f7d71ff6bdb98429... [10] https://elixir.bootlin.com/linux/v6.12.6/source/fs/fat/namei_vfat.c#L643
Maybe you could describe these design considerations in the commit message.
Please, document which fields and flags are implicitly set in the function description.
- Return: 0 for success
- */
+static int create_link(fat_itr *itr, char *basename, __u32 clust, __u32 size,
__u8 attr)+{
char shortname[SHORT_NAME_SIZE];int ndent;int ret;/* Check if long name is needed */ndent = set_name(itr, basename, shortname);if (ndent < 0)return ndent;ret = fat_find_empty_dentries(itr, ndent);if (ret)return ret;if (ndent > 1) {/* Set long name entries */ret = fill_dir_slot(itr, basename, shortname);if (ret)return ret;}/* Add attribute as archive */Could this be changed to "Add archive attribute"?.
fill_dentry(itr->fsdata, itr->dent, shortname, clust, size,attr | ATTR_ARCH);return 0;+}
/**
- find_directory_entry() - find a directory entry by filename
@@ -1420,35 +1458,15 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer, /* Update change date */ dentry_set_time(retdent); } else {
/* Create a new file */char shortname[SHORT_NAME_SIZE];int ndent;if (pos) { /* No hole allowed */This is another deficiency of our FAT driver worth looking into in future.
Best regards
Heinrich
ret = -EINVAL; goto exit; }
/* Check if long name is needed */ndent = set_name(itr, basename, shortname);if (ndent < 0) {ret = ndent;goto exit;}ret = fat_find_empty_dentries(itr, ndent);
ret = create_link(itr, basename, 0, size, 0); if (ret) goto exit;
if (ndent > 1) {/* Set long name entries */ret = fill_dir_slot(itr, basename, shortname);if (ret)goto exit;}/* Set short name entry */fill_dentry(itr->fsdata, itr->dent, shortname, 0, size,ATTR_ARCH); retdent = itr->dent; }@@ -1564,6 +1582,31 @@ static int delete_long_name(fat_itr *itr) return 0; }
+/**
- delete_dentry_link() - deletes a directory entry, but not the cluster chain
- it points to
- @itr: the first directory entry (if a longname) to remove
- Return: 0 for success
- */
+static int delete_dentry_link(fat_itr *itr) +{
itr->dent = itr->dent_start;itr->remaining = itr->dent_rem;/* Delete long name */if ((itr->dent->attr & ATTR_VFAT) == ATTR_VFAT &&(itr->dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {int ret;ret = delete_long_name(itr);if (ret)return ret;}/* Delete short name */delete_single_dentry(itr);return flush_dir(itr);+}
/**
- delete_dentry_long() - remove directory entry
@@ -1589,21 +1632,7 @@ static int delete_dentry_long(fat_itr *itr) if (ret) return ret; }
itr->dent = itr->dent_start;itr->remaining = itr->dent_rem;dent = itr->dent_start;/* Delete long name */if ((dent->attr & ATTR_VFAT) == ATTR_VFAT &&(dent->nameext.name[0] & LAST_LONG_ENTRY_MASK)) {int ret;ret = delete_long_name(itr);if (ret)return ret;}/* Delete short name */delete_single_dentry(itr);return flush_dir(itr);
return delete_dentry_link(itr);}
int fat_unlink(const char *filename) @@ -1725,9 +1754,6 @@ int fat_mkdir(const char *dirname) ret = -EEXIST; goto exit; } else {
char shortname[SHORT_NAME_SIZE];int ndent;if (itr->is_root) { /* root dir cannot have "." or ".." */ if (!strcmp(l_dirname, ".") ||@@ -1737,25 +1763,9 @@ int fat_mkdir(const char *dirname) } }
/* Check if long name is needed */ndent = set_name(itr, basename, shortname);if (ndent < 0) {ret = ndent;goto exit;}ret = fat_find_empty_dentries(itr, ndent);
ret = create_link(itr, basename, 0, 0, ATTR_DIR); if (ret) goto exit;
if (ndent > 1) {/* Set long name entries */ret = fill_dir_slot(itr, basename, shortname);if (ret)goto exit;}/* Set attribute as archive for regular file */fill_dentry(itr->fsdata, itr->dent, shortname, 0, 0,ATTR_DIR | ATTR_ARCH); retdent = itr->dent; }