public inbox for linux-btrfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
@ 2023-12-09 18:53 Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 1/9] Killing dirstream: add helpers Goffredo Baroncelli
                   ` (9 more replies)
  0 siblings, 10 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
directory returning the 'fd' and a 'dirstream' variable returned by
opendir(3).

If the path is a file, the 'fd' is computed from open(2) and
dirstream is set to NULL.
If the path is a directory, first the directory is opened by opendir(3), then
the 'fd' is computed using dirfd(3).
However the 'dirstream' returned by opendir(3) is left open until 'fd'
is not needed anymore.

In near every case the 'dirstream' variable is not used. Only 'fd' is
used.

A call to close_file_or_dir() freed both 'fd' and 'dirstream'.

Aim of this patch set is to getrid of this complexity; when the path of
a directory is passed, the fd is get directly using open(path, O_RDONLY):
so we don't need to use readdir(3) and to maintain the not used variable
'dirstream'.

So each call of a legacy [btrfs_]open_[file_or_]dir() helper is
replaced by a call to the new btrfs_open_[file_or_]dir_fd() functions.
These functions return only the file descriptor.

Also close_file_or_dir() is not needed anymore.

The first patch, introduces the new helpers; the last patch removed the
unused older helpers. The intermediate patches updated the code.

The 3rd patch updated also the add_seen_fsid() function. Before it
stored the dirstream variable. But now it is not needed anymore.
So we removed a parameter of the functions and a field in the structure.

In the 8th patch, the only occurrences where 'dirstream' is used was
corrected: the dirstream is computed using fdopendir(3), and the cleanup
is updated according.

The results is:
- removed 7 functions
- add 4 new functions
- removed 100 lines
- removed 43 occurrences of an unused 'dirstream' variable.

BR
G.Baroncelli

*** BLURB HERE ***

Goffredo Baroncelli (9):
  Killing dirstream: add helpers
  Killing dirstream: replace btrfs_open_dir with btrfs_open_dir_fd
  Killing dirstream: replace btrfs_open_dir with btrfs_open_dir_fd
  Killing dirstream: replace open_path_or_dev_mnt with btrfs_open_mnt_fd
  Killing dirstream: replace open_file_or_dir3 with btrfs_open_fd2
  Killing dirstream: replace btrfs_open_file_or_dir with
    btrfs_open_file_or_dir_fd
  Killing dirstream: replace open_file_or_dir with btrfs_open_fd2
  Killing dirstream: remove open_file_or_dir3 from du_add_file
  Killing dirstream: remove unused functions

 cmds/balance.c          |  27 ++++-----
 cmds/device.c           |  26 ++++----
 cmds/filesystem-du.c    |  18 +++++-
 cmds/filesystem-usage.c |   5 +-
 cmds/filesystem.c       |  26 ++++----
 cmds/inspect.c          |  35 +++++------
 cmds/property.c         |   5 +-
 cmds/qgroup.c           |  29 ++++-----
 cmds/quota.c            |  16 +++--
 cmds/replace.c          |  17 +++---
 cmds/scrub.c            |  15 ++---
 cmds/subvolume-list.c   |   5 +-
 cmds/subvolume.c        |  44 ++++++--------
 common/device-scan.c    |   6 +-
 common/device-scan.h    |   4 +-
 common/open-utils.c     | 127 ++++++++++++----------------------------
 common/open-utils.h     |  13 ++--
 common/utils.c          |   5 +-
 18 files changed, 164 insertions(+), 259 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/9] Killing dirstream: add helpers
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 2/9] Killing dirstream: replace btrfs_open_dir with btrfs_open_dir_fd Goffredo Baroncelli
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason the helpers [btrfs_]open_dir... return also
the 'DIR *dirstream' value when a dir is opened.

However this is never used. So avoid calling diropen() and return
only the fd.

This is a preparatory patch that adds some helpers.

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 common/open-utils.c | 80 +++++++++++++++++++++++++++++++++++++++++++++
 common/open-utils.h |  5 +++
 2 files changed, 85 insertions(+)

diff --git a/common/open-utils.c b/common/open-utils.c
index 111a51d9..61153294 100644
--- a/common/open-utils.c
+++ b/common/open-utils.c
@@ -318,3 +318,83 @@ void close_file_or_dir(int fd, DIR *dirstream)
 }
 
 
+/*
+ * Do the following checks before calling open:
+ * 1: path is in a btrfs filesystem
+ */
+int btrfs_open_fd2(const char *path, bool verbose, bool read_write, bool dir_only)
+{
+	struct statfs stfs;
+	struct stat st;
+	int ret;
+
+	if (stat(path, &st) != 0) {
+		error_on(verbose, "cannot access '%s': %m", path);
+		return -1;
+	}
+
+	if (statfs(path, &stfs) != 0) {
+		error_on(verbose, "cannot access '%s': %m", path);
+		return -1;
+	}
+
+	if (stfs.f_type != BTRFS_SUPER_MAGIC) {
+		error_on(verbose, "not a btrfs filesystem: %s", path);
+		return -2;
+	}
+
+	if (dir_only && !S_ISDIR(st.st_mode)) {
+		error_on(verbose, "not a directory: %s", path);
+		return -3;
+	}
+
+	if (S_ISDIR(st.st_mode) || !read_write)
+		ret = open(path, O_RDONLY);
+	else
+		ret = open(path, O_RDWR);
+	if (ret < 0) {
+		error_on(verbose, "cannot access '%s': %m", path);
+	}
+
+	return ret;
+}
+
+int btrfs_open_file_or_dir_fd(const char *path)
+{
+	return btrfs_open_fd2(path, true, true, false);
+}
+
+int btrfs_open_dir_fd(const char *path)
+{
+	return btrfs_open_fd2(path, true, true, true);
+}
+
+
+/*
+ * Given a pathname, return a filehandle to:
+ * 	the original pathname or,
+ * 	if the pathname is a mounted btrfs device, to its mountpoint.
+ *
+ * On error, return -1, errno should be set.
+ */
+int btrfs_open_mnt_fd(const char *path, bool verbose)
+{
+	char mp[PATH_MAX];
+	int ret;
+
+	if (path_is_block_device(path)) {
+		ret = get_btrfs_mount(path, mp, sizeof(mp));
+		if (ret < 0) {
+			/* not a mounted btrfs dev */
+			error_on(verbose, "'%s' is not a mounted btrfs device",
+				 path);
+			errno = EINVAL;
+			return -1;
+		}
+		ret = btrfs_open_fd2(mp, verbose, true, true);
+	} else {
+		ret = btrfs_open_dir_fd(path);
+	}
+
+	return ret;
+}
diff --git a/common/open-utils.h b/common/open-utils.h
index 3f8c004e..96d99f5d 100644
--- a/common/open-utils.h
+++ b/common/open-utils.h
@@ -39,4 +39,9 @@ int btrfs_open_file_or_dir(const char *path, DIR **dirstream, int verbose);
 
 void close_file_or_dir(int fd, DIR *dirstream);
 
+int btrfs_open_fd2(const char *path, bool verbose, bool read_write, bool dir_only);
+int btrfs_open_file_or_dir_fd(const char *path);
+int btrfs_open_dir_fd(const char *path);
+int btrfs_open_mnt_fd(const char *path, bool verbose);
+
 #endif
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/9] Killing dirstream: replace btrfs_open_dir with btrfs_open_dir_fd
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 1/9] Killing dirstream: add helpers Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 3/9] " Goffredo Baroncelli
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason the helpers [btrfs_]open_dir... return also
the 'DIR *dirstream' value when a dir is opened.

However this is never used. So avoid calling diropen() and return
only the fd.

This patch replaces btrfs_open_dir() with btrfs_open_dir_fd() removing
any reference to the unused/useless dirstream variables.

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 cmds/balance.c          | 27 +++++++++++----------------
 cmds/device.c           | 21 +++++++++------------
 cmds/filesystem-usage.c |  5 ++---
 cmds/filesystem.c       | 14 ++++++--------
 cmds/inspect.c          | 25 ++++++++++---------------
 cmds/qgroup.c           | 29 ++++++++++++-----------------
 cmds/quota.c            | 16 +++++++---------
 cmds/replace.c          | 10 ++++------
 cmds/subvolume-list.c   |  5 ++---
 cmds/subvolume.c        | 31 +++++++++++++------------------
 10 files changed, 76 insertions(+), 107 deletions(-)

diff --git a/cmds/balance.c b/cmds/balance.c
index 65c7da0b..7c15729c 100644
--- a/cmds/balance.c
+++ b/cmds/balance.c
@@ -299,9 +299,8 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args,
 {
 	int fd;
 	int ret;
-	DIR *dirstream = NULL;
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
@@ -309,7 +308,7 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args,
 	if (ret != 0) {
 		if (ret < 0)
 			error("unable to check status of exclusive operation: %m");
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 		return 1;
 	}
 
@@ -348,7 +347,7 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args,
 	}
 
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return ret;
 }
 
@@ -606,7 +605,6 @@ static int cmd_balance_pause(const struct cmd_struct *cmd,
 	const char *path;
 	int fd;
 	int ret;
-	DIR *dirstream = NULL;
 
 	clean_args_no_options(cmd, argc, argv);
 
@@ -615,7 +613,7 @@ static int cmd_balance_pause(const struct cmd_struct *cmd,
 
 	path = argv[optind];
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
@@ -630,7 +628,7 @@ static int cmd_balance_pause(const struct cmd_struct *cmd,
 	}
 
 	btrfs_warn_multiple_profiles(fd);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return ret;
 }
 static DEFINE_SIMPLE_COMMAND(balance_pause, "pause");
@@ -647,7 +645,6 @@ static int cmd_balance_cancel(const struct cmd_struct *cmd,
 	const char *path;
 	int fd;
 	int ret;
-	DIR *dirstream = NULL;
 
 	clean_args_no_options(cmd, argc, argv);
 
@@ -656,7 +653,7 @@ static int cmd_balance_cancel(const struct cmd_struct *cmd,
 
 	path = argv[optind];
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
@@ -671,7 +668,7 @@ static int cmd_balance_cancel(const struct cmd_struct *cmd,
 	}
 
 	btrfs_warn_multiple_profiles(fd);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return ret;
 }
 static DEFINE_SIMPLE_COMMAND(balance_cancel, "cancel");
@@ -689,7 +686,6 @@ static int cmd_balance_resume(const struct cmd_struct *cmd,
 {
 	struct btrfs_ioctl_balance_args args;
 	const char *path;
-	DIR *dirstream = NULL;
 	int fd;
 	int ret;
 
@@ -700,7 +696,7 @@ static int cmd_balance_resume(const struct cmd_struct *cmd,
 
 	path = argv[optind];
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
@@ -734,7 +730,7 @@ static int cmd_balance_resume(const struct cmd_struct *cmd,
 			   args.stat.completed, args.stat.considered);
 	}
 
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return ret;
 }
 static DEFINE_SIMPLE_COMMAND(balance_resume, "resume");
@@ -760,7 +756,6 @@ static int cmd_balance_status(const struct cmd_struct *cmd,
 {
 	struct btrfs_ioctl_balance_args args;
 	const char *path;
-	DIR *dirstream = NULL;
 	int fd;
 	int ret;
 
@@ -790,7 +785,7 @@ static int cmd_balance_status(const struct cmd_struct *cmd,
 
 	path = argv[optind];
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 2;
 
@@ -828,7 +823,7 @@ static int cmd_balance_status(const struct cmd_struct *cmd,
 
 	ret = 1;
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return ret;
 }
 static DEFINE_SIMPLE_COMMAND(balance_status, "status");
diff --git a/cmds/device.c b/cmds/device.c
index 5e20498d..0b75e110 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -62,7 +62,6 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 {
 	char	*mntpnt;
 	int i, fdmnt, ret = 0;
-	DIR	*dirstream = NULL;
 	bool discard = true;
 	bool force = false;
 	int last_dev;
@@ -105,7 +104,7 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 	last_dev = argc - 1;
 	mntpnt = argv[last_dev];
 
-	fdmnt = btrfs_open_dir(mntpnt, &dirstream, 1);
+	fdmnt = btrfs_open_dir_fd(mntpnt);
 	if (fdmnt < 0)
 		return 1;
 
@@ -113,7 +112,7 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 	if (ret != 0) {
 		if (ret < 0)
 			error("unable to check status of exclusive operation: %m");
-		close_file_or_dir(fdmnt, dirstream);
+		close(fdmnt);
 		return 1;
 	}
 
@@ -181,7 +180,7 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 
 error_out:
 	btrfs_warn_multiple_profiles(fdmnt);
-	close_file_or_dir(fdmnt, dirstream);
+	close(fdmnt);
 	return !!ret;
 }
 static DEFINE_SIMPLE_COMMAND(device_add, "add");
@@ -191,7 +190,6 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 {
 	char	*mntpnt;
 	int i, fdmnt, ret = 0;
-	DIR	*dirstream = NULL;
 	bool enqueue = false;
 	bool cancel = false;
 
@@ -221,7 +219,7 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 
 	mntpnt = argv[argc - 1];
 
-	fdmnt = btrfs_open_dir(mntpnt, &dirstream, 1);
+	fdmnt = btrfs_open_dir_fd(mntpnt);
 	if (fdmnt < 0)
 		return 1;
 
@@ -230,7 +228,7 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 		if (cancel) {
 			error("cancel requested but another device specified: %s\n",
 				argv[i]);
-			close_file_or_dir(fdmnt, dirstream);
+			close(fdmnt);
 			return 1;
 		}
 		if (strcmp("cancel", argv[i]) == 0) {
@@ -246,7 +244,7 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 			if (ret < 0)
 				error(
 			"unable to check status of exclusive operation: %m");
-			close_file_or_dir(fdmnt, dirstream);
+			close(fdmnt);
 			return 1;
 		}
 	}
@@ -312,7 +310,7 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 	}
 
 	btrfs_warn_multiple_profiles(fdmnt);
-	close_file_or_dir(fdmnt, dirstream);
+	close(fdmnt);
 	return !!ret;
 }
 
@@ -875,12 +873,11 @@ static int cmd_device_usage(const struct cmd_struct *cmd, int argc, char **argv)
 
 	for (i = optind; i < argc; i++) {
 		int fd;
-		DIR *dirstream = NULL;
 
 		if (i > 1)
 			pr_verbose(LOG_DEFAULT, "\n");
 
-		fd = btrfs_open_dir(argv[i], &dirstream, 1);
+		fd = btrfs_open_dir_fd(argv[i]);
 		if (fd < 0) {
 			ret = 1;
 			break;
@@ -888,7 +885,7 @@ static int cmd_device_usage(const struct cmd_struct *cmd, int argc, char **argv)
 
 		ret = _cmd_device_usage(fd, argv[i], unit_mode);
 		btrfs_warn_multiple_profiles(fd);
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 
 		if (ret)
 			break;
diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index 0db91e9c..d9d218fe 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -1216,11 +1216,10 @@ static int cmd_filesystem_usage(const struct cmd_struct *cmd,
 
 	for (i = optind; i < argc; i++) {
 		int fd;
-		DIR *dirstream = NULL;
 		struct array chunkinfos = { 0 };
 		struct array devinfos = { 0 };
 
-		fd = btrfs_open_dir(argv[i], &dirstream, 1);
+		fd = btrfs_open_dir_fd(argv[i]);
 		if (fd < 0) {
 			ret = 1;
 			goto out;
@@ -1240,7 +1239,7 @@ static int cmd_filesystem_usage(const struct cmd_struct *cmd,
 		ret = print_filesystem_usage_by_chunk(fd, &chunkinfos,
 				&devinfos, argv[i], unit_mode, tabular);
 cleanup:
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 		array_free_elements(&chunkinfos);
 		array_free(&chunkinfos);
 		array_free_elements(&devinfos);
diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 1b444b8f..cad97238 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -146,7 +146,6 @@ static int cmd_filesystem_df(const struct cmd_struct *cmd,
 	int ret;
 	int fd;
 	char *path;
-	DIR *dirstream = NULL;
 	unsigned unit_mode;
 
 	unit_mode = get_unit_mode_from_arg(&argc, argv, 1);
@@ -158,7 +157,7 @@ static int cmd_filesystem_df(const struct cmd_struct *cmd,
 
 	path = argv[optind];
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
@@ -176,7 +175,7 @@ static int cmd_filesystem_df(const struct cmd_struct *cmd,
 	}
 
 	btrfs_warn_multiple_profiles(fd);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return !!ret;
 }
 static DEFINE_COMMAND_WITH_FLAGS(filesystem_df, "df", CMD_FORMAT_JSON);
@@ -1361,7 +1360,6 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 	struct btrfs_ioctl_vol_args	args;
 	int	fd, res, len, e;
 	char	*amount, *path;
-	DIR	*dirstream = NULL;
 	u64 devid;
 	int ret;
 	bool enqueue = false;
@@ -1399,7 +1397,7 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 
 	cancel = (strcmp("cancel", amount) == 0);
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0) {
 		/* The path is a directory */
 		if (fd == -3) {
@@ -1422,14 +1420,14 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 			if (ret < 0)
 				error(
 			"unable to check status of exclusive operation: %m");
-			close_file_or_dir(fd, dirstream);
+			close(fd);
 			return 1;
 		}
 	}
 
 	ret = check_resize_args(amount, path, &devid);
 	if (ret != 0) {
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 		return 1;
 	}
 
@@ -1444,7 +1442,7 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 	pr_verbose(LOG_VERBOSE, "adjust resize argument to: %s\n", args.name);
 	res = ioctl(fd, BTRFS_IOC_RESIZE, &args);
 	e = errno;
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	if( res < 0 ){
 		switch (e) {
 		case EFBIG:
diff --git a/cmds/inspect.c b/cmds/inspect.c
index a90c373a..86023270 100644
--- a/cmds/inspect.c
+++ b/cmds/inspect.c
@@ -109,7 +109,6 @@ static int cmd_inspect_inode_resolve(const struct cmd_struct *cmd,
 {
 	int fd;
 	int ret;
-	DIR *dirstream = NULL;
 
 	optind = 0;
 	while (1) {
@@ -129,12 +128,12 @@ static int cmd_inspect_inode_resolve(const struct cmd_struct *cmd,
 	if (check_argc_exact(argc - optind, 2))
 		return 1;
 
-	fd = btrfs_open_dir(argv[optind + 1], &dirstream, 1);
+	fd = btrfs_open_dir_fd(argv[optind + 1]);
 	if (fd < 0)
 		return 1;
 
 	ret = __ino_to_path_fd(arg_strtou64(argv[optind]), fd, argv[optind+1]);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return !!ret;
 
 }
@@ -169,7 +168,6 @@ static int cmd_inspect_logical_resolve(const struct cmd_struct *cmd,
 	u64 size = SZ_64K;
 	char full_path[PATH_MAX];
 	char *path_ptr;
-	DIR *dirstream = NULL;
 	u64 flags = 0;
 	unsigned long request = BTRFS_IOC_LOGICAL_INO;
 
@@ -214,7 +212,7 @@ static int cmd_inspect_logical_resolve(const struct cmd_struct *cmd,
 	loi.flags = flags;
 	loi.inodes = ptr_to_u64(inodes);
 
-	fd = btrfs_open_dir(argv[optind + 1], &dirstream, 1);
+	fd = btrfs_open_dir_fd(argv[optind + 1]);
 	if (fd < 0) {
 		ret = 12;
 		goto out;
@@ -247,7 +245,6 @@ static int cmd_inspect_logical_resolve(const struct cmd_struct *cmd,
 		u64 offset = inodes->val[i+1];
 		u64 root = inodes->val[i+2];
 		int path_fd;
-		DIR *dirs = NULL;
 
 		if (getpath) {
 			char mount_path[PATH_MAX];
@@ -296,7 +293,7 @@ static int cmd_inspect_logical_resolve(const struct cmd_struct *cmd,
 				strncpy(mount_path, mounted, PATH_MAX);
 				free(mounted);
 
-				path_fd = btrfs_open_dir(mount_path, &dirs, 1);
+				path_fd = btrfs_open_dir_fd(mount_path);
 				if (path_fd < 0) {
 					ret = -ENOENT;
 					goto out;
@@ -304,7 +301,7 @@ static int cmd_inspect_logical_resolve(const struct cmd_struct *cmd,
 			}
 			ret = __ino_to_path_fd(inum, path_fd, mount_path);
 			if (path_fd != fd)
-				close_file_or_dir(path_fd, dirs);
+				close(path_fd);
 		} else {
 			pr_verbose(LOG_DEFAULT, "inode %llu offset %llu root %llu\n", inum,
 				offset, root);
@@ -312,7 +309,7 @@ static int cmd_inspect_logical_resolve(const struct cmd_struct *cmd,
 	}
 
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	free(inodes);
 	return !!ret;
 }
@@ -331,14 +328,13 @@ static int cmd_inspect_subvolid_resolve(const struct cmd_struct *cmd,
 	int fd = -1;
 	u64 subvol_id;
 	char path[PATH_MAX];
-	DIR *dirstream = NULL;
 
 	clean_args_no_options(cmd, argc, argv);
 
 	if (check_argc_exact(argc - optind, 2))
 		return 1;
 
-	fd = btrfs_open_dir(argv[optind + 1], &dirstream, 1);
+	fd = btrfs_open_dir_fd(argv[optind + 1]);
 	if (fd < 0) {
 		ret = -ENOENT;
 		goto out;
@@ -356,7 +352,7 @@ static int cmd_inspect_subvolid_resolve(const struct cmd_struct *cmd,
 	pr_verbose(LOG_DEFAULT, "%s\n", path);
 
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return !!ret;
 }
 static DEFINE_SIMPLE_COMMAND(inspect_subvolid_resolve, "subvolid-resolve");
@@ -655,7 +651,6 @@ static int cmd_inspect_min_dev_size(const struct cmd_struct *cmd,
 {
 	int ret;
 	int fd = -1;
-	DIR *dirstream = NULL;
 	u64 devid = 1;
 
 	optind = 0;
@@ -682,14 +677,14 @@ static int cmd_inspect_min_dev_size(const struct cmd_struct *cmd,
 	if (check_argc_exact(argc - optind, 1))
 		return 1;
 
-	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
+	fd = btrfs_open_dir_fd(argv[optind]);
 	if (fd < 0) {
 		ret = -ENOENT;
 		goto out;
 	}
 
 	ret = print_min_dev_size(fd, devid);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 out:
 	return !!ret;
 }
diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index 2f1893cb..43f973d7 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -1740,7 +1740,6 @@ static int _cmd_qgroup_assign(const struct cmd_struct *cmd, int assign,
 	bool rescan = true;
 	char *path;
 	struct btrfs_ioctl_qgroup_assign_args args;
-	DIR *dirstream = NULL;
 
 	optind = 0;
 	while (1) {
@@ -1785,7 +1784,7 @@ static int _cmd_qgroup_assign(const struct cmd_struct *cmd, int assign,
 		error("bad relation requested: %s", path);
 		return 1;
 	}
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
@@ -1794,7 +1793,7 @@ static int _cmd_qgroup_assign(const struct cmd_struct *cmd, int assign,
 		error("unable to assign quota group: %s",
 				errno == ENOTCONN ? "quota not enabled"
 						: strerror(errno));
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 		return 1;
 	}
 
@@ -1820,7 +1819,7 @@ static int _cmd_qgroup_assign(const struct cmd_struct *cmd, int assign,
 			ret = 0;
 		}
 	}
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return ret;
 }
 
@@ -1830,7 +1829,6 @@ static int _cmd_qgroup_create(int create, int argc, char **argv)
 	int fd;
 	char *path;
 	struct btrfs_ioctl_qgroup_create_args args;
-	DIR *dirstream = NULL;
 
 	if (check_argc_exact(argc - optind, 2))
 		return 1;
@@ -1844,12 +1842,12 @@ static int _cmd_qgroup_create(int create, int argc, char **argv)
 	}
 	path = argv[optind + 1];
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
 	ret = ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	if (ret < 0) {
 		error("unable to %s quota group: %s",
 			create ? "create":"destroy",
@@ -1957,7 +1955,6 @@ static int cmd_qgroup_show(const struct cmd_struct *cmd, int argc, char **argv)
 	char *path;
 	int ret = 0;
 	int fd;
-	DIR *dirstream = NULL;
 	u64 qgroupid;
 	int filter_flag = 0;
 	unsigned unit_mode;
@@ -2030,7 +2027,7 @@ static int cmd_qgroup_show(const struct cmd_struct *cmd, int argc, char **argv)
 		return 1;
 
 	path = argv[optind];
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0) {
 		free(filter_set);
 		free(comparer_set);
@@ -2048,7 +2045,7 @@ static int cmd_qgroup_show(const struct cmd_struct *cmd, int argc, char **argv)
 		if (ret < 0) {
 			errno = -ret;
 			error("cannot resolve rootid for %s: %m", path);
-			close_file_or_dir(fd, dirstream);
+			close(fd);
 			goto out;
 		}
 		if (filter_flag & 0x1)
@@ -2061,7 +2058,7 @@ static int cmd_qgroup_show(const struct cmd_struct *cmd, int argc, char **argv)
 					qgroupid);
 	}
 	ret = show_qgroups(fd, filter_set, comparer_set);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	free(filter_set);
 	free(comparer_set);
 
@@ -2088,7 +2085,6 @@ static int cmd_qgroup_limit(const struct cmd_struct *cmd, int argc, char **argv)
 	unsigned long long size;
 	bool compressed = false;
 	bool exclusive = false;
-	DIR *dirstream = NULL;
 	enum btrfs_util_error err;
 
 	optind = 0;
@@ -2146,12 +2142,12 @@ static int cmd_qgroup_limit(const struct cmd_struct *cmd, int argc, char **argv)
 	} else
 		usage(cmd, 1);
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
 	ret = ioctl(fd, BTRFS_IOC_QGROUP_LIMIT, &args);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	if (ret < 0) {
 		error("unable to limit requested quota group: %s",
 				errno == ENOTCONN ? "quota not enabled"
@@ -2179,7 +2175,6 @@ static int cmd_qgroup_clear_stale(const struct cmd_struct *cmd, int argc, char *
 	int ret = 0;
 	int fd;
 	char *path = NULL;
-	DIR *dirstream = NULL;
 	struct qgroup_lookup qgroup_lookup;
 	struct rb_node *node;
 	struct btrfs_qgroup *entry;
@@ -2189,7 +2184,7 @@ static int cmd_qgroup_clear_stale(const struct cmd_struct *cmd, int argc, char *
 
 	path = argv[optind];
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
@@ -2225,7 +2220,7 @@ static int cmd_qgroup_clear_stale(const struct cmd_struct *cmd, int argc, char *
 	}
 
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return !!ret;
 }
 static DEFINE_SIMPLE_COMMAND(qgroup_clear_stale, "clear-stale");
diff --git a/cmds/quota.c b/cmds/quota.c
index fa069d79..adf7bf1a 100644
--- a/cmds/quota.c
+++ b/cmds/quota.c
@@ -41,17 +41,16 @@ static int quota_ctl(int cmd, char *path)
 	int ret = 0;
 	int fd;
 	struct btrfs_ioctl_quota_ctl_args args;
-	DIR *dirstream = NULL;
 
 	memset(&args, 0, sizeof(args));
 	args.cmd = cmd;
 
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
 	ret = ioctl(fd, BTRFS_IOC_QUOTA_CTL, &args);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	if (ret < 0) {
 		error("quota command failed: %m");
 		return 1;
@@ -148,7 +147,6 @@ static int cmd_quota_rescan(const struct cmd_struct *cmd, int argc, char **argv)
 	char *path = NULL;
 	struct btrfs_ioctl_quota_rescan_args args;
 	unsigned long ioctlnum = BTRFS_IOC_QUOTA_RESCAN;
-	DIR *dirstream = NULL;
 	bool wait_for_completion = false;
 
 	optind = 0;
@@ -193,7 +191,7 @@ static int cmd_quota_rescan(const struct cmd_struct *cmd, int argc, char **argv)
 	memset(&args, 0, sizeof(args));
 
 	path = argv[optind];
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
@@ -203,7 +201,7 @@ static int cmd_quota_rescan(const struct cmd_struct *cmd, int argc, char **argv)
 	}
 
 	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN_STATUS) {
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 		if (ret < 0) {
 			error("could not obtain quota rescan status: %m");
 			return 1;
@@ -221,7 +219,7 @@ static int cmd_quota_rescan(const struct cmd_struct *cmd, int argc, char **argv)
 		fflush(stdout);
 	} else if (ret < 0 && (!wait_for_completion || e != EINPROGRESS)) {
 		error("quota rescan failed: %m");
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 		return 1;
 	}
 
@@ -230,12 +228,12 @@ static int cmd_quota_rescan(const struct cmd_struct *cmd, int argc, char **argv)
 		e = errno;
 		if (ret < 0) {
 			error("quota rescan wait failed: %m");
-			close_file_or_dir(fd, dirstream);
+			close(fd);
 			return 1;
 		}
 	}
 
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return 0;
 }
 static DEFINE_SIMPLE_COMMAND(quota_rescan, "rescan");
diff --git a/cmds/replace.c b/cmds/replace.c
index 138a22e4..171a72b4 100644
--- a/cmds/replace.c
+++ b/cmds/replace.c
@@ -378,7 +378,6 @@ static int cmd_replace_status(const struct cmd_struct *cmd,
 	char *path;
 	int once = 0;
 	int ret;
-	DIR *dirstream = NULL;
 
 	optind = 0;
 	while ((c = getopt(argc, argv, "1")) != -1) {
@@ -395,12 +394,12 @@ static int cmd_replace_status(const struct cmd_struct *cmd,
 		return 1;
 
 	path = argv[optind];
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
 	ret = print_replace_status(fd, path, once);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return !!ret;
 }
 static DEFINE_SIMPLE_COMMAND(replace_status, "status");
@@ -546,7 +545,6 @@ static int cmd_replace_cancel(const struct cmd_struct *cmd,
 	int c;
 	int fd;
 	char *path;
-	DIR *dirstream = NULL;
 
 	optind = 0;
 	while ((c = getopt(argc, argv, "")) != -1) {
@@ -561,14 +559,14 @@ static int cmd_replace_cancel(const struct cmd_struct *cmd,
 		return 1;
 
 	path = argv[optind];
-	fd = btrfs_open_dir(path, &dirstream, 1);
+	fd = btrfs_open_dir_fd(path);
 	if (fd < 0)
 		return 1;
 
 	args.cmd = BTRFS_IOCTL_DEV_REPLACE_CMD_CANCEL;
 	args.result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT;
 	ret = ioctl(fd, BTRFS_IOC_DEV_REPLACE, &args);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	if (ret < 0) {
 		error("ioctl(DEV_REPLACE_CANCEL) failed on \"%s\": %m", path);
 		if (args.result != BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT)
diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
index 5a91f41d..a26e8b02 100644
--- a/cmds/subvolume-list.c
+++ b/cmds/subvolume-list.c
@@ -1587,7 +1587,6 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 	char *subvol;
 	bool is_list_all = false;
 	bool is_only_in_path = false;
-	DIR *dirstream = NULL;
 	enum btrfs_list_layout layout = BTRFS_LIST_LAYOUT_DEFAULT;
 
 	filter_set = btrfs_list_alloc_filter_set();
@@ -1689,7 +1688,7 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 		goto out;
 
 	subvol = argv[optind];
-	fd = btrfs_open_dir(subvol, &dirstream, 1);
+	fd = btrfs_open_dir_fd(subvol);
 	if (fd < 0) {
 		ret = -1;
 		error("can't access '%s'", subvol);
@@ -1729,7 +1728,7 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 			layout, !is_list_all && !is_only_in_path, NULL);
 
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	if (filter_set)
 		free(filter_set);
 	if (comparer_set)
diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 8504c380..17fb23fb 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -136,7 +136,6 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
 	char	*dstdir;
 	char	*dst;
 	struct btrfs_qgroup_inherit *inherit = NULL;
-	DIR	*dirstream = NULL;
 	bool create_parents = false;
 
 	optind = 0;
@@ -238,7 +237,7 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
 		}
 	}
 
-	fddst = btrfs_open_dir(dstdir, &dirstream, 1);
+	fddst = btrfs_open_dir_fd(dstdir);
 	if (fddst < 0)
 		goto out;
 
@@ -269,7 +268,7 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
 
 	retval = 0;	/* success */
 out:
-	close_file_or_dir(fddst, dirstream);
+	close(fddst);
 	free(inherit);
 	free(dupname);
 	free(dupdir);
@@ -610,7 +609,6 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
 	enum btrfs_util_error err;
 	struct btrfs_ioctl_vol_args_v2	args;
 	struct btrfs_qgroup_inherit *inherit = NULL;
-	DIR *dirstream1 = NULL, *dirstream2 = NULL;
 
 	memset(&args, 0, sizeof(args));
 	optind = 0;
@@ -697,11 +695,11 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
 		goto out;
 	}
 
-	fddst = btrfs_open_dir(dstdir, &dirstream1, 1);
+	fddst = btrfs_open_dir_fd(dstdir);
 	if (fddst < 0)
 		goto out;
 
-	fd = btrfs_open_dir(subvol, &dirstream2, 1);
+	fd = btrfs_open_dir_fd(subvol);
 	if (fd < 0)
 		goto out;
 
@@ -736,8 +734,8 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
 	retval = 0;	/* success */
 
 out:
-	close_file_or_dir(fddst, dirstream1);
-	close_file_or_dir(fd, dirstream2);
+	close(fddst);
+	close(fd);
 	free(inherit);
 	free(dupname);
 	free(dupdir);
@@ -761,7 +759,6 @@ static int cmd_subvolume_get_default(const struct cmd_struct *cmd, int argc, cha
 	int fd = -1;
 	int ret = 1;
 	uint64_t default_id;
-	DIR *dirstream = NULL;
 	enum btrfs_util_error err;
 	struct btrfs_util_subvolume_info subvol;
 	struct format_ctx fctx;
@@ -772,7 +769,7 @@ static int cmd_subvolume_get_default(const struct cmd_struct *cmd, int argc, cha
 	if (check_argc_exact(argc - optind, 1))
 		return 1;
 
-	fd = btrfs_open_dir(argv[1], &dirstream, 1);
+	fd = btrfs_open_dir_fd(argv[1]);
 	if (fd < 0)
 		return 1;
 
@@ -824,7 +821,7 @@ static int cmd_subvolume_get_default(const struct cmd_struct *cmd, int argc, cha
 
 	ret = 0;
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return ret;
 }
 #if EXPERIMENTAL
@@ -1295,7 +1292,6 @@ static int cmd_subvolume_find_new(const struct cmd_struct *cmd, int argc, char *
 	int ret;
 	char *subvol;
 	u64 last_gen;
-	DIR *dirstream = NULL;
 	enum btrfs_util_error err;
 
 	clean_args_no_options(cmd, argc, argv);
@@ -1312,19 +1308,19 @@ static int cmd_subvolume_find_new(const struct cmd_struct *cmd, int argc, char *
 		return 1;
 	}
 
-	fd = btrfs_open_dir(subvol, &dirstream, 1);
+	fd = btrfs_open_dir_fd(subvol);
 	if (fd < 0)
 		return 1;
 
 	err = btrfs_util_sync_fd(fd);
 	if (err) {
 		error_btrfs_util(err);
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 		return 1;
 	}
 
 	ret = btrfs_list_find_updated_files(fd, 0, last_gen);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return !!ret;
 }
 static DEFINE_SIMPLE_COMMAND(subvolume_find_new, "find-new");
@@ -1721,7 +1717,6 @@ static int cmd_subvolume_sync(const struct cmd_struct *cmd, int argc, char **arg
 {
 	int fd = -1;
 	int ret = 1;
-	DIR *dirstream = NULL;
 	uint64_t *ids = NULL;
 	size_t id_count, i;
 	int sleep_interval = 1;
@@ -1751,7 +1746,7 @@ static int cmd_subvolume_sync(const struct cmd_struct *cmd, int argc, char **arg
 	if (check_argc_min(argc - optind, 1))
 		return 1;
 
-	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
+	fd = btrfs_open_dir_fd(argv[optind]);
 	if (fd < 0) {
 		ret = 1;
 		goto out;
@@ -1804,7 +1799,7 @@ static int cmd_subvolume_sync(const struct cmd_struct *cmd, int argc, char **arg
 
 out:
 	free(ids);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 
 	return !!ret;
 }
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/9] Killing dirstream: replace btrfs_open_dir with btrfs_open_dir_fd
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 1/9] Killing dirstream: add helpers Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 2/9] Killing dirstream: replace btrfs_open_dir with btrfs_open_dir_fd Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 4/9] Killing dirstream: replace open_path_or_dev_mnt with btrfs_open_mnt_fd Goffredo Baroncelli
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason the helpers [btrfs_]open_dir... return also
the 'DIR *dirstream' value when a dir is opened.

However this is never used. So avoid calling diropen() and return
only the fd.

This patch replaces the last btrfs_open_dir() call with btrfs_open_dir_fd()
removing any reference to the unused/useless dirstream variables.

At the same time this patch updates the add_seen_fsid() function removing
any reference to dir stream (again this is never used).

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 cmds/filesystem.c    | 4 ++--
 cmds/subvolume.c     | 8 +++-----
 common/device-scan.c | 6 ++----
 common/device-scan.h | 4 +---
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index cad97238..9a89e2c6 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -306,7 +306,7 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices,
 	u64 devs_found = 0;
 	u64 total;
 
-	if (add_seen_fsid(fs_devices->fsid, seen_fsid_hash, -1, NULL))
+	if (add_seen_fsid(fs_devices->fsid, seen_fsid_hash, -1))
 		return;
 
 	uuid_unparse(fs_devices->fsid, uuidbuf);
@@ -351,7 +351,7 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
 	struct btrfs_ioctl_dev_info_args *tmp_dev_info;
 	int ret;
 
-	ret = add_seen_fsid(fs_info->fsid, seen_fsid_hash, -1, NULL);
+	ret = add_seen_fsid(fs_info->fsid, seen_fsid_hash, -1);
 	if (ret == -EEXIST)
 		return 0;
 	else if (ret)
diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 17fb23fb..cc1a660b 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -325,7 +325,6 @@ static int cmd_subvolume_delete(const struct cmd_struct *cmd, int argc, char **a
 	char	*dupdname = NULL;
 	char	*dupvname = NULL;
 	char	*path = NULL;
-	DIR	*dirstream = NULL;
 	int commit_mode = 0;
 	bool subvol_path_not_found = false;
 	u8 fsid[BTRFS_FSID_SIZE];
@@ -438,7 +437,7 @@ again:
 	if (subvolid > 0)
 		dname = dupvname;
 
-	fd = btrfs_open_dir(dname, &dirstream, 1);
+	fd = btrfs_open_dir_fd(dname);
 	if (fd < 0) {
 		ret = 1;
 		goto out;
@@ -518,7 +517,7 @@ again:
 			goto out;
 		}
 
-		if (add_seen_fsid(fsid, seen_fsid_hash, fd, dirstream) == 0) {
+		if (add_seen_fsid(fsid, seen_fsid_hash, fd) == 0) {
 			uuid_unparse(fsid, uuidbuf);
 			pr_verbose(LOG_INFO, "  new fs is found for '%s', fsid: %s\n",
 				   path, uuidbuf);
@@ -532,10 +531,9 @@ again:
 	}
 
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 keep_fd:
 	fd = -1;
-	dirstream = NULL;
 	free(dupdname);
 	free(dupvname);
 	dupdname = NULL;
diff --git a/common/device-scan.c b/common/device-scan.c
index c1cd7266..c04c2388 100644
--- a/common/device-scan.c
+++ b/common/device-scan.c
@@ -313,8 +313,7 @@ int is_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[])
 	return 0;
 }
 
-int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[],
-		int fd, DIR *dirstream)
+int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[], int fd)
 {
 	u8 hash = fsid[0];
 	int slot = hash % SEEN_FSID_HASH_SIZE;
@@ -342,7 +341,6 @@ insert:
 	alloc->next = NULL;
 	memcpy(alloc->fsid, fsid, BTRFS_FSID_SIZE);
 	alloc->fd = fd;
-	alloc->dirstream = dirstream;
 
 	if (seen)
 		seen->next = alloc;
@@ -362,7 +360,7 @@ void free_seen_fsid(struct seen_fsid *seen_fsid_hash[])
 		seen = seen_fsid_hash[slot];
 		while (seen) {
 			next = seen->next;
-			close_file_or_dir(seen->fd, seen->dirstream);
+			close(seen->fd);
 			free(seen);
 			seen = next;
 		}
diff --git a/common/device-scan.h b/common/device-scan.h
index 8a875832..90ec766d 100644
--- a/common/device-scan.h
+++ b/common/device-scan.h
@@ -41,7 +41,6 @@ struct btrfs_trans_handle;
 struct seen_fsid {
 	u8 fsid[BTRFS_FSID_SIZE];
 	struct seen_fsid *next;
-	DIR *dirstream;
 	int fd;
 };
 
@@ -56,8 +55,7 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
 				 int super_offset);
 int is_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[]);
-int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[],
-		int fd, DIR *dirstream);
+int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[], int fd);
 void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]);
 int test_uuid_unique(const char *uuid_str);
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/9] Killing dirstream: replace open_path_or_dev_mnt with btrfs_open_mnt_fd
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
                   ` (2 preceding siblings ...)
  2023-12-09 18:53 ` [PATCH 3/9] " Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 5/9] Killing dirstream: replace open_file_or_dir3 with btrfs_open_fd2 Goffredo Baroncelli
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason the helpers [btrfs_]open_dir... return also
the 'DIR *dirstream' value when a dir is opened.

However this is never used. So avoid calling diropen() and return
only the fd.

This patch replace open_path_or_dev_mnt() with btrfs_open_mnt_fd() removing
any reference to the unused/useless dirstream variables.

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 cmds/device.c  |  5 ++---
 cmds/replace.c |  7 +++----
 cmds/scrub.c   | 15 ++++++---------
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/cmds/device.c b/cmds/device.c
index 0b75e110..3ff7120f 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -692,7 +692,6 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 	bool free_table = false;
 	bool tabular = false;
 	__u64 flags = 0;
-	DIR *dirstream = NULL;
 	struct format_ctx fctx;
 
 	optind = 0;
@@ -728,7 +727,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 
 	dev_path = argv[optind];
 
-	fdmnt = open_path_or_dev_mnt(dev_path, &dirstream, 1);
+	fdmnt = btrfs_open_mnt_fd(dev_path, true);
 	if (fdmnt < 0)
 		return 1;
 
@@ -814,7 +813,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
 
 out:
 	free(di_args);
-	close_file_or_dir(fdmnt, dirstream);
+	close(fdmnt);
 	if (free_table)
 		table_free(table);
 
diff --git a/cmds/replace.c b/cmds/replace.c
index 171a72b4..f1db9477 100644
--- a/cmds/replace.c
+++ b/cmds/replace.c
@@ -136,7 +136,6 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 	bool force_using_targetdev = false;
 	u64 dstdev_block_count;
 	bool do_not_background = false;
-	DIR *dirstream = NULL;
 	u64 srcdev_size;
 	u64 dstdev_size;
 	bool enqueue = false;
@@ -184,7 +183,7 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 		return 1;
 	path = argv[optind + 2];
 
-	fdmnt = open_path_or_dev_mnt(path, &dirstream, 1);
+	fdmnt = btrfs_open_mnt_fd(path, true);
 	if (fdmnt < 0)
 		goto leave_with_error;
 
@@ -200,7 +199,7 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 	if (ret != 0) {
 		if (ret < 0)
 			error("unable to check status of exclusive operation: %m");
-		close_file_or_dir(fdmnt, dirstream);
+		close(fdmnt);
 		goto leave_with_error;
 	}
 
@@ -348,7 +347,7 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 			goto leave_with_error;
 		}
 	}
-	close_file_or_dir(fdmnt, dirstream);
+	close(fdmnt);
 	return 0;
 
 leave_with_error:
diff --git a/cmds/scrub.c b/cmds/scrub.c
index 4a741355..85626d11 100644
--- a/cmds/scrub.c
+++ b/cmds/scrub.c
@@ -1203,7 +1203,6 @@ static int scrub_start(const struct cmd_struct *cmd, int argc, char **argv,
 	pthread_mutex_t spc_write_mutex = PTHREAD_MUTEX_INITIALIZER;
 	void *terr;
 	u64 devid;
-	DIR *dirstream = NULL;
 	bool force = false;
 	bool nothing_to_resume = false;
 
@@ -1260,7 +1259,7 @@ static int scrub_start(const struct cmd_struct *cmd, int argc, char **argv,
 
 	path = argv[optind];
 
-	fdmnt = open_path_or_dev_mnt(path, &dirstream, !do_quiet);
+	fdmnt = btrfs_open_mnt_fd(path, !do_quiet);
 	if (fdmnt < 0)
 		return 1;
 
@@ -1618,7 +1617,7 @@ out:
 		if (sock_path[0])
 			unlink(sock_path);
 	}
-	close_file_or_dir(fdmnt, dirstream);
+	close(fdmnt);
 
 	if (err)
 		return 1;
@@ -1671,7 +1670,6 @@ static int cmd_scrub_cancel(const struct cmd_struct *cmd, int argc, char **argv)
 	char *path;
 	int ret;
 	int fdmnt = -1;
-	DIR *dirstream = NULL;
 
 	clean_args_no_options(cmd, argc, argv);
 
@@ -1680,7 +1678,7 @@ static int cmd_scrub_cancel(const struct cmd_struct *cmd, int argc, char **argv)
 
 	path = argv[optind];
 
-	fdmnt = open_path_or_dev_mnt(path, &dirstream, 1);
+	fdmnt = btrfs_open_mnt_fd(path, true);
 	if (fdmnt < 0) {
 		ret = 1;
 		goto out;
@@ -1702,7 +1700,7 @@ static int cmd_scrub_cancel(const struct cmd_struct *cmd, int argc, char **argv)
 	pr_verbose(LOG_DEFAULT, "scrub cancelled\n");
 
 out:
-	close_file_or_dir(fdmnt, dirstream);
+	close(fdmnt);
 	return ret;
 }
 static DEFINE_SIMPLE_COMMAND(scrub_cancel, "cancel");
@@ -1761,7 +1759,6 @@ static int cmd_scrub_status(const struct cmd_struct *cmd, int argc, char **argv)
 	char fsid[BTRFS_UUID_UNPARSED_SIZE];
 	int fdres = -1;
 	int err = 0;
-	DIR *dirstream = NULL;
 
 	unit_mode = get_unit_mode_from_arg(&argc, argv, 0);
 
@@ -1784,7 +1781,7 @@ static int cmd_scrub_status(const struct cmd_struct *cmd, int argc, char **argv)
 
 	path = argv[optind];
 
-	fdmnt = open_path_or_dev_mnt(path, &dirstream, 1);
+	fdmnt = btrfs_open_mnt_fd(path, true);
 	if (fdmnt < 0)
 		return 1;
 
@@ -1888,7 +1885,7 @@ out:
 	free(si_args);
 	if (fdres > -1)
 		close(fdres);
-	close_file_or_dir(fdmnt, dirstream);
+	close(fdmnt);
 
 	return !!err;
 }
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 5/9] Killing dirstream: replace open_file_or_dir3 with btrfs_open_fd2
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
                   ` (3 preceding siblings ...)
  2023-12-09 18:53 ` [PATCH 4/9] Killing dirstream: replace open_path_or_dev_mnt with btrfs_open_mnt_fd Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 6/9] Killing dirstream: replace btrfs_open_file_or_dir with btrfs_open_file_or_dir_fd Goffredo Baroncelli
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason the helpers [btrfs_]open_dir... return also
the 'DIR *dirstream' value when a dir is opened.

However this is never used. So avoid calling diropen() and return
only the fd.

This patch replace open_file_or_dir3() with btrfs_open_fd2() removing
any reference to the unused/useless dirstream variables.
btrfs_open_fd2() is needed because sometime the caller need
to set the RDONLY/RDWRITE mode, and to avoid spourios diagnosis messages.

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 cmds/filesystem.c | 8 +++-----
 cmds/property.c   | 5 ++---
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 9a89e2c6..65370886 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -1003,7 +1003,6 @@ static int cmd_filesystem_defrag(const struct cmd_struct *cmd,
 	bool recursive = false;
 	int ret = 0;
 	int compress_type = BTRFS_COMPRESS_NONE;
-	DIR *dirstream;
 
 	/*
 	 * Kernel 4.19+ supports defragmention of files open read-only,
@@ -1141,8 +1140,7 @@ static int cmd_filesystem_defrag(const struct cmd_struct *cmd,
 		struct stat st;
 		int defrag_err = 0;
 
-		dirstream = NULL;
-		fd = open_file_or_dir3(argv[i], &dirstream, defrag_open_mode);
+		fd = btrfs_open_fd2(argv[i], false, defrag_open_mode==O_RDWR, false);
 		if (fd < 0) {
 			error("cannot open %s: %m", argv[i]);
 			ret = -errno;
@@ -1176,7 +1174,7 @@ static int cmd_filesystem_defrag(const struct cmd_struct *cmd,
 				error(
 "defrag range ioctl not supported in this kernel version, 2.6.33 and newer is required");
 				defrag_global_errors++;
-				close_file_or_dir(fd, dirstream);
+				close(fd);
 				break;
 			}
 			if (ret) {
@@ -1188,7 +1186,7 @@ static int cmd_filesystem_defrag(const struct cmd_struct *cmd,
 next:
 		if (ret)
 			defrag_global_errors++;
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 	}
 
 	if (defrag_global_errors)
diff --git a/cmds/property.c b/cmds/property.c
index be9bdf63..e189e505 100644
--- a/cmds/property.c
+++ b/cmds/property.c
@@ -175,12 +175,11 @@ static int prop_compression(enum prop_object_type type,
 	int ret;
 	ssize_t sret;
 	int fd = -1;
-	DIR *dirstream = NULL;
 	char *buf = NULL;
 	char *xattr_name = NULL;
 	int open_flags = value ? O_RDWR : O_RDONLY;
 
-	fd = open_file_or_dir3(object, &dirstream, open_flags);
+	fd = btrfs_open_fd2(object, false, open_flags == O_RDWR, false);
 	if (fd == -1) {
 		ret = -errno;
 		error("failed to open %s: %m", object);
@@ -232,7 +231,7 @@ out:
 	free(xattr_name);
 	free(buf);
 	if (fd >= 0)
-		close_file_or_dir(fd, dirstream);
+		close(fd);
 
 	return ret;
 }
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 6/9] Killing dirstream: replace btrfs_open_file_or_dir with btrfs_open_file_or_dir_fd
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
                   ` (4 preceding siblings ...)
  2023-12-09 18:53 ` [PATCH 5/9] Killing dirstream: replace open_file_or_dir3 with btrfs_open_fd2 Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 7/9] Killing dirstream: replace open_file_or_dir with btrfs_open_fd2 Goffredo Baroncelli
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason the helpers [btrfs_]open_dir... return also
the 'DIR *dirstream' value when a dir is opened.

However this is never used. So avoid calling diropen() and return
only the fd.

This patch replace btrfs_open_file_or_dir() with btrfs_open_file_or_dir_fd()
removing any reference to the unused/useless dirstream variables.

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 cmds/inspect.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/cmds/inspect.c b/cmds/inspect.c
index 86023270..4d4e24d2 100644
--- a/cmds/inspect.c
+++ b/cmds/inspect.c
@@ -369,14 +369,13 @@ static int cmd_inspect_rootid(const struct cmd_struct *cmd,
 	int ret;
 	int fd = -1;
 	u64 rootid;
-	DIR *dirstream = NULL;
 
 	clean_args_no_options(cmd, argc, argv);
 
 	if (check_argc_exact(argc - optind, 1))
 		return 1;
 
-	fd = btrfs_open_file_or_dir(argv[optind], &dirstream, 1);
+	fd = btrfs_open_file_or_dir_fd(argv[optind]);
 	if (fd < 0) {
 		ret = -ENOENT;
 		goto out;
@@ -391,7 +390,7 @@ static int cmd_inspect_rootid(const struct cmd_struct *cmd,
 
 	pr_verbose(LOG_DEFAULT, "%llu\n", rootid);
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 
 	return !!ret;
 }
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 7/9] Killing dirstream: replace open_file_or_dir with btrfs_open_fd2
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
                   ` (5 preceding siblings ...)
  2023-12-09 18:53 ` [PATCH 6/9] Killing dirstream: replace btrfs_open_file_or_dir with btrfs_open_file_or_dir_fd Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 8/9] Killing dirstream: remove open_file_or_dir3 from du_add_file Goffredo Baroncelli
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason the helpers [btrfs_]open_dir... return also
the 'DIR *dirstream' value when a dir is opened.

However this is never used. So avoid calling diropen() and return
only the fd.

This patch replace open_file_or_dir() with btrfs_open_fd2() removing
any reference to the unused/useless dirstream variables.
btrfs_open_fd2() is required to avoid spourious error messages.

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 cmds/inspect.c   | 5 ++---
 cmds/subvolume.c | 5 ++---
 common/utils.c   | 5 ++---
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/cmds/inspect.c b/cmds/inspect.c
index 4d4e24d2..268a902a 100644
--- a/cmds/inspect.c
+++ b/cmds/inspect.c
@@ -1020,7 +1020,6 @@ static int cmd_inspect_list_chunks(const struct cmd_struct *cmd,
 	int ret;
 	int fd;
 	int i;
-	DIR *dirstream = NULL;
 	unsigned unit_mode;
 	char *sortmode = NULL;
 	bool with_usage = true;
@@ -1083,7 +1082,7 @@ static int cmd_inspect_list_chunks(const struct cmd_struct *cmd,
 
 	path = argv[optind];
 
-	fd = open_file_or_dir(path, &dirstream);
+	fd = btrfs_open_fd2(path, false, true, false);
 	if (fd < 0) {
 	        error("cannot access '%s': %m", path);
 		return 1;
@@ -1187,7 +1186,7 @@ static int cmd_inspect_list_chunks(const struct cmd_struct *cmd,
 	}
 
 	ret = print_list_chunks(&ctx, sortmode, unit_mode, with_usage, with_empty);
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 
 out_nomem:
 	free(ctx.stats);
diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index cc1a660b..b6653a40 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -1492,7 +1492,6 @@ static int cmd_subvolume_show(const struct cmd_struct *cmd, int argc, char **arg
 	char *fullpath = NULL;
 	int fd = -1;
 	int ret = 1;
-	DIR *dirstream1 = NULL;
 	int by_rootid = 0;
 	int by_uuid = 0;
 	u64 rootid_arg = 0;
@@ -1550,7 +1549,7 @@ static int cmd_subvolume_show(const struct cmd_struct *cmd, int argc, char **arg
 		goto out;
 	}
 
-	fd = open_file_or_dir(fullpath, &dirstream1);
+	fd = btrfs_open_fd2(fullpath, false, true, false);
 	if (fd < 0) {
 		error("can't access '%s'", fullpath);
 		goto out;
@@ -1688,7 +1687,7 @@ out2:
 
 out:
 	free(subvol_path);
-	close_file_or_dir(fd, dirstream1);
+	close(fd);
 	free(fullpath);
 	return !!ret;
 }
diff --git a/common/utils.c b/common/utils.c
index 0c2fa8fe..3b86c9f3 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -211,7 +211,6 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 	struct btrfs_ioctl_dev_info_args *di_args;
 	struct btrfs_ioctl_dev_info_args tmp;
 	char mp[PATH_MAX];
-	DIR *dirstream = NULL;
 
 	memset(fi_args, 0, sizeof(*fi_args));
 
@@ -251,7 +250,7 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 	}
 
 	/* at this point path must not be for a block device */
-	fd = open_file_or_dir(path, &dirstream);
+	fd = btrfs_open_fd2(path, false, true, false);
 	if (fd < 0) {
 		ret = -errno;
 		goto out;
@@ -317,7 +316,7 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 	}
 
 out:
-	close_file_or_dir(fd, dirstream);
+	close(fd);
 	return ret;
 }
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 8/9] Killing dirstream: remove open_file_or_dir3 from du_add_file
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
                   ` (6 preceding siblings ...)
  2023-12-09 18:53 ` [PATCH 7/9] Killing dirstream: replace open_file_or_dir with btrfs_open_fd2 Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-09 18:53 ` [PATCH 9/9] Killing dirstream: remove unused functions Goffredo Baroncelli
  2023-12-14 16:17 ` [PATCH 0/9][btrfs-progs] Remove unused dirstream variable David Sterba
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

For historical reason the helpers [btrfs_]open_dir... return also
the 'DIR *dirstream' value when a dir is opened.

This patch replace the last reference to btrfs_open_file_or_dir3()
with btrfs_open_fd2().

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 cmds/filesystem-du.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/cmds/filesystem-du.c b/cmds/filesystem-du.c
index 4982123d..cffeafd5 100644
--- a/cmds/filesystem-du.c
+++ b/cmds/filesystem-du.c
@@ -456,7 +456,7 @@ static int du_add_file(const char *filename, int dirfd,
 		ret = sprintf(pathp, "/%s", filename);
 	pathp += ret;
 
-	fd = open_file_or_dir3(path, &dirstream, O_RDONLY);
+	fd = btrfs_open_fd2(path, false, false, false);
 	if (fd < 0) {
 		ret = -errno;
 		goto out;
@@ -489,6 +489,12 @@ static int du_add_file(const char *filename, int dirfd,
 	} else if (S_ISDIR(st.st_mode)) {
 		struct rb_root *root = shared_extents;
 
+		dirstream = fdopendir(fd);
+		if (!dirstream) {
+			ret = -errno;
+			goto out_close;
+		}
+
 		/*
 		 * We collect shared extents in an rb_root, the top
 		 * level caller will not pass a root down, so use the
@@ -542,7 +548,15 @@ static int du_add_file(const char *filename, int dirfd,
 		*ret_shared = file_shared;
 
 out_close:
-	close_file_or_dir(fd, dirstream);
+	/*
+	 * if dirstream is not NULL, it is derived by fd, so it is enough
+	 * to close the former
+	 */
+	if (dirstream)
+		closedir(dirstream);
+	else
+		close(fd);
+
 out:
 	/* reset path to just before this element */
 	pathp = pathtmp;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 9/9] Killing dirstream: remove unused functions
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
                   ` (7 preceding siblings ...)
  2023-12-09 18:53 ` [PATCH 8/9] Killing dirstream: remove open_file_or_dir3 from du_add_file Goffredo Baroncelli
@ 2023-12-09 18:53 ` Goffredo Baroncelli
  2023-12-14 16:17 ` [PATCH 0/9][btrfs-progs] Remove unused dirstream variable David Sterba
  9 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-09 18:53 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goffredo Baroncelli

From: Goffredo Baroncelli <kreijack@inwind.it>

Remove the following unused functions:
- btrfs_open_dir()
- open_file_or_dir()
- btrfs_open_file_or_dir()
- btrfs_open()
- open_path_or_dev_mnt()
- open_file_or_dir3()
- close_file_or_dir()

Signed-off-by: Goffredo Baroncelli <kreijack@libero.it>
---
 common/open-utils.c | 135 --------------------------------------------
 common/open-utils.h |  10 ----
 2 files changed, 145 deletions(-)

diff --git a/common/open-utils.c b/common/open-utils.c
index 61153294..2a201fd4 100644
--- a/common/open-utils.c
+++ b/common/open-utils.c
@@ -183,141 +183,6 @@ out:
 	return ret;
 }
 
-/*
- * Given a pathname, return a filehandle to:
- * 	the original pathname or,
- * 	if the pathname is a mounted btrfs device, to its mountpoint.
- *
- * On error, return -1, errno should be set.
- */
-int open_path_or_dev_mnt(const char *path, DIR **dirstream, int verbose)
-{
-	char mp[PATH_MAX];
-	int ret;
-
-	if (path_is_block_device(path)) {
-		ret = get_btrfs_mount(path, mp, sizeof(mp));
-		if (ret < 0) {
-			/* not a mounted btrfs dev */
-			error_on(verbose, "'%s' is not a mounted btrfs device",
-				 path);
-			errno = EINVAL;
-			return -1;
-		}
-		ret = open_file_or_dir(mp, dirstream);
-		error_on(verbose && ret < 0, "can't access '%s': %m",
-			 path);
-	} else {
-		ret = btrfs_open_dir(path, dirstream, 1);
-	}
-
-	return ret;
-}
-
-/*
- * Do the following checks before calling open_file_or_dir():
- * 1: path is in a btrfs filesystem
- * 2: path is a directory if dir_only is 1
- */
-int btrfs_open(const char *path, DIR **dirstream, int verbose, int dir_only)
-{
-	struct statfs stfs;
-	struct stat st;
-	int ret;
-
-	if (stat(path, &st) != 0) {
-		error_on(verbose, "cannot access '%s': %m", path);
-		return -1;
-	}
-
-	if (dir_only && !S_ISDIR(st.st_mode)) {
-		error_on(verbose, "not a directory: %s", path);
-		return -3;
-	}
-
-	if (statfs(path, &stfs) != 0) {
-		error_on(verbose, "cannot access '%s': %m", path);
-		return -1;
-	}
-
-	if (stfs.f_type != BTRFS_SUPER_MAGIC) {
-		error_on(verbose, "not a btrfs filesystem: %s", path);
-		return -2;
-	}
-
-	ret = open_file_or_dir(path, dirstream);
-	if (ret < 0) {
-		error_on(verbose, "cannot access '%s': %m", path);
-	}
-
-	return ret;
-}
-
-int btrfs_open_dir(const char *path, DIR **dirstream, int verbose)
-{
-	return btrfs_open(path, dirstream, verbose, 1);
-}
-
-int btrfs_open_file_or_dir(const char *path, DIR **dirstream, int verbose)
-{
-	return btrfs_open(path, dirstream, verbose, 0);
-}
-
-int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags)
-{
-	int ret;
-	struct stat st;
-	int fd;
-
-	ret = stat(fname, &st);
-	if (ret < 0) {
-		return -1;
-	}
-	if (S_ISDIR(st.st_mode)) {
-		*dirstream = opendir(fname);
-		if (!*dirstream)
-			return -1;
-		fd = dirfd(*dirstream);
-	} else if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
-		fd = open(fname, open_flags);
-	} else {
-		/*
-		 * we set this on purpose, in case the caller output
-		 * strerror(errno) as success
-		 */
-		errno = EINVAL;
-		return -1;
-	}
-	if (fd < 0) {
-		fd = -1;
-		if (*dirstream) {
-			closedir(*dirstream);
-			*dirstream = NULL;
-		}
-	}
-	return fd;
-}
-
-int open_file_or_dir(const char *fname, DIR **dirstream)
-{
-	return open_file_or_dir3(fname, dirstream, O_RDWR);
-}
-
-void close_file_or_dir(int fd, DIR *dirstream)
-{
-	int old_errno;
-
-	old_errno = errno;
-	if (dirstream) {
-		closedir(dirstream);
-	} else if (fd >= 0) {
-		close(fd);
-	}
-
-	errno = old_errno;
-}
-
-
 /*
  * Do the following checks before calling open:
  * 1: path is in a btrfs filesystem
diff --git a/common/open-utils.h b/common/open-utils.h
index 96d99f5d..5642b951 100644
--- a/common/open-utils.h
+++ b/common/open-utils.h
@@ -28,16 +28,6 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 			bool noscan);
 int check_mounted(const char* file);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
-int open_path_or_dev_mnt(const char *path, DIR **dirstream, int verbose);
-
-int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags);
-int open_file_or_dir(const char *fname, DIR **dirstream);
-
-int btrfs_open(const char *path, DIR **dirstream, int verbose, int dir_only);
-int btrfs_open_dir(const char *path, DIR **dirstream, int verbose);
-int btrfs_open_file_or_dir(const char *path, DIR **dirstream, int verbose);
-
-void close_file_or_dir(int fd, DIR *dirstream);
 
 int btrfs_open_fd2(const char *path, bool verbose, bool read_write, bool dir_only);
 int btrfs_open_file_or_dir_fd(const char *path);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
  2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
                   ` (8 preceding siblings ...)
  2023-12-09 18:53 ` [PATCH 9/9] Killing dirstream: remove unused functions Goffredo Baroncelli
@ 2023-12-14 16:17 ` David Sterba
  2023-12-14 19:11   ` Goffredo Baroncelli
  2023-12-30 11:20   ` Goffredo Baroncelli
  9 siblings, 2 replies; 18+ messages in thread
