[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 at gmail.com>:
>Signed-off-by: Gabriel Dalimonte <gabriel.dalimonte at 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;
> 	}