[Concept,20/26] ext4l: Add unlink support for file deletion

Message ID 20251231223008.3251711-21-sjg@u-boot.org
State New
Headers
Series ext4l: Add write support (part L) |

Commit Message

Simon Glass Dec. 31, 2025, 10:29 p.m. UTC
  From: Simon Glass <simon.glass@canonical.com>

Implement ext4l_unlink() to delete files from ext4 filesystems. This
enables the 'rm' command to work with ext4l.

The implementation:
- Resolves the parent directory and target file
- Verifies the target is not a directory (use rmdir for that)
- Calls ext4_unlink() to remove the directory entry
- Uses journal transactions for crash safety

Add ext4l_op_ptr() macro to select between ext4l_unlink() and the
fallback based on CONFIG_EXT4_WRITE

Call ext4_commit_super() to ensure the changes are written to disk.

Co-developed-by: Claude <noreply@anthropic.com>
Signed-off-by: Simon Glass <simon.glass@canonical.com>
---

 fs/ext4l/interface.c                | 251 +++++++++++++++++-----------
 fs/fs_legacy.c                      |   2 +-
 include/ext4l.h                     |   9 +
 test/fs/ext4l.c                     |  41 +++++
 test/py/tests/test_fs/test_ext4l.py |   5 +
 5 files changed, 212 insertions(+), 96 deletions(-)
  

Patch

diff --git a/fs/ext4l/interface.c b/fs/ext4l/interface.c
index 94bfd71bfb5..442617a0d3e 100644
--- a/fs/ext4l/interface.c
+++ b/fs/ext4l/interface.c
@@ -874,75 +874,131 @@  int ext4l_read(const char *filename, void *buf, loff_t offset, loff_t len,
 	return 0;
 }
 