From: David Sterba @ 2023-12-14 16:17 UTC (permalink / raw)
  To: Goffredo Baroncelli; +Cc: linux-btrfs, Goffredo Baroncelli

On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@inwind.it>
> 
> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
> directory returning the 'fd' and a 'dirstream' variable returned by
> opendir(3).
> 
> If the path is a file, the 'fd' is computed from open(2) and
> dirstream is set to NULL.
> If the path is a directory, first the directory is opened by opendir(3), then
> the 'fd' is computed using dirfd(3).
> However the 'dirstream' returned by opendir(3) is left open until 'fd'
> is not needed anymore.
> 
> In near every case the 'dirstream' variable is not used. Only 'fd' is
> used.

As I'm reading dirfd manual page, dirfd returns the internal file
descriptor of the dirstream and it gets closed after call to closedir().
This means if we pass a directory and want a file descriptor then its
lifetime matches the correspoinding DIR.

> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
> 
> Aim of this patch set is to getrid of this complexity; when the path of
> a directory is passed, the fd is get directly using open(path, O_RDONLY):
> so we don't need to use readdir(3) and to maintain the not used variable
> 'dirstream'.

Does this work the same way as with the dirstream?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
  2023-12-14 16:17 ` [PATCH 0/9][btrfs-progs] Remove unused dirstream variable David Sterba
@ 2023-12-14 19:11   ` Goffredo Baroncelli
  2023-12-30 11:20   ` Goffredo Baroncelli
  1 sibling, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-14 19:11 UTC (permalink / raw)
  To: dsterba, Goffredo Baroncelli; +Cc: linux-btrfs

On 14/12/2023 17.17, David Sterba wrote:
> On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>
[...]
> 
> As I'm reading dirfd manual page, dirfd returns the internal file
> descriptor of the dirstream and it gets closed after call to closedir().
> This means if we pass a directory and want a file descriptor then its
> lifetime matches the correspoinding DIR.

Yes, more below
> 
>> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
>>
>> Aim of this patch set is to getrid of this complexity; when the path of
>> a directory is passed, the fd is get directly using open(path, O_RDONLY):
>> so we don't need to use readdir(3) and to maintain the not used variable
>> 'dirstream'.
> 
> Does this work the same way as with the dirstream?

Yes; I tested my patches and these worked properly.

My understanding is that opendir(3)/readdir(3) are based to the pair
open(2)/getdents64(2). opendir(3) does a "open(path, O_RDONLY)",
and store the file descriptors in a malloc-ed DIR structure. Then closedir(3)
closes the fd and free the DIR structure..

You may see further details about the glib implementation.

https://github.com/kraj/glibc/blob/8757659b3cee57c1defee7772c3ee687d310b076/sysdeps/unix/sysv/linux/opendir.c#L81

The BIONIC implementation does the same thing.

The key point is that you may open(2) a directory only in O_RDONLY mode. In fact you cannot write to a directory. You may only read(); to "write" you have to use a different API like rmdir/mkdir/mknod or open("<dir>/<filename>", O_RDWR").

Another test that I did was:

ghigo@venice:/tmp$ cat test.c
#include <sys/types.h>
#include <dirent.h>

int main(int argc, char **argv) {
         DIR *dir = opendir(argv[1]);
         return (int)dir;
}

ghigo@venice:/tmp$ gcc -Wno-pointer-to-int-cast -o test test.c
ghigo@venice:/tmp$ mkdir test2
ghigo@venice:/tmp$ strace ./test test2
[...]
prlimit64(0, RLIMIT_STACK, NULL, {rlim_cur=8192*1024, rlim_max=RLIM64_INFINITY}) = 0
munmap(0x7f328b589000, 222607)          = 0
openat(AT_FDCWD, "test2", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3        <----------
newfstatat(3, "", {st_mode=S_IFDIR|0755, st_size=0, ...}, AT_EMPTY_PATH) = 0
getrandom("\x04\x76\xb1\xe0\xee\xd8\x8a\xc8", 8, GRND_NONBLOCK) = 8
brk(NULL)                               = 0x55676fffd000
brk(0x55677001e000)                     = 0x55677001e000
exit_group(1879036576)                  = ?
+++ exited with 160 +++
ghigo@venice:/tmp$


BR
G.Baroncelli

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
  2023-12-14 16:17 ` [PATCH 0/9][btrfs-progs] Remove unused dirstream variable David Sterba
  2023-12-14 19:11   ` Goffredo Baroncelli
@ 2023-12-30 11:20   ` Goffredo Baroncelli
  2024-01-02 18:17     ` David Sterba
  2024-02-07 10:16     ` David Sterba
  1 sibling, 2 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2023-12-30 11:20 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs

On 14/12/2023 17.17, David Sterba wrote:
> On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>
>> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
>> directory returning the 'fd' and a 'dirstream' variable returned by
>> opendir(3).
>>
>> If the path is a file, the 'fd' is computed from open(2) and
>> dirstream is set to NULL.
>> If the path is a directory, first the directory is opened by opendir(3), then
>> the 'fd' is computed using dirfd(3).
>> However the 'dirstream' returned by opendir(3) is left open until 'fd'
>> is not needed anymore.
>>
>> In near every case the 'dirstream' variable is not used. Only 'fd' is
>> used.
> 
> As I'm reading dirfd manual page, dirfd returns the internal file
> descriptor of the dirstream and it gets closed after call to closedir().
> This means if we pass a directory and want a file descriptor then its
> lifetime matches the correspoinding DIR.
> 
>> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
>>
>> Aim of this patch set is to getrid of this complexity; when the path of
>> a directory is passed, the fd is get directly using open(path, O_RDONLY):
>> so we don't need to use readdir(3) and to maintain the not used variable
>> 'dirstream'.
> 
> Does this work the same way as with the dirstream?
> 

Hi David, are you interested in this patch ? I think that it is a
great simplification.

BR

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
  2023-12-30 11:20   ` Goffredo Baroncelli
@ 2024-01-02 18:17     ` David Sterba
  2024-02-07 10:16     ` David Sterba
  1 sibling, 0 replies; 18+ messages in thread
From: David Sterba @ 2024-01-02 18:17 UTC (permalink / raw)
  To: kreijack; +Cc: dsterba, linux-btrfs

On Sat, Dec 30, 2023 at 12:20:54PM +0100, Goffredo Baroncelli wrote:
> On 14/12/2023 17.17, David Sterba wrote:
> > On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli <kreijack@inwind.it>
> >>
> >> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
> >> directory returning the 'fd' and a 'dirstream' variable returned by
> >> opendir(3).
> >>
> >> If the path is a file, the 'fd' is computed from open(2) and
> >> dirstream is set to NULL.
> >> If the path is a directory, first the directory is opened by opendir(3), then
> >> the 'fd' is computed using dirfd(3).
> >> However the 'dirstream' returned by opendir(3) is left open until 'fd'
> >> is not needed anymore.
> >>
> >> In near every case the 'dirstream' variable is not used. Only 'fd' is
> >> used.
> > 
> > As I'm reading dirfd manual page, dirfd returns the internal file
> > descriptor of the dirstream and it gets closed after call to closedir().
> > This means if we pass a directory and want a file descriptor then its
> > lifetime matches the correspoinding DIR.
> > 
> >> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
> >>
> >> Aim of this patch set is to getrid of this complexity; when the path of
> >> a directory is passed, the fd is get directly using open(path, O_RDONLY):
> >> so we don't need to use readdir(3) and to maintain the not used variable
> >> 'dirstream'.
> > 
> > Does this work the same way as with the dirstream?
> > 
> 
> Hi David, are you interested in this patch ? I think that it is a
> great simplification.

Yes, it's a good cleanup, I'll get to that after 6.7 is released.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
  2023-12-30 11:20   ` Goffredo Baroncelli
  2024-01-02 18:17     ` David Sterba
@ 2024-02-07 10:16     ` David Sterba
  2024-02-08 17:28       ` Goffredo Baroncelli
  1 sibling, 1 reply; 18+ messages in thread
From: David Sterba @ 2024-02-07 10:16 UTC (permalink / raw)
  To: kreijack; +Cc: dsterba, linux-btrfs

On Sat, Dec 30, 2023 at 12:20:54PM +0100, Goffredo Baroncelli wrote:
> On 14/12/2023 17.17, David Sterba wrote:
> > On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli <kreijack@inwind.it>
> >>
> >> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
> >> directory returning the 'fd' and a 'dirstream' variable returned by
> >> opendir(3).
> >>
> >> If the path is a file, the 'fd' is computed from open(2) and
> >> dirstream is set to NULL.
> >> If the path is a directory, first the directory is opened by opendir(3), then
> >> the 'fd' is computed using dirfd(3).
> >> However the 'dirstream' returned by opendir(3) is left open until 'fd'
> >> is not needed anymore.
> >>
> >> In near every case the 'dirstream' variable is not used. Only 'fd' is
> >> used.
> > 
> > As I'm reading dirfd manual page, dirfd returns the internal file
> > descriptor of the dirstream and it gets closed after call to closedir().
> > This means if we pass a directory and want a file descriptor then its
> > lifetime matches the correspoinding DIR.
> > 
> >> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
> >>
> >> Aim of this patch set is to getrid of this complexity; when the path of
> >> a directory is passed, the fd is get directly using open(path, O_RDONLY):
> >> so we don't need to use readdir(3) and to maintain the not used variable
> >> 'dirstream'.
> > 
> > Does this work the same way as with the dirstream?
> > 
> 
> Hi David, are you interested in this patch ? I think that it is a
> great simplification.

Sorry, I got distracted by other things.

The patchset does not apply cleanly on 6.7 so I've used 6.6.3 as the
base for testing. There's one failure in the cli tests, result can be
seen here https://github.com/kdave/btrfs-progs/actions/runs/7813042695/job/21311365019

====== RUN MUSTFAIL /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
ERROR: not a btrfs filesystem: /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
failed (expected): /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
no expected error message in the output 2
test failed for case 003-fi-resize-args

I think it's related to the changes to dirstream, however I can't
reproduce it locally, only on the github actions CI.

Unlike other commands in the test, this one is called on an image and it
must fail to prevent accidentaly resizing underlying filesystem.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
  2024-02-07 10:16     ` David Sterba