-static int ext4l_write_file(struct inode *dir, const char *filename, void *buf,
-			    loff_t offset, loff_t len, loff_t *actwrite)
+/**
+ * ext4l_resolve_file() - Resolve a file path for write operations
+ * @path: Path to process
+ * @dir_dentryp: Returns parent directory dentry (caller must kfree)
+ * @dentryp: Returns file dentry after lookup (caller must kfree)
+ * @path_copyp: Returns path copy (caller must free)
+ *
+ * Common setup for write operations. Validates inputs, checks read-write
+ * mount, parses path, resolves parent directory, creates dentries, and
+ * performs lookup.
+ *
+ * Return: 0 on success, negative on error
+ */
+static int ext4l_resolve_file(const char *path, struct dentry **dir_dentryp,
+			      struct dentry **dentryp, char **path_copyp)
 {
+	char *path_copy, *dir_path, *last_slash;
 	struct dentry *dir_dentry, *dentry, *result;
-	handle_t *handle = NULL;
-	struct buffer_head *bh;
-	struct inode *inode;
-	loff_t pos, end;
-	umode_t mode;
+	struct inode *dir_inode;
+	const char *basename;
 	int ret;
 
-	/* Create dentry for the parent directory */
+	if (!ext4l_sb)
+		return -ENODEV;
+
+	if (!path)
+		return -EINVAL;
+
+	/* Check if filesystem is mounted read-write */
+	if (ext4l_sb->s_flags & SB_RDONLY)
+		return -EROFS;
+
+	/* Parse path to get parent directory and basename */
+	path_copy = strdup(path);
+	if (!path_copy)
+		return -ENOMEM;
+
+	last_slash = strrchr(path_copy, '/');
+	if (last_slash) {
+		*last_slash = '\0';
+		dir_path = path_copy;
+		basename = last_slash + 1;
+		if (*dir_path == '\0')
+			dir_path = "/";
+	} else {
+		dir_path = "/";
+		basename = path;
+	}
+
+	/* Resolve parent directory */
+	ret = ext4l_resolve_path(dir_path, &dir_inode);
+	if (ret) {
+		free(path_copy);
+		return ret;
+	}
+
+	if (!S_ISDIR(dir_inode->i_mode)) {
+		free(path_copy);
+		return -ENOTDIR;
+	}
+
+	/* Create dentry for parent directory */
 	dir_dentry = kzalloc(sizeof(struct dentry), GFP_KERNEL);
-	if (!dir_dentry)
+	if (!dir_dentry) {
+		free(path_copy);
 		return -ENOMEM;
-	dir_dentry->d_inode = dir;
-	dir_dentry->d_sb = dir->i_sb;
+	}
+	dir_dentry->d_inode = dir_inode;
+	dir_dentry->d_sb = dir_inode->i_sb;
 
-	/* Create dentry for the filename */
+	/* Create dentry for the file */
 	dentry = kzalloc(sizeof(struct dentry), GFP_KERNEL);
 	if (!dentry) {
 		kfree(dir_dentry);
+		free(path_copy);
 		return -ENOMEM;
 	}
-
-	/* Initialize dentry (kzalloc already zeros memory) */
-	dentry->d_name.name = filename;
-	dentry->d_name.len = strlen(filename);
-	dentry->d_sb = dir->i_sb;
+	dentry->d_name.name = basename;
+	dentry->d_name.len = strlen(basename);
+	dentry->d_sb = dir_inode->i_sb;
 	dentry->d_parent = dir_dentry;
 
-	/* Lookup file */
-	result = ext4_lookup(dir, dentry, 0);
-
+	/* Look up the file */
+	result = ext4_lookup(dir_inode, dentry, 0);
 	if (IS_ERR(result)) {
-		ret = PTR_ERR(result);
-		goto out_dentry;
+		kfree(dentry);
+		kfree(dir_dentry);
+		free(path_copy);
+		return PTR_ERR(result);
 	}
 
-	if (result && result->d_inode) {
+	*dir_dentryp = dir_dentry;
+	*dentryp = dentry;
+	*path_copyp = path_copy;
+
+	return 0;
+}
+
+static int ext4l_write_file(struct dentry *dir_dentry, struct dentry *dentry,
+			    void *buf, loff_t offset, loff_t len,
+			    loff_t *actwrite)
+{
+	struct inode *dir = dir_dentry->d_inode;
+	handle_t *handle = NULL;
+	struct buffer_head *bh;
+	struct inode *inode;
+	loff_t pos, end;
+	umode_t mode;
+	int ret;
+
+	if (dentry->d_inode) {
 		/* File exists - use the existing inode for overwrite */
-		inode = result->d_inode;
-		if (result != dentry) {
-			/* Use the result dentry instead */
-			kfree(dentry);
-			dentry = result;
-		}
+		inode = dentry->d_inode;
 	} else {
-		/* ext4_lookup returned NULL or a dentry with NULL inode */
-		if (result && result != dentry) {
-			/* Free the result dentry since it doesn't have an inode */
-			kfree(result);
-		}
-		/* Keep using our original dentry */
-		dentry->d_inode = NULL;
-
 		/* File does not exist, create it */
 		/* Mode: 0644 (rw-r--r--) | S_IFREG */
 		mode = S_IFREG | 0644;
 		ret = ext4_create(&nop_mnt_idmap, dir, dentry, mode, true);
 		if (ret)
-			goto out_dentry;
+			return ret;
 
 		inode = dentry->d_inode;
 	}
-	if (!inode) {
-		ret = -EIO;
-		goto out_dentry;
-	}
+	if (!inode)
+		return -EIO;
 
 	/*
 	 * Attach jinode for journaling if needed (like ext4_file_open does).
@@ -950,7 +1006,7 @@  static int ext4l_write_file(struct inode *dir, const char *filename, void *buf,
 	 */
 	ret = ext4_inode_attach_jinode(inode);
 	if (ret < 0)
-		goto out_dentry;
+		return ret;
 
 	/*
 	 * Start a journal handle for the write operation.
@@ -959,11 +1015,8 @@  static int ext4l_write_file(struct inode *dir, const char *filename, void *buf,
 	 */
 	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
 				    EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		handle = NULL;
-		goto out_dentry;
-	}
+	if (IS_ERR(handle))
+		return PTR_ERR(handle);
 
 	/* Write data to file */
 	pos = offset;
@@ -1063,84 +1116,92 @@  out_handle:
 	if (handle) {
 		int stop_ret = ext4_journal_stop(handle);
 
-		if (stop_ret) {
-			if (!ret)
-				ret = stop_ret;
-		}
+		if (stop_ret && !ret)
+			ret = stop_ret;
 	}
 
-out_dentry:
-	/*
-	 * Free our manually allocated dentries. In U-Boot's minimal dcache,
-	 * these won't be cached elsewhere.
-	 */
-	kfree(dir_dentry);
-	kfree(dentry);
 	return ret;
 }
 
 int ext4l_write(const char *filename, void *buf, loff_t offset, loff_t len,
 		loff_t *actwrite)
 {
-	struct inode *parent_inode = NULL;
-	char *parent_path = NULL;
-	const char *basename;
+	struct dentry *dir_dentry, *dentry;
 	char *path_copy;
-	char *last_slash;
 	int ret;
 
-	if (!ext4l_sb)
-		return -ENODEV;
-
-	if (!filename || !buf || !actwrite)
+	if (!buf || !actwrite)
 		return -EINVAL;
 
-	/* Check if filesystem is mounted read-write */
-	if (ext4l_sb->s_flags & SB_RDONLY)
-		return -EROFS;
+	ret = ext4l_resolve_file(filename, &dir_dentry, &dentry, &path_copy);
+	if (ret)
+		return ret;
 
-	/* Parse filename to get parent directory and basename */
-	path_copy = strdup(filename);
-	if (!path_copy)
-		return -ENOMEM;
+	/* Call write implementation */
+	ret = ext4l_write_file(dir_dentry, dentry, buf, offset, len, actwrite);
 
-	last_slash = strrchr(path_copy, '/');
+	/* Sync all dirty buffers - U-Boot has no journal thread */
+	if (!ret) {
+		int sync_ret = bh_cache_sync();
 
-	if (last_slash) {
-		*last_slash = '\0';
-		parent_path = path_copy;
-		basename = last_slash + 1;
-		if (*parent_path == '\0') /* Root directory */
-			parent_path = "/";
-	} else {
-		parent_path = "/";
-		basename = filename;
+		if (sync_ret)
+			ret = sync_ret;
 	}
 
-	/* Resolve parent directory inode */
-	ret = ext4l_resolve_path(parent_path, &parent_inode);
-	if (ret) {
-		free(path_copy);
+	kfree(dentry);
+	kfree(dir_dentry);
+	free(path_copy);
+	return ret;
+}
+
+int ext4l_unlink(const char *filename)
+{
+	struct dentry *dentry, *dir_dentry;
+	char *path_copy;
+	int ret;
+
+	ret = ext4l_resolve_file(filename, &dir_dentry, &dentry, &path_copy);
+	if (ret)
 		return ret;
+
+	/* Check if file exists */
+	if (!dentry->d_inode) {
+		ret = -ENOENT;
+		goto out;
 	}
 
-	if (!S_ISDIR(parent_inode->i_mode)) {
-		free(path_copy);
-		return -ENOTDIR;
+	/* Cannot unlink directories with unlink - use rmdir */
+	if (S_ISDIR(dentry->d_inode->i_mode)) {
+		ret = -EISDIR;
+		goto out;
 	}
 
-	/* Call write implementation */
-	ret = ext4l_write_file(parent_inode, basename, buf, offset, len,
-			       actwrite);
+	/* Unlink the file */
+	ret = __ext4_unlink(dir_dentry->d_inode, &dentry->d_name,
+			    dentry->d_inode, dentry);
 
-	/* Sync all dirty buffers - U-Boot has no journal thread */
+	/*
+	 * Release inode - this triggers ext4_evict_inode for nlink=0 inodes,
+	 * which frees the data blocks and inode.
+	 */
+	if (dentry->d_inode) {
+		iput(dentry->d_inode);
+		dentry->d_inode = NULL;
+	}
+
+	/* Sync all dirty buffers after inode eviction */
 	if (!ret) {
 		int sync_ret = bh_cache_sync();
 
 		if (sync_ret)
 			ret = sync_ret;
+		/* Commit superblock with updated free counts */
+		ext4_commit_super(ext4l_sb);
 	}
 
+out:
+	kfree(dentry);
+	kfree(dir_dentry);
 	free(path_copy);
 	return ret;
 }
diff --git a/fs/fs_legacy.c b/fs/fs_legacy.c
index 66545834928..1eb79b338ee 100644
--- a/fs/fs_legacy.c
+++ b/fs/fs_legacy.c
@@ -288,7 +288,7 @@  static struct fstype_info fstypes[] = {
 		.opendir = ext4l_opendir,
 		.readdir = ext4l_readdir,
 		.closedir = ext4l_closedir,
-		.unlink = fs_unlink_unsupported,
+		.unlink = ext4l_op_ptr(ext4l_unlink, fs_unlink_unsupported),
 		.mkdir = fs_mkdir_unsupported,
 		.ln = fs_ln_unsupported,
 		.rename = fs_rename_unsupported,
diff --git a/include/ext4l.h b/include/ext4l.h
index 5dfb671690c..abcf17f99ad 100644
--- a/include/ext4l.h
+++ b/include/ext4l.h
@@ -92,6 +92,15 @@  int ext4l_read(const char *filename, void *buf, loff_t offset, loff_t len,
 int ext4l_write(const char *filename, void *buf, loff_t offset, loff_t len,
 		loff_t *actwrite);
 
+/**
+ * ext4l_unlink() - Delete a file
+ *
+ * @filename: Path to file to delete
+ * Return: 0 on success, -ENOENT if file not found, -EISDIR if path is a
+ *	   directory, -EROFS if read-only, negative on other errors
+ */
+int ext4l_unlink(const char *filename);
+
 /**
  * ext4l_get_uuid() - Get the filesystem UUID
  *
diff --git a/test/fs/ext4l.c b/test/fs/ext4l.c
index 82587a87894..dec6e3340bb 100644
--- a/test/fs/ext4l.c
+++ b/test/fs/ext4l.c
@@ -438,3 +438,44 @@  static int fs_test_ext4l_write_norun(struct unit_test_state *uts)
 }
 FS_TEST_ARGS(fs_test_ext4l_write_norun, UTF_SCAN_FDT | UTF_CONSOLE | UTF_MANUAL,
 	     { "fs_image", UT_ARG_STR });
+
+/**
+ * fs_test_ext4l_unlink_norun() - Test ext4l_unlink function
+ *
+ * Verifies that ext4l can delete files from the filesystem.
+ *
+ * Arguments:
+ *   fs_image: Path to the ext4 filesystem image
+ */
+static int fs_test_ext4l_unlink_norun(struct unit_test_state *uts)
+{
+	const char *fs_image = ut_str(EXT4L_ARG_IMAGE);
+	const char *test_data = "unlink test\n";
+	size_t test_len = strlen(test_data);
+	loff_t actwrite;
+
+	ut_assertnonnull(fs_image);
+	ut_assertok(run_commandf("host bind 0 %s", fs_image));
+	ut_assertok(fs_set_blk_dev("host", "0", FS_TYPE_ANY));
+
+	/* Create a new file to unlink */
+	ut_assertok(ext4l_write("/unlinkme.txt", (void *)test_data, 0,
+				test_len, &actwrite));
+	ut_asserteq(test_len, actwrite);
+
+	/* Verify file exists (same mount) */
+	ut_asserteq(1, ext4l_exists("/unlinkme.txt"));
+
+	/* Unlink the file */
+	ut_assertok(ext4l_unlink("/unlinkme.txt"));
+
+	/* Verify file no longer exists */
+	ut_asserteq(0, ext4l_exists("/unlinkme.txt"));
+
+	/* Verify unlinking non-existent file returns -ENOENT */
+	ut_asserteq(-ENOENT, ext4l_unlink("/nonexistent"));
+
+	return 0;
+}
+FS_TEST_ARGS(fs_test_ext4l_unlink_norun, UTF_SCAN_FDT | UTF_CONSOLE | UTF_MANUAL,
+	     { "fs_image", UT_ARG_STR });
diff --git a/test/py/tests/test_fs/test_ext4l.py b/test/py/tests/test_fs/test_ext4l.py
index e953be379eb..edea38f7faf 100644
--- a/test/py/tests/test_fs/test_ext4l.py
+++ b/test/py/tests/test_fs/test_ext4l.py
@@ -123,3 +123,8 @@  class TestExt4l:
         """Test that ext4l can write file contents."""
         with ubman.log.section('Test ext4l write'):
             ubman.run_ut('fs', 'fs_test_ext4l_write', fs_image=ext4_image)
+
+    def test_unlink(self, ubman, ext4_image):
+        """Test that ext4l can delete files."""
+        with ubman.log.section('Test ext4l unlink'):
+            ubman.run_ut('fs', 'fs_test_ext4l_unlink', fs_image=ext4_image)