@ 2024-02-08 17:28       ` Goffredo Baroncelli
  2024-02-08 17:34         ` Goffredo Baroncelli
  0 siblings, 1 reply; 18+ messages in thread
From: Goffredo Baroncelli @ 2024-02-08 17:28 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs

On 07/02/2024 11.16, David Sterba wrote:
> On Sat, Dec 30, 2023 at 12:20:54PM +0100, Goffredo Baroncelli wrote:
>> On 14/12/2023 17.17, David Sterba wrote:
>>> On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
>>>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>>>
>>>> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
>>>> directory returning the 'fd' and a 'dirstream' variable returned by
>>>> opendir(3).
>>>>
>>>> If the path is a file, the 'fd' is computed from open(2) and
>>>> dirstream is set to NULL.
>>>> If the path is a directory, first the directory is opened by opendir(3), then
>>>> the 'fd' is computed using dirfd(3).
>>>> However the 'dirstream' returned by opendir(3) is left open until 'fd'
>>>> is not needed anymore.
>>>>
>>>> In near every case the 'dirstream' variable is not used. Only 'fd' is
>>>> used.
>>>
>>> As I'm reading dirfd manual page, dirfd returns the internal file
>>> descriptor of the dirstream and it gets closed after call to closedir().
>>> This means if we pass a directory and want a file descriptor then its
>>> lifetime matches the correspoinding DIR.
>>>
>>>> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
>>>>
>>>> Aim of this patch set is to getrid of this complexity; when the path of
>>>> a directory is passed, the fd is get directly using open(path, O_RDONLY):
>>>> so we don't need to use readdir(3) and to maintain the not used variable
>>>> 'dirstream'.
>>>
>>> Does this work the same way as with the dirstream?
>>>
>>
>> Hi David, are you interested in this patch ? I think that it is a
>> great simplification.
> 
> Sorry, I got distracted by other things.
> 
> The patchset does not apply cleanly on 6.7 so I've used 6.6.3 as the
> base for testing. There's one failure in the cli tests, result can be
> seen here https://github.com/kdave/btrfs-progs/actions/runs/7813042695/job/21311365019
> 
> ====== RUN MUSTFAIL /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
> ERROR: not a btrfs filesystem: /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
> failed (expected): /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
> no expected error message in the output 2
> test failed for case 003-fi-resize-args
> 
> I think it's related to the changes to dirstream, however I can't
> reproduce it locally, only on the github actions CI.
> 
> Unlike other commands in the test, this one is called on an image and it
> must fail to prevent accidentaly resizing underlying filesystem.


I will rebase on 6.7 and I will try to reproduce the problem. Could you share the full script, or point me where is it ?

BR
-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
  2024-02-08 17:28       ` Goffredo Baroncelli
@ 2024-02-08 17:34         ` Goffredo Baroncelli
  2024-02-08 20:18           ` Goffredo Baroncelli
  0 siblings, 1 reply; 18+ messages in thread
From: Goffredo Baroncelli @ 2024-02-08 17:34 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs

On 08/02/2024 18.28, Goffredo Baroncelli wrote:
> On 07/02/2024 11.16, David Sterba wrote:
>> On Sat, Dec 30, 2023 at 12:20:54PM +0100, Goffredo Baroncelli wrote:
>>> On 14/12/2023 17.17, David Sterba wrote:
>>>> On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
>>>>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>>>>
>>>>> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
>>>>> directory returning the 'fd' and a 'dirstream' variable returned by
>>>>> opendir(3).
>>>>>
>>>>> If the path is a file, the 'fd' is computed from open(2) and
>>>>> dirstream is set to NULL.
>>>>> If the path is a directory, first the directory is opened by opendir(3), then
>>>>> the 'fd' is computed using dirfd(3).
>>>>> However the 'dirstream' returned by opendir(3) is left open until 'fd'
>>>>> is not needed anymore.
>>>>>
>>>>> In near every case the 'dirstream' variable is not used. Only 'fd' is
>>>>> used.
>>>>
>>>> As I'm reading dirfd manual page, dirfd returns the internal file
>>>> descriptor of the dirstream and it gets closed after call to closedir().
>>>> This means if we pass a directory and want a file descriptor then its
>>>> lifetime matches the correspoinding DIR.
>>>>
>>>>> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
>>>>>
>>>>> Aim of this patch set is to getrid of this complexity; when the path of
>>>>> a directory is passed, the fd is get directly using open(path, O_RDONLY):
>>>>> so we don't need to use readdir(3) and to maintain the not used variable
>>>>> 'dirstream'.
>>>>
>>>> Does this work the same way as with the dirstream?
>>>>
>>>
>>> Hi David, are you interested in this patch ? I think that it is a
>>> great simplification.
>>
>> Sorry, I got distracted by other things.
>>
>> The patchset does not apply cleanly on 6.7 so I've used 6.6.3 as the
>> base for testing. There's one failure in the cli tests, result can be
>> seen here https://github.com/kdave/btrfs-progs/actions/runs/7813042695/job/21311365019
>>
>> ====== RUN MUSTFAIL /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>> ERROR: not a btrfs filesystem: /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>> failed (expected): /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>> no expected error message in the output 2
>> test failed for case 003-fi-resize-args
>>
>> I think it's related to the changes to dirstream, however I can't
>> reproduce it locally, only on the github actions CI.
>>
>> Unlike other commands in the test, this one is called on an image and it
>> must fail to prevent accidentaly resizing underlying filesystem.
> 
> 
> I will rebase on 6.7 and I will try to reproduce the problem. Could you share the full script, or point me where is it ?

Ignore my latter request. I found the test code.

> 
> BR

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable
  2024-02-08 17:34         ` Goffredo Baroncelli
@ 2024-02-08 20:18           ` Goffredo Baroncelli
  0 siblings, 0 replies; 18+ messages in thread
From: Goffredo Baroncelli @ 2024-02-08 20:18 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs

On 08/02/2024 18.34, Goffredo Baroncelli wrote:
> On 08/02/2024 18.28, Goffredo Baroncelli wrote:
>> On 07/02/2024 11.16, David Sterba wrote:
>>> On Sat, Dec 30, 2023 at 12:20:54PM +0100, Goffredo Baroncelli wrote:
>>>> On 14/12/2023 17.17, David Sterba wrote:
>>>>> On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
>>>>>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>>>>>
>>>>>> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
>>>>>> directory returning the 'fd' and a 'dirstream' variable returned by
>>>>>> opendir(3).
>>>>>>
>>>>>> If the path is a file, the 'fd' is computed from open(2) and
>>>>>> dirstream is set to NULL.
>>>>>> If the path is a directory, first the directory is opened by opendir(3), then
>>>>>> the 'fd' is computed using dirfd(3).
>>>>>> However the 'dirstream' returned by opendir(3) is left open until 'fd'
>>>>>> is not needed anymore.
>>>>>>
>>>>>> In near every case the 'dirstream' variable is not used. Only 'fd' is
>>>>>> used.
>>>>>
>>>>> As I'm reading dirfd manual page, dirfd returns the internal file
>>>>> descriptor of the dirstream and it gets closed after call to closedir().
>>>>> This means if we pass a directory and want a file descriptor then its
>>>>> lifetime matches the correspoinding DIR.
>>>>>
>>>>>> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
>>>>>>
>>>>>> Aim of this patch set is to getrid of this complexity; when the path of
>>>>>> a directory is passed, the fd is get directly using open(path, O_RDONLY):
>>>>>> so we don't need to use readdir(3) and to maintain the not used variable
>>>>>> 'dirstream'.
>>>>>
>>>>> Does this work the same way as with the dirstream?
>>>>>
>>>>
>>>> Hi David, are you interested in this patch ? I think that it is a
>>>> great simplification.
>>>
>>> Sorry, I got distracted by other things.
>>>
>>> The patchset does not apply cleanly on 6.7 so I've used 6.6.3 as the
>>> base for testing. There's one failure in the cli tests, result can be
>>> seen here https://github.com/kdave/btrfs-progs/actions/runs/7813042695/job/21311365019
>>>
>>> ====== RUN MUSTFAIL /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>>> ERROR: not a btrfs filesystem: /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>>> failed (expected): /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>>> no expected error message in the output 2
>>> test failed for case 003-fi-resize-args
>>>
>>> I think it's related to the changes to dirstream, however I can't
>>> reproduce it locally, only on the github actions CI.

I found the problem, and I have an idea why you cannot reproduce the problem

The test

	btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img

Expect the following error message:

	ERROR: resize works on mounted filesystems and accepts only

However if you pass a file image on a NON btrfs fs, after my patch you got the following error:

	ERROR: not a btrfs filesystem: /home/ghigo/btrfs/btrfs-progs/tests/test.img

But if you do the test on a BTRFS filesystem, you get the following error

	ERROR: not a directory: /mnt/btrfs-raid1/test.img
	ERROR: resize works on mounted filesystems and accepts only
	directories as argument. Passing file containing a btrfs image
	would resize the underlying filesystem instead of the image.


Basically, my patch rearranged the tests as:
- is a btrfs filesystem
- is a directory

where before the tests were
- is a directory
- is a btrfs filesystem


In the test 013-xxxx before failed the test "is a directory", now the test "is a btrfs filesystem" fails first.


The code fails here:

cmds/filesystem.c:
static int cmd_filesystem_resize(const struct cmd_struct *cmd,
[...]
         fd = btrfs_open_dir_fd(path);
         if (fd < 0) {
                 /* The path is a directory */
                 if (fd == -3) {
                         error(
                 "resize works on mounted filesystems and accepts only\n"
                 "directories as argument. Passing file containing a btrfs image\n"
                 "would resize the underlying filesystem instead of the image.\n");
                 }
                 return 1;
         }


However the check implemented in btrfs_open_dir_fd() are:

btrfs_open_dir_fd()
   btrfs_open_fd_2()

[...]
         if (stat(path, &st) != 0) {
                 error_on(verbose, "cannot access '%s': %m", path);
                 return -1;
         }

         if (statfs(path, &stfs) != 0) {
                 error_on(verbose, "cannot access '%s': %m", path);
                 return -1;
         }

         if (stfs.f_type != BTRFS_SUPER_MAGIC) {
                 error_on(verbose, "not a btrfs filesystem: %s", path);    // <---- NOW fail first here IF the filesystem is a not
                 return -2;						  //       btrfs
         }

         if (dir_only && !S_ISDIR(st.st_mode)) {
                 error_on(verbose, "not a directory: %s", path);          // <----- BEFORE failed here
                 return -3;
         }

         if (S_ISDIR(st.st_mode) || !read_write)
                 ret = open(path, O_RDONLY);
         else
                 ret = open(path, O_RDWR);
         if (ret < 0) {
                 error_on(verbose, "cannot access '%s': %m", path);
         }




IN order to solve the issue, I swapped the check orders; to be consistent with the previous errors.
Soon I will send you a V2 version

>>>
>>> Unlike other commands in the test, this one is called on an image and it
>>> must fail to prevent accidentaly resizing underlying filesystem.
>>
>>
>> I will rebase on 6.7 and I will try to reproduce the problem. Could you share the full script, or point me where is it ?
> 
> Ignore my latter request. I found the test code.
> 
>>
>> BR
> 

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2024-02-08 20:18 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-09 18:53 [PATCH 0/9][btrfs-progs] Remove unused dirstream variable Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 1/9] Killing dirstream: add helpers Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 2/9] Killing dirstream: replace btrfs_open_dir with btrfs_open_dir_fd Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 3/9] " Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 4/9] Killing dirstream: replace open_path_or_dev_mnt with btrfs_open_mnt_fd Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 5/9] Killing dirstream: replace open_file_or_dir3 with btrfs_open_fd2 Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 6/9] Killing dirstream: replace btrfs_open_file_or_dir with btrfs_open_file_or_dir_fd Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 7/9] Killing dirstream: replace open_file_or_dir with btrfs_open_fd2 Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 8/9] Killing dirstream: remove open_file_or_dir3 from du_add_file Goffredo Baroncelli
2023-12-09 18:53 ` [PATCH 9/9] Killing dirstream: remove unused functions Goffredo Baroncelli
2023-12-14 16:17 ` [PATCH 0/9][btrfs-progs] Remove unused dirstream variable David Sterba
2023-12-14 19:11   ` Goffredo Baroncelli
2023-12-30 11:20   ` Goffredo Baroncelli
2024-01-02 18:17     ` David Sterba
2024-02-07 10:16     ` David Sterba
2024-02-08 17:28       ` Goffredo Baroncelli
2024-02-08 17:34         ` Goffredo Baroncelli
2024-02-08 20:18           ` Goffredo Baroncelli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox