[PATCH 0/8] btrfs-progs: add subvol list options for sane path behavior

[PATCH 0/8] btrfs-progs: add subvol list options for sane path behavior

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hello,

btrfs subvol list's path handling has been a constant source of
confusion for users. None of -o, -a, or the default do what users
expect. This has been broken for a decade; see [1].

This series adds two new options, -O and -A, which do what users
actually want: list subvolumes below a path, or list all subvolumes,
with minimal path shenanigans. This approach is conservative and tries
to maintain backwards compatibility, but it's worth discussing whether
we should instead fix the existing options/default, or more noisily
deprecate the existing options.

One additional benefit of this is that -O can be used by unprivileged
users.

Patch 1 fixes a libbtrfsutil bug I encountered while testing this.
Patches 2 and 3 fix libbtrfsutil's privilege checks to work in user
namespaces. Patches 4 and 5 remove some unused subvol list code. Patch 6
documents and tests the current, insane behavior. Patch 7 converts
subvol list to use libbtrfsutil. This is a revival of one of my old
patches [2], but is much easier now that libbtrfs has been pared down.
Patch 8 adds the new options, including documentation and tests.

Thanks!
Omar

1: https://lore.kernel.org/all/bdd9af61-b408-c8d2-6697-84230b0bcf89@gmail.com/
2: https://lore.kernel.org/all/6492726d6e89bf792627e4431f7ba7691f09c3d2.1518720598.git.osandov@fb.com/

Omar Sandoval (8):
  libbtrfsutil: fix accidentally closing fd passed to subvolume iterator
  libbtrfsutil: don't check for UID 0 in subvolume_info()
  libbtrfsutil: don't check for UID 0 in subvolume iterator
  btrfs-progs: subvol list: remove unused raw layout code
  btrfs-progs: subvol list: remove unused filters
  btrfs-progs: subvol list: document and test actual behavior of paths
  btrfs-progs: subvol list: use libbtrfsutil
  btrfs-progs: subvol list: add sane -O and -A options

 Documentation/btrfs-subvolume.rst             |   37 +-
 cmds/subvolume-list.c                         | 1081 +++++------------
 libbtrfsutil/python/tests/test_subvolume.py   |   18 +
 libbtrfsutil/subvolume.c                      |   50 +-
 .../026-subvolume-list-path-filtering/test.sh |  156 +++
 5 files changed, 565 insertions(+), 777 deletions(-)
 create mode 100755 tests/cli-tests/026-subvolume-list-path-filtering/test.sh

-- 
2.45.2

[PATCH 1/8] libbtrfsutil: fix accidentally closing fd passed to subvolume iterator

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

For an unprivileged subvolume iterator, append_to_search_stack() closes
cur_fd. On the first call to btrfs_util_subvolume_iterator_next(),
cur_fd is equal to the fd that was passed to
btrfs_util_create_subvolume_iterator_fd(). We're not supposed to close
that. We didn't notice it because it's more common to use it through
btrfs_util_create_subvolume_iterator(), which opens its own fd that
should be closed, and because the fd number is often reused internally
by the subvolume iterator.

pop_search_stack() already has a check to avoid closing the passed fd;
add the same check to append_to_search_stack(). Also add a regression
test.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libbtrfsutil/python/tests/test_subvolume.py | 18 ++++++++++++++++++
 libbtrfsutil/subvolume.c                    |  3 ++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/libbtrfsutil/python/tests/test_subvolume.py b/libbtrfsutil/python/tests/test_subvolume.py
index 690ff107..1e7df00e 100644
--- a/libbtrfsutil/python/tests/test_subvolume.py
+++ b/libbtrfsutil/python/tests/test_subvolume.py
@@ -561,3 +561,21 @@ class TestSubvolume(BtrfsTestCase):
                 self._test_subvolume_iterator_race()
         finally:
             os.chdir(pwd)
+
+    def test_subvolume_iterator_fd_unprivileged(self):
+        pwd = os.getcwd()
+        try:
+            os.chdir(self.mountpoint)
+            btrfsutil.create_subvolume('subvol')
+            with drop_privs():
+                fd = os.open('.', os.O_RDONLY | os.O_DIRECTORY)
+                try:
+                    with btrfsutil.SubvolumeIterator(fd) as it:
+                        for _ in it:
+                            break
+                finally:
+                    # A bug in SubvolumeIterator previously made it close the
+                    # passed in fd, so this would fail with EBADF.
+                    os.close(fd)
+        finally:
+            os.chdir(pwd)
diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
index 6b2f2671..1f0f2d2b 100644
--- a/libbtrfsutil/subvolume.c
+++ b/libbtrfsutil/subvolume.c
@@ -930,7 +930,8 @@ static enum btrfs_util_error append_to_search_stack(struct btrfs_util_subvolume_
 				return err;
 			}
 
-			close(iter->cur_fd);
+			if (iter->cur_fd != iter->fd)
+				close(iter->cur_fd);
 			iter->cur_fd = fd;
 		}
 	}
-- 
2.45.2

[PATCH 2/8] libbtrfsutil: don't check for UID 0 in subvolume_info()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

btrfs_util_subvolume_info() explicitly checks whether geteuid() == 0 to
decide whether to use the unprivileged BTRFS_IOC_GET_SUBVOL_INFO ioctl
or the privileged BTRFS_IOC_TREE_SEARCH ioctl. This breaks in user
namespaces:

  $ unshare -r python3 -c 'import btrfsutil; print(btrfsutil.subvolume_info("/"))'
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
  btrfsutil.BtrfsUtilError: [BtrfsUtilError 12 Errno 1] Could not search B-tree: Operation not permitted: '/'

The unprivileged ioctl has been supported since Linux 4.18. Let's try
the unprivileged ioctl first, then fall back to the privileged version
only if it isn't supported.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libbtrfsutil/subvolume.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
index 1f0f2d2b..70f2ec70 100644
--- a/libbtrfsutil/subvolume.c
+++ b/libbtrfsutil/subvolume.c
@@ -451,8 +451,10 @@ PUBLIC enum btrfs_util_error btrfs_util_subvolume_info_fd(int fd, uint64_t id,
 		if (err)
 			return err;
 
-		if (!is_root())
-			return get_subvolume_info_unprivileged(fd, subvol);
+		err = get_subvolume_info_unprivileged(fd, subvol);
+		if (err != BTRFS_UTIL_ERROR_GET_SUBVOL_INFO_FAILED ||
+		    errno != ENOTTY)
+			return err;
 
 		err = btrfs_util_subvolume_id_fd(fd, &id);
 		if (err)
-- 
2.45.2

[PATCH 3/8] libbtrfsutil: don't check for UID 0 in subvolume iterator

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The subvolume iterator API explicitly checks whether geteuid() == 0 to
decide whether to use the unprivileged BTRFS_IOC_GET_SUBVOL_ROOTREF and
BTRFS_IOC_INO_LOOKUP_USER ioctls or the privileged BTRFS_IOC_TREE_SEARCH
ioctl. This breaks in user namespaces:

  $ unshare -r python3 -c 'import btrfsutil; print(list(btrfsutil.SubvolumeIterator("/home")))'
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
  btrfsutil.BtrfsUtilError: [BtrfsUtilError 12 Errno 1] Could not search B-tree: Operation not permitted

Instead of the explicit check, let's try the privileged mode first, and
if it fails with a permission error, fall back to the unprivileged mode
(which has been supported since Linux 4.18). Note that we have to try
the privileged mode first, since even for privileged users, the
unprivileged mode may omit some subvolumes that are hidden by filesystem
mounts.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libbtrfsutil/subvolume.c | 41 +++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
index 70f2ec70..eba1c9a2 100644
--- a/libbtrfsutil/subvolume.c
+++ b/libbtrfsutil/subvolume.c
@@ -32,11 +32,6 @@
 
 #include "btrfsutil_internal.h"
 
-static bool is_root(void)
-{
-	return geteuid() == 0;
-}
-
 /*
  * This intentionally duplicates btrfs_util_is_subvolume_fd() instead of opening
  * a file descriptor and calling it, because fstat() and fstatfs() don't accept
@@ -807,7 +802,11 @@ struct search_stack_entry {
 };
 
 struct btrfs_util_subvolume_iterator {
-	bool use_tree_search;
+	/*
+	 * 1 if using tree search, 0 if using unprivileged ioctls, -1 if not
+	 * determined yet.
+	 */
+	int use_tree_search;
 	int fd;
 	/* cur_fd is only used for subvolume_iterator_next_unprivileged(). */
 	int cur_fd;
@@ -1009,14 +1008,14 @@ PUBLIC enum btrfs_util_error btrfs_util_create_subvolume_iterator_fd(int fd,
 {
 	struct btrfs_util_subvolume_iterator *iter;
 	enum btrfs_util_error err;
-	bool use_tree_search;
+	int use_tree_search;
 
 	if (flags & ~BTRFS_UTIL_SUBVOLUME_ITERATOR_MASK) {
 		errno = EINVAL;
 		return BTRFS_UTIL_ERROR_INVALID_ARGUMENT;
 	}
 
-	use_tree_search = top != 0 || is_root();
+	use_tree_search = top == 0 ? -1 : 1;
 	if (top == 0) {
 		err = btrfs_util_is_subvolume_fd(fd);
 		if (err)
@@ -1666,13 +1665,29 @@ PUBLIC enum btrfs_util_error btrfs_util_subvolume_iterator_next(struct btrfs_uti
 								char **path_ret,
 								uint64_t *id_ret)
 {
+	/*
+	 * On the first iteration, iter->use_tree_search < 0. In that case, we
+	 * try a tree search, and if it fails with a permission error, we fall
+	 * back to the unprivileged ioctls.
+	 */
 	if (iter->use_tree_search) {
-		return subvolume_iterator_next_tree_search(iter, path_ret,
-							   id_ret);
-	} else {
-		return subvolume_iterator_next_unprivileged(iter, path_ret,
-							    id_ret);
+		enum btrfs_util_error err;
+		struct search_stack_entry *entry;
+
+		err = subvolume_iterator_next_tree_search(iter, path_ret,
+							  id_ret);
+		if (iter->use_tree_search > 0)
+			return err;
+
+		if (err != BTRFS_UTIL_ERROR_SEARCH_FAILED || errno != EPERM) {
+			iter->use_tree_search = 1;
+			return err;
+		}
+		entry = iter->search_stack;
+		entry->id = entry->search.key.min_objectid;
+		iter->use_tree_search = 0;
 	}
+	return subvolume_iterator_next_unprivileged(iter, path_ret, id_ret);
 }
 PUBLIC enum btrfs_util_error btrfs_util_subvolume_iter_next(struct btrfs_util_subvolume_iterator *iter,
 							     char **path_ret,
-- 
2.45.2

[PATCH 4/8] btrfs-progs: subvol list: remove unused raw layout code

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

It hasn't been used since commit 9005b603d723 ("btrfs-progs: use
libbtrfsutil for subvol show").

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 cmds/subvolume-list.c | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
index 52a30aa0..060f4f31 100644
--- a/cmds/subvolume-list.c
+++ b/cmds/subvolume-list.c
@@ -98,7 +98,6 @@ static const char * const cmd_subvolume_list_usage[] = {
 enum btrfs_list_layout {
 	BTRFS_LIST_LAYOUT_DEFAULT = 0,
 	BTRFS_LIST_LAYOUT_TABLE,
-	BTRFS_LIST_LAYOUT_RAW,
 	BTRFS_LIST_LAYOUT_JSON
 };
 
@@ -1202,23 +1201,6 @@ static void print_subvolume_column(struct root_info *subv,
 	}
 }
 
-static void print_one_subvol_info_raw(struct root_info *subv,
-		const char *raw_prefix)
-{
-	int i;
-
-	for (i = 0; i < BTRFS_LIST_ALL; i++) {
-		if (!btrfs_list_columns[i].need_print)
-			continue;
-
-		if (raw_prefix)
-			pr_verbose(LOG_DEFAULT, "%s",raw_prefix);
-
-		print_subvolume_column(subv, i);
-	}
-	pr_verbose(LOG_DEFAULT, "\n");
-}
-
 static void print_one_subvol_info_table(struct root_info *subv)
 {
 	int i;
@@ -1349,7 +1331,7 @@ static void print_one_subvol_info_json(struct format_ctx *fctx,
 
 
 static void print_all_subvol_info(struct rb_root *sorted_tree,
-		  enum btrfs_list_layout layout, const char *raw_prefix)
+		  enum btrfs_list_layout layout)
 {
 	struct rb_node *n;
 	struct root_info *entry;
@@ -1377,9 +1359,6 @@ static void print_all_subvol_info(struct rb_root *sorted_tree,
 		case BTRFS_LIST_LAYOUT_TABLE:
 			print_one_subvol_info_table(entry);
 			break;
-		case BTRFS_LIST_LAYOUT_RAW:
-			print_one_subvol_info_raw(entry, raw_prefix);
-			break;
 		case BTRFS_LIST_LAYOUT_JSON:
 			print_one_subvol_info_json(&fctx, entry);
 			break;
@@ -1425,8 +1404,7 @@ static int btrfs_list_subvols(int fd, struct rb_root *root_lookup)
 
 static int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 		       struct btrfs_list_comparer_set *comp_set,
-		       enum btrfs_list_layout layout, int full_path,
-		       const char *raw_prefix)
+		       enum btrfs_list_layout layout, int full_path)
 {
 	struct rb_root root_lookup;
 	struct rb_root root_sort;
@@ -1448,7 +1426,7 @@ static int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter
 	filter_and_sort_subvol(&root_lookup, &root_sort, filter_set,
 				 comp_set, top_id);
 
-	print_all_subvol_info(&root_sort, layout, raw_prefix);
+	print_all_subvol_info(&root_sort, layout);
 	rb_free_nodes(&root_lookup, free_root_info);
 
 	return 0;
@@ -1726,7 +1704,7 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 		layout = BTRFS_LIST_LAYOUT_JSON;
 
 	ret = btrfs_list_subvols_print(fd, filter_set, comparer_set,
-			layout, !is_list_all && !is_only_in_path, NULL);
+			layout, !is_list_all && !is_only_in_path);
 
 out:
 	close(fd);
-- 
2.45.2

[PATCH 5/8] btrfs-progs: subvol list: remove unused filters

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

BTRFS_LIST_FILTER_ROOTID hasn't been used since commit 9e73a416f0ac
("btrfs-progs: use libbtrfsutil for get-default"), and
BTRFS_LIST_FILTER_BY_PARENT hasn't been used since commit 9005b603d723
("btrfs-progs: use libbtrfsutil for subvol show").

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 cmds/subvolume-list.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
index 060f4f31..cfe165f9 100644
--- a/cmds/subvolume-list.c
+++ b/cmds/subvolume-list.c
@@ -193,7 +193,6 @@ enum btrfs_list_column_enum {
 };
 
 enum btrfs_list_filter_enum {
-	BTRFS_LIST_FILTER_ROOTID,
 	BTRFS_LIST_FILTER_SNAPSHOT_ONLY,
 	BTRFS_LIST_FILTER_FLAGS,
 	BTRFS_LIST_FILTER_GEN,
@@ -206,7 +205,6 @@ enum btrfs_list_filter_enum {
 	BTRFS_LIST_FILTER_CGEN_MORE,
 	BTRFS_LIST_FILTER_TOPID_EQUAL,
 	BTRFS_LIST_FILTER_FULL_PATH,
-	BTRFS_LIST_FILTER_BY_PARENT,
 	BTRFS_LIST_FILTER_DELETED,
 	BTRFS_LIST_FILTER_MAX,
 };
@@ -932,11 +930,6 @@ static int list_subvol_search(int fd, struct rb_root *root_lookup)
 	return 0;
 }
 
-static int filter_by_rootid(struct root_info *ri, u64 data)
-{
-	return ri->root_id == data;
-}
-
 static int filter_snapshot(struct root_info *ri, u64 data)
 {
 	return !!ri->root_offset;
@@ -1005,18 +998,12 @@ static int filter_full_path(struct root_info *ri, u64 data)
 	return 1;
 }
 
-static int filter_by_parent(struct root_info *ri, u64 data)
-{
-	return !uuid_compare(ri->puuid, (u8 *)(unsigned long)data);
-}
-
 static int filter_deleted(struct root_info *ri, u64 data)
 {
 	return ri->deleted;
 }
 
 static btrfs_list_filter_func all_filter_funcs[] = {
-	[BTRFS_LIST_FILTER_ROOTID]		= filter_by_rootid,
 	[BTRFS_LIST_FILTER_SNAPSHOT_ONLY]	= filter_snapshot,
 	[BTRFS_LIST_FILTER_FLAGS]		= filter_flags,
 	[BTRFS_LIST_FILTER_GEN_MORE]		= filter_gen_more,
@@ -1027,7 +1014,6 @@ static btrfs_list_filter_func all_filter_funcs[] = {
 	[BTRFS_LIST_FILTER_CGEN_EQUAL]          = filter_cgen_equal,
 	[BTRFS_LIST_FILTER_TOPID_EQUAL]		= filter_topid_equal,
 	[BTRFS_LIST_FILTER_FULL_PATH]		= filter_full_path,
-	[BTRFS_LIST_FILTER_BY_PARENT]		= filter_by_parent,
 	[BTRFS_LIST_FILTER_DELETED]		= filter_deleted,
 };
 
-- 
2.45.2

[PATCH 6/8] btrfs-progs: subvol list: document and test actual behavior of paths

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The way btrfs subvol list prints paths and what the -o and -a flags do
are all nonsense.

Apparently, very early versions of Btrfs had a concept of a "top level"
of subvolumes rather than the root filesystem tree that we have today;
see commit 4ff9e2af1721 ("Add btrfs-list for listing subvolumes"). The
original subvol list code tracked the ID of that top level subvolume.
Eventually, 5 became the only possibility for the top level, and -o, -a,
and path printing were based on that. Commit 4f5ebb3ef553 ("Btrfs-progs:
fix to make list specified directory's subvolumes work") broke this and
changed the top level to be the same as the parent subvolume ID, which
gave us the illogical behavior we have today.

It has been this way for a decade, so we're probably stuck with it. But
let's at least document precisely what these all do in preparation for
adding sensible options. Let's also add tests in preparation for the
upcoming changes.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 Documentation/btrfs-subvolume.rst             |  20 ++-
 cmds/subvolume-list.c                         |  13 +-
 .../026-subvolume-list-path-filtering/test.sh | 118 ++++++++++++++++++
 3 files changed, 143 insertions(+), 8 deletions(-)
 create mode 100755 tests/cli-tests/026-subvolume-list-path-filtering/test.sh

diff --git a/Documentation/btrfs-subvolume.rst b/Documentation/btrfs-subvolume.rst
index d1e89f15..fe84ab1c 100644
--- a/Documentation/btrfs-subvolume.rst
+++ b/Documentation/btrfs-subvolume.rst
@@ -134,7 +134,8 @@ list [options] [-G [\+|-]<value>] [-C [+|-]<value>] [--sort=rootid,gen,ogen,path
 
         where *ID* is subvolume's (root)id, *generation* is an internal counter which is
         updated every transaction, *parent_ID* is the same as the parent subvolume's id,
-        and *path* is the relative path of the subvolume to the top level subvolume.
+        and *path* is the path of the subvolume. The exact meaning of *path*
+        depends on the **Path filtering** option used.
         The subvolume's ID may be used by the subvolume set-default command,
         or at mount time via the *subvolid=* option.
 
@@ -143,11 +144,20 @@ list [options] [-G [\+|-]<value>] [-C [+|-]<value>] [--sort=rootid,gen,ogen,path
         Path filtering:
 
         -o
-                Print only subvolumes below specified <path>. Note that this is not a
-                recursive command, and won't show nested subvolumes under <path>.
+                Print only the immediate children subvolumes of the subvolume
+                containing <path>. Paths are printed relative to the root of
+                the filesystem.
         -a
-                print all the subvolumes in the filesystem and distinguish between
-                absolute and relative path with respect to the given *path*.
+                Print all subvolumes in the filesystem other than the root
+                subvolume. Paths are printed relative to the root of the
+                filesystem, except that subvolumes that are not an immediate
+                child of the subvolume containing <path> are prefixed with
+                "<FS_TREE>/".
+
+        If none of these are given, print all subvolumes in the filesystem
+        other than the root subvolume. Paths below the subvolume containing
+        <path> are printed relative to that subvolume, and other paths are
+        printed relative to the root of the filesystem.
 
         Field selection:
 
diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
index cfe165f9..3b32a5ff 100644
--- a/cmds/subvolume-list.c
+++ b/cmds/subvolume-list.c
@@ -57,9 +57,16 @@ static const char * const cmd_subvolume_list_usage[] = {
 	"List subvolumes and snapshots in the filesystem.",
 	"",
 	"Path filtering:",
-	OPTLINE("-o", "print only subvolumes below specified path"),
-	OPTLINE("-a", "print all the subvolumes in the filesystem and "
-		"distinguish absolute and relative path with respect to the given <path>"),
+	OPTLINE("-o", "print only the immediate children subvolumes of the "
+		"subvolume containing <path>"),
+	OPTLINE("-a", "print all subvolumes in the filesystem other than the "
+		"root subvolume, and prefix subvolumes that are not an "
+		"immediate child of the subvolume containing <path> with "
+		"\"<FS_TREE>/\""),
+	"",
+	"If none of these are given, print all subvolumes other than the root",
+	"subvolume relative to the subvolume containing <path> if below it,",
+	"otherwise relative to the root of the filesystem.",
 	"",
 	"Field selection:",
 	OPTLINE("-p", "print parent ID"),
diff --git a/tests/cli-tests/026-subvolume-list-path-filtering/test.sh b/tests/cli-tests/026-subvolume-list-path-filtering/test.sh
new file mode 100755
index 00000000..1b272ddc
--- /dev/null
+++ b/tests/cli-tests/026-subvolume-list-path-filtering/test.sh
@@ -0,0 +1,118 @@
+#!/bin/bash
+# Test how btrfs subvolume list prints paths, including all of the weird
+# accidental behavior.
+
+source "$TEST_TOP/common" || exit
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+
+setup_root_helper
+prepare_test_dev
+
+run_check_mkfs_test_dev
+run_check_mount_test_dev
+
+cd "$TEST_MNT"
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "a"
+run_check $SUDO_HELPER mkdir "a/b"
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "a/b/c"
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "a/b/c/d"
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "a/e"
+
+subvol_list_paths() {
+	run_check_stdout $SUDO_HELPER "$TOP/btrfs" subvolume list "$@" | sed 's/.*path //'
+}
+
+expect_subvol_list_paths() {
+	diff -u - <(subvol_list_paths "$@") || _fail "wrong output from btrfs subvolume list $*"
+}
+
+### No options ###
+
+# Paths are relative to the given subvolume if they are beneath it and relative
+# to the root of the filesystem otherwise.
+expect_subvol_list_paths . << EOF
+a
+a/b/c
+a/b/c/d
+a/e
+EOF
+
+expect_subvol_list_paths a << EOF
+a
+b/c
+b/c/d
+e
+EOF
+
+expect_subvol_list_paths a/b/c << EOF
+a
+a/b/c
+d
+a/e
+EOF
+
+# If passed a directory, it's treated as the containing subvolume.
+expect_subvol_list_paths a/b << EOF
+a
+b/c
+b/c/d
+e
+EOF
+
+### -a ###
+
+# Paths are relative to the root of the filesystem. Subvolumes that are not an
+# immediate child of the passed subvolume are prefixed with <FS_TREE>/.
+expect_subvol_list_paths -a . << EOF
+a
+<FS_TREE>/a/b/c
+<FS_TREE>/a/b/c/d
+<FS_TREE>/a/e
+EOF
+
+expect_subvol_list_paths -a a << EOF
+<FS_TREE>/a
+a/b/c
+<FS_TREE>/a/b/c/d
+a/e
+EOF
+
+# If passed a directory, it's treated as the containing subvolume.
+expect_subvol_list_paths -a a/b << EOF
+<FS_TREE>/a
+a/b/c
+<FS_TREE>/a/b/c/d
+a/e
+EOF
+
+### -o ###
+
+# Only immediate children of the passed subvolume are printed, and they are
+# printed relative to the root of the filesystem.
+expect_subvol_list_paths -o . << EOF
+a
+EOF
+
+expect_subvol_list_paths -o a << EOF
+a/b/c
+a/e
+EOF
+
+# If passed a directory, it's treated as the containing subvolume.
+expect_subvol_list_paths -o a/b << EOF
+a/b/c
+a/e
+EOF
+
+expect_subvol_list_paths -o a/b/c << EOF
+a/b/c/d
+EOF
+
+expect_subvol_list_paths -o a/b/c/d << EOF
+EOF
+
+cd ..
+run_check_umount_test_dev
-- 
2.45.2

[PATCH 7/8] btrfs-progs: subvol list: use libbtrfsutil

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

btrfs subvol list has its own subvolume walking implementation that we
can replace with a libbtrfsutil subvolume iterator. Most of the changed
lines are removing the old implementation and mechanically updating the
comparators, filters, and printers to use libbtrfsutil's subvolume info.
The interesting parts are:

1. We can replace the red-black tree of subvolumes with an array that we
   qsort.
2. Listing deleted subvolumes needs a different codepath, but we don't
   need a filter for it anymore.
3. We need some hacks to maintain the weird path behavior documented in
   the previous commit.

In addition to removing a bunch of redundant code, this also prepares us
for allowing subvol list by unprivileged users in some cases.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 cmds/subvolume-list.c | 967 +++++++++++-------------------------------
 1 file changed, 259 insertions(+), 708 deletions(-)

diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
index 3b32a5ff..fa5082af 100644
--- a/cmds/subvolume-list.c
+++ b/cmds/subvolume-list.c
@@ -15,6 +15,7 @@
  */
 
 #include "kerncompat.h"
+#include <inttypes.h>
 #include <sys/ioctl.h>
 #include <getopt.h>
 #include <time.h>
@@ -26,6 +27,7 @@
 #include <stdbool.h>
 #include <unistd.h>
 #include <uuid/uuid.h>
+#include "libbtrfsutil/btrfsutil.h"
 #include "kernel-lib/rbtree.h"
 #include "kernel-lib/rbtree_types.h"
 #include "kernel-shared/accessors.h"
@@ -108,54 +110,14 @@ enum btrfs_list_layout {
 	BTRFS_LIST_LAYOUT_JSON
 };
 
-/*
- * one of these for each root we find.
- */
 struct root_info {
-	struct rb_node rb_node;
-	struct rb_node sort_node;
-
-	/* this root's id */
-	u64 root_id;
-
-	/* equal the offset of the root's key */
-	u64 root_offset;
-
-	/* flags of the root */
-	u64 flags;
-
-	/* the id of the root that references this one */
-	u64 ref_tree;
-
-	/* the dir id we're in from ref_tree */
-	u64 dir_id;
-
-	u64 top_id;
-
-	/* generation when the root is created or last updated */
-	u64 gen;
-
-	/* creation generation of this root in sec*/
-	u64 ogen;
-
-	/* creation time of this root in sec*/
-	time_t otime;
-
-	u8 uuid[BTRFS_UUID_SIZE];
-	u8 puuid[BTRFS_UUID_SIZE];
-	u8 ruuid[BTRFS_UUID_SIZE];
-
-	/* path from the subvol we live in to this root, including the
-	 * root's name.  This is null until we do the extra lookup ioctl.
-	 */
+	struct btrfs_util_subvolume_info info;
 	char *path;
+};
 
-	/* the name of this root in the directory it lives in */
-	char *name;
-
-	char *full_path;
-
-	int deleted;
+struct subvol_list {
+	size_t num;
+	struct root_info subvols[];
 };
 
 typedef int (*btrfs_list_filter_func)(struct root_info *, u64);
@@ -212,7 +174,7 @@ enum btrfs_list_filter_enum {
 	BTRFS_LIST_FILTER_CGEN_MORE,
 	BTRFS_LIST_FILTER_TOPID_EQUAL,
 	BTRFS_LIST_FILTER_FULL_PATH,
-	BTRFS_LIST_FILTER_DELETED,
+	BTRFS_LIST_FILTER_REMOVE_PATH_PREFIX,
 	BTRFS_LIST_FILTER_MAX,
 };
 
@@ -224,16 +186,6 @@ enum btrfs_list_comp_enum {
 	BTRFS_LIST_COMP_MAX,
 };
 
-static inline struct root_info *to_root_info(struct rb_node *node)
-{
-	return rb_entry(node, struct root_info, rb_node);
-}
-
-static inline struct root_info *to_root_info_sorted(struct rb_node *node)
-{
-	return rb_entry(node, struct root_info, sort_node);
-}
-
 static struct {
 	char	*name;
 	char	*column_name;
@@ -317,9 +269,9 @@ static void btrfs_list_setup_print_column(enum btrfs_list_column_enum column)
 static int comp_entry_with_rootid(const struct root_info *entry1,
 				  const struct root_info *entry2)
 {
-	if (entry1->root_id > entry2->root_id)
+	if (entry1->info.id > entry2->info.id)
 		return 1;
-	else if (entry1->root_id < entry2->root_id)
+	else if (entry1->info.id < entry2->info.id)
 		return -1;
 	return 0;
 }
@@ -327,9 +279,9 @@ static int comp_entry_with_rootid(const struct root_info *entry1,
 static int comp_entry_with_gen(const struct root_info *entry1,
 			       const struct root_info *entry2)
 {
-	if (entry1->gen > entry2->gen)
+	if (entry1->info.generation > entry2->info.generation)
 		return 1;
-	else if (entry1->gen < entry2->gen)
+	else if (entry1->info.generation < entry2->info.generation)
 		return -1;
 	return 0;
 }
@@ -337,9 +289,9 @@ static int comp_entry_with_gen(const struct root_info *entry1,
 static int comp_entry_with_ogen(const struct root_info *entry1,
 				const struct root_info *entry2)
 {
-	if (entry1->ogen > entry2->ogen)
+	if (entry1->info.otransid > entry2->info.otransid)
 		return 1;
-	else if (entry1->ogen < entry2->ogen)
+	else if (entry1->info.otransid < entry2->info.otransid)
 		return -1;
 	return 0;
 }
@@ -347,9 +299,9 @@ static int comp_entry_with_ogen(const struct root_info *entry1,
 static int comp_entry_with_path(const struct root_info *entry1,
 				const struct root_info *entry2)
 {
-	if (strcmp(entry1->full_path, entry2->full_path) > 0)
+	if (strcmp(entry1->path, entry2->path) > 0)
 		return 1;
-	else if (strcmp(entry1->full_path, entry2->full_path) < 0)
+	else if (strcmp(entry1->path, entry2->path) < 0)
 		return -1;
 	return 0;
 }
@@ -418,9 +370,9 @@ static int btrfs_list_setup_comparer(struct btrfs_list_comparer_set **comp_set,
 	return 0;
 }
 
-static int sort_comp(const struct root_info *entry1, const struct root_info *entry2,
-		     struct btrfs_list_comparer_set *set)
+static int sort_comp(const void *entry1, const void *entry2, void *arg)
 {
+	struct btrfs_list_comparer_set *set = arg;
 	bool rootid_compared = false;
 	int i, ret = 0;
 
@@ -447,567 +399,96 @@ static int sort_comp(const struct root_info *entry1, const struct root_info *ent
 	return ret;
 }
 
-static int sort_tree_insert(struct rb_root *sort_tree,
-			    struct root_info *ins,
-			    struct btrfs_list_comparer_set *comp_set)
+static void sort_subvols(struct btrfs_list_comparer_set *comp_set,
+			 struct subvol_list *subvols)
 {
-	struct rb_node **p = &sort_tree->rb_node;
-	struct rb_node *parent = NULL;
-	struct root_info *curr;
-	int ret;
-
-	while (*p) {
-		parent = *p;
-		curr = to_root_info_sorted(parent);
-
-		ret = sort_comp(ins, curr, comp_set);
-		if (ret < 0)
-			p = &(*p)->rb_left;
-		else if (ret > 0)
-			p = &(*p)->rb_right;
-		else
-			return -EEXIST;
-	}
-
-	rb_link_node(&ins->sort_node, parent, p);
-	rb_insert_color(&ins->sort_node, sort_tree);
-	return 0;
-}
-
-/*
- * insert a new root into the tree.  returns the existing root entry
- * if one is already there.  Both root_id and ref_tree are used
- * as the key
- */
-static int root_tree_insert(struct rb_root *root_tree,
-			    struct root_info *ins)
-{
-	struct rb_node **p = &root_tree->rb_node;
-	struct rb_node * parent = NULL;
-	struct root_info *curr;
-	int ret;
-
-	while(*p) {
-		parent = *p;
-		curr = to_root_info(parent);
-
-		ret = comp_entry_with_rootid(ins, curr);
-		if (ret < 0)
-			p = &(*p)->rb_left;
-		else if (ret > 0)
-			p = &(*p)->rb_right;
-		else
-			return -EEXIST;
-	}
-
-	rb_link_node(&ins->rb_node, parent, p);
-	rb_insert_color(&ins->rb_node, root_tree);
-	return 0;
-}
-
-/*
- * find a given root id in the tree.  We return the smallest one,
- * rb_next can be used to move forward looking for more if required
- */
-static struct root_info *root_tree_search(struct rb_root *root_tree,
-					  u64 root_id)
-{
-	struct rb_node *n = root_tree->rb_node;
-	struct root_info *entry;
-	struct root_info tmp;
-	int ret;
-
-	tmp.root_id = root_id;
-
-	while(n) {
-		entry = to_root_info(n);
-
-		ret = comp_entry_with_rootid(&tmp, entry);
-		if (ret < 0)
-			n = n->rb_left;
-		else if (ret > 0)
-			n = n->rb_right;
-		else
-			return entry;
-	}
-	return NULL;
-}
-
-static int update_root(struct rb_root *root_lookup,
-		       u64 root_id, u64 ref_tree, u64 root_offset, u64 flags,
-		       u64 dir_id, char *name, int name_len, u64 ogen, u64 gen,
-		       time_t otime, u8 *uuid, u8 *puuid, u8 *ruuid)
-{
-	struct root_info *ri;
-
-	ri = root_tree_search(root_lookup, root_id);
-	if (!ri || ri->root_id != root_id)
-		return -ENOENT;
-	if (name && name_len > 0) {
-		free(ri->name);
-
-		ri->name = malloc(name_len + 1);
-		if (!ri->name) {
-			error_msg(ERROR_MSG_MEMORY, NULL);
-			exit(1);
-		}
-		strncpy_null(ri->name, name, name_len);
-	}
-	if (ref_tree)
-		ri->ref_tree = ref_tree;
-	if (root_offset)
-		ri->root_offset = root_offset;
-	if (flags)
-		ri->flags = flags;
-	if (dir_id)
-		ri->dir_id = dir_id;
-	if (gen)
-		ri->gen = gen;
-	if (ogen)
-		ri->ogen = ogen;
-	if (!ri->ogen && root_offset)
-		ri->ogen = root_offset;
-	if (otime)
-		ri->otime = otime;
-	if (uuid)
-		memcpy(&ri->uuid, uuid, BTRFS_UUID_SIZE);
-	if (puuid)
-		memcpy(&ri->puuid, puuid, BTRFS_UUID_SIZE);
-	if (ruuid)
-		memcpy(&ri->ruuid, ruuid, BTRFS_UUID_SIZE);
-
-	return 0;
-}
-
-/*
- * add_root - update the existed root, or allocate a new root and insert it
- *	      into the lookup tree.
- * root_id: object id of the root
- * ref_tree: object id of the referring root.
- * root_offset: offset value of the root'key
- * dir_id: inode id of the directory in ref_tree where this root can be found.
- * name: the name of root_id in that directory
- * name_len: the length of name
- * ogen: the original generation of the root
- * gen: the current generation of the root
- * otime: the original time (creation time) of the root
- * uuid: uuid of the root
- * puuid: uuid of the root parent if any
- * ruuid: uuid of the received subvol, if any
- */
-static int add_root(struct rb_root *root_lookup,
-		    u64 root_id, u64 ref_tree, u64 root_offset, u64 flags,
-		    u64 dir_id, char *name, int name_len, u64 ogen, u64 gen,
-		    time_t otime, u8 *uuid, u8 *puuid, u8 *ruuid)
-{
-	struct root_info *ri;
-	int ret;
-
-	ret = update_root(root_lookup, root_id, ref_tree, root_offset, flags,
-			  dir_id, name, name_len, ogen, gen, otime,
-			  uuid, puuid, ruuid);
-	if (!ret)
-		return 0;
-
-	ri = calloc(1, sizeof(*ri));
-	if (!ri) {
-		error_msg(ERROR_MSG_MEMORY, NULL);
-		exit(1);
-	}
-	ri->root_id = root_id;
-
-	if (name && name_len > 0) {
-		ri->name = malloc(name_len + 1);
-		if (!ri->name) {
-			error_msg(ERROR_MSG_MEMORY, NULL);
-			exit(1);
-		}
-		strncpy_null(ri->name, name, name_len);
-	}
-	if (ref_tree)
-		ri->ref_tree = ref_tree;
-	if (dir_id)
-		ri->dir_id = dir_id;
-	if (root_offset)
-		ri->root_offset = root_offset;
-	if (flags)
-		ri->flags = flags;
-	if (gen)
-		ri->gen = gen;
-	if (ogen)
-		ri->ogen = ogen;
-	if (!ri->ogen && root_offset)
-		ri->ogen = root_offset;
-	if (otime)
-		ri->otime = otime;
-
-	if (uuid)
-		memcpy(&ri->uuid, uuid, BTRFS_UUID_SIZE);
-
-	if (puuid)
-		memcpy(&ri->puuid, puuid, BTRFS_UUID_SIZE);
-
-	if (ruuid)
-		memcpy(&ri->ruuid, ruuid, BTRFS_UUID_SIZE);
-
-	ret = root_tree_insert(root_lookup, ri);
-	if (ret < 0) {
-		errno = -ret;
-		error("failed to insert subvolume %llu to tree: %m", root_id);
-		exit(1);
-	}
-	return 0;
-}
-
-/*
- * Simplified add_root for back references, omits the uuid and original info
- * parameters, root offset and flags.
- */
-static int add_root_backref(struct rb_root *root_lookup, u64 root_id,
-		u64 ref_tree, u64 dir_id, char *name, int name_len)
-{
-	return add_root(root_lookup, root_id, ref_tree, 0, 0, dir_id, name,
-			name_len, 0, 0, 0, NULL, NULL, NULL);
-}
-
-static void free_root_info(struct rb_node *node)
-{
-	struct root_info *ri;
-
-	ri = to_root_info(node);
-	free(ri->name);
-	free(ri->path);
-	free(ri->full_path);
-	free(ri);
-}
-
-/*
- * for a given root_info, search through the root_lookup tree to construct
- * the full path name to it.
- *
- * This can't be called until all the root_info->path fields are filled
- * in by lookup_ino_path
- */
-static int resolve_root(struct rb_root *rl, struct root_info *ri,
-		       u64 top_id)
-{
-	char *full_path = NULL;
-	int len = 0;
-	struct root_info *found;
-
-	/*
-	 * we go backwards from the root_info object and add pathnames
-	 * from parent directories as we go.
-	 */
-	found = ri;
-	while (1) {
-		char *tmp;
-		u64 next;
-		int add_len;
-
-		/*
-		 * ref_tree = 0 indicates the subvolume
-		 * has been deleted.
-		 */
-		if (!found->ref_tree) {
-			free(full_path);
-			return -ENOENT;
-		}
-
-		add_len = strlen(found->path);
-
-		if (full_path) {
-			/* room for / and for null */
-			tmp = malloc(add_len + 2 + len);
-			if (!tmp) {
-				error_msg(ERROR_MSG_MEMORY, NULL);
-				exit(1);
-			}
-			memcpy(tmp + add_len + 1, full_path, len);
-			tmp[add_len] = '/';
-			memcpy(tmp, found->path, add_len);
-			tmp [add_len + len + 1] = '\0';
-			free(full_path);
-			full_path = tmp;
-			len += add_len + 1;
-		} else {
-			full_path = strdup(found->path);
-			len = add_len;
-		}
-		if (!ri->top_id)
-			ri->top_id = found->ref_tree;
-
-		next = found->ref_tree;
-		if (next == top_id)
-			break;
-		/*
-		* if the ref_tree = BTRFS_FS_TREE_OBJECTID,
-		* we are at the top
-		*/
-		if (next == BTRFS_FS_TREE_OBJECTID)
-			break;
-		/*
-		* if the ref_tree wasn't in our tree of roots, the
-		* subvolume was deleted.
-		*/
-		found = root_tree_search(rl, next);
-		if (!found) {
-			free(full_path);
-			return -ENOENT;
-		}
-	}
-
-	ri->full_path = full_path;
-
-	return 0;
-}
-
-/*
- * for a single root_info, ask the kernel to give us a path name
- * inside it's ref_root for the dir_id where it lives.
- *
- * This fills in root_info->path with the path to the directory and and
- * appends this root's name.
- */
-static int lookup_ino_path(int fd, struct root_info *ri)
-{
-	struct btrfs_ioctl_ino_lookup_args args;
-	int ret;
-
-	if (ri->path)
-		return 0;
-
-	if (!ri->ref_tree)
-		return -ENOENT;
-
-	memset(&args, 0, sizeof(args));
-	args.treeid = ri->ref_tree;
-	args.objectid = ri->dir_id;
-
-	ret = ioctl(fd, BTRFS_IOC_INO_LOOKUP, &args);
-	if (ret < 0) {
-		if (errno == ENOENT) {
-			ri->ref_tree = 0;
-			return -ENOENT;
-		}
-		error("failed to lookup path for root %llu: %m", ri->ref_tree);
-		return ret;
-	}
-
-	if (args.name[0]) {
-		/*
-		 * we're in a subdirectory of ref_tree, the kernel ioctl
-		 * puts a / in there for us
-		 */
-		ri->path = malloc(strlen(ri->name) + strlen(args.name) + 1);
-		if (!ri->path) {
-			error_msg(ERROR_MSG_MEMORY, NULL);
-			exit(1);
-		}
-		strcpy(ri->path, args.name);
-		strcat(ri->path, ri->name);
-	} else {
-		/* we're at the root of ref_tree */
-		ri->path = strdup(ri->name);
-		if (!ri->path) {
-			perror("strdup failed");
-			exit(1);
-		}
-	}
-	return 0;
-}
-
-static int list_subvol_search(int fd, struct rb_root *root_lookup)
-{
-	int ret;
-	struct btrfs_tree_search_args args;
-	struct btrfs_ioctl_search_key *sk;
-	struct btrfs_root_ref *ref;
-	struct btrfs_root_item *ri;
-	unsigned long off;
-	int name_len;
-	char *name;
-	u64 dir_id;
-	u64 gen = 0;
-	u64 ogen;
-	u64 flags;
-	int i;
-
-	root_lookup->rb_node = NULL;
-
-	memset(&args, 0, sizeof(args));
-	sk = btrfs_tree_search_sk(&args);
-	sk->tree_id = BTRFS_ROOT_TREE_OBJECTID;
-	/* Search both live and deleted subvolumes */
-	sk->min_type = BTRFS_ROOT_ITEM_KEY;
-	sk->max_type = BTRFS_ROOT_BACKREF_KEY;
-	sk->min_objectid = BTRFS_FS_TREE_OBJECTID;
-	sk->max_objectid = BTRFS_LAST_FREE_OBJECTID;
-	sk->max_offset = (u64)-1;
-	sk->max_transid = (u64)-1;
-
-	while(1) {
-		sk->nr_items = 4096;
-		ret = btrfs_tree_search_ioctl(fd, &args);
-		if (ret < 0)
-			return ret;
-		if (sk->nr_items == 0)
-			break;
-
-		off = 0;
-
-		/*
-		 * for each item, pull the key out of the header and then
-		 * read the root_ref item it contains
-		 */
-		for (i = 0; i < sk->nr_items; i++) {
-			struct btrfs_ioctl_search_header sh;
-
-			memcpy(&sh, btrfs_tree_search_data(&args, off), sizeof(sh));
-			off += sizeof(sh);
-			if (sh.type == BTRFS_ROOT_BACKREF_KEY) {
-				ref = btrfs_tree_search_data(&args, off);
-				name_len = btrfs_stack_root_ref_name_len(ref);
-				name = (char *)(ref + 1);
-				dir_id = btrfs_stack_root_ref_dirid(ref);
-
-				add_root_backref(root_lookup, sh.objectid,
-						sh.offset, dir_id, name,
-						name_len);
-			} else if (sh.type == BTRFS_ROOT_ITEM_KEY &&
-				   (sh.objectid >= BTRFS_FIRST_FREE_OBJECTID ||
-				    sh.objectid == BTRFS_FS_TREE_OBJECTID)) {
-				time_t otime;
-				u8 uuid[BTRFS_UUID_SIZE];
-				u8 puuid[BTRFS_UUID_SIZE];
-				u8 ruuid[BTRFS_UUID_SIZE];
-
-				ri = btrfs_tree_search_data(&args, off);
-				gen = btrfs_root_generation(ri);
-				flags = btrfs_root_flags(ri);
-				if(sh.len >= sizeof(struct btrfs_root_item)) {
-					/*
-					 * The new full btrfs_root_item with
-					 * timestamp and UUID.
-					 */
-					otime = btrfs_stack_timespec_sec(&ri->otime);
-					ogen = btrfs_root_otransid(ri);
-					memcpy(uuid, ri->uuid, BTRFS_UUID_SIZE);
-					memcpy(puuid, ri->parent_uuid, BTRFS_UUID_SIZE);
-					memcpy(ruuid, ri->received_uuid, BTRFS_UUID_SIZE);
-				} else {
-					/*
-					 * The old v0 root item, which doesn't
-					 * have timestamp nor UUID.
-					 */
-					otime = 0;
-					ogen = 0;
-					memset(uuid, 0, BTRFS_UUID_SIZE);
-					memset(puuid, 0, BTRFS_UUID_SIZE);
-					memset(ruuid, 0, BTRFS_UUID_SIZE);
-				}
-
-				add_root(root_lookup, sh.objectid, 0,
-					 sh.offset, flags, 0, NULL, 0, ogen,
-					 gen, otime, uuid, puuid, ruuid);
-			}
-
-			off += sh.len;
-
-			sk->min_objectid = sh.objectid;
-			sk->min_type = sh.type;
-			sk->min_offset = sh.offset;
-		}
-		sk->min_offset++;
-		if (!sk->min_offset)
-			sk->min_type++;
-		else
-			continue;
-
-		if (sk->min_type > BTRFS_ROOT_BACKREF_KEY) {
-			sk->min_type = BTRFS_ROOT_ITEM_KEY;
-			sk->min_objectid++;
-		} else
-			continue;
-
-		if (sk->min_objectid > sk->max_objectid)
-			break;
-	}
-
-	return 0;
+	qsort_r(subvols->subvols, subvols->num, sizeof(subvols->subvols[0]),
+		sort_comp, comp_set);
 }
 
 static int filter_snapshot(struct root_info *ri, u64 data)
 {
-	return !!ri->root_offset;
+	return !uuid_is_null(ri->info.parent_uuid);
 }
 
 static int filter_flags(struct root_info *ri, u64 flags)
 {
-	return ri->flags & flags;
+	return ri->info.flags & flags;
 }
 
 static int filter_gen_more(struct root_info *ri, u64 data)
 {
-	return ri->gen >= data;
+	return ri->info.generation >= data;
 }
 
 static int filter_gen_less(struct root_info *ri, u64 data)
 {
-	return ri->gen <= data;
+	return ri->info.generation <= data;
 }
 
 static int filter_gen_equal(struct root_info  *ri, u64 data)
 {
-	return ri->gen == data;
+	return ri->info.generation == data;
 }
 
 static int filter_cgen_more(struct root_info *ri, u64 data)
 {
-	return ri->ogen >= data;
+	return ri->info.otransid >= data;
 }
 
 static int filter_cgen_less(struct root_info *ri, u64 data)
 {
-	return ri->ogen <= data;
+	return ri->info.otransid <= data;
 }
 
 static int filter_cgen_equal(struct root_info *ri, u64 data)
 {
-	return ri->ogen == data;
+	return ri->info.otransid == data;
 }
 
 static int filter_topid_equal(struct root_info *ri, u64 data)
 {
-	return ri->top_id == data;
+	/* See the comment in print_subvolume_column() about top level. */
+	return ri->info.parent_id == data;
 }
 
 static int filter_full_path(struct root_info *ri, u64 data)
 {
-	if (ri->full_path && ri->top_id != data) {
+	/*
+	 * If this subvolume's parent is not the subvolume containing the path
+	 * given on the command line, prepend "<FS_TREE>/". This behavior is
+	 * nonsense, but we keep it for backwards compatibility. It was
+	 * introduced by the same change to top level mentioned in
+	 * print_subvolume_column().
+	 */
+	if (ri->info.parent_id != data) {
 		char *tmp;
-		char p[] = "<FS_TREE>";
-		int add_len = strlen(p);
-		int len = strlen(ri->full_path);
+		int ret;
 
-		tmp = malloc(len + add_len + 2);
-		if (!tmp) {
-			error_msg(ERROR_MSG_MEMORY, NULL);
+		ret = asprintf(&tmp, "<FS_TREE>/%s", ri->path);
+		if (ret == -1) {
+			error("out of memory");
 			exit(1);
 		}
-		memcpy(tmp + add_len + 1, ri->full_path, len);
-		tmp[len + add_len + 1] = '\0';
-		tmp[add_len] = '/';
-		memcpy(tmp, p, add_len);
-		free(ri->full_path);
-		ri->full_path = tmp;
+
+		free(ri->path);
+		ri->path = tmp;
 	}
 	return 1;
 }
 
-static int filter_deleted(struct root_info *ri, u64 data)
+static int filter_remove_path_prefix(struct root_info *ri, u64 data)
 {
-	return ri->deleted;
+	/*
+	 * If this subvolume is a descendant of the given path, remove that path
+	 * prefix. Otherwise, leave it alone. This is also nonsense that we keep
+	 * for backwards compatibility.
+	 */
+	const char *prefix = (const char *)data;
+	size_t len = strlen(prefix);
+	if (strncmp(ri->path, prefix, len) == 0 && ri->path[len] == '/')
+		memmove(ri->path, &ri->path[len + 1], strlen(ri->path) - len);
+	return 1;
 }
 
 static btrfs_list_filter_func all_filter_funcs[] = {
@@ -1021,7 +502,7 @@ static btrfs_list_filter_func all_filter_funcs[] = {
 	[BTRFS_LIST_FILTER_CGEN_EQUAL]          = filter_cgen_equal,
 	[BTRFS_LIST_FILTER_TOPID_EQUAL]		= filter_topid_equal,
 	[BTRFS_LIST_FILTER_FULL_PATH]		= filter_full_path,
-	[BTRFS_LIST_FILTER_DELETED]		= filter_deleted,
+	[BTRFS_LIST_FILTER_REMOVE_PATH_PREFIX]	= filter_remove_path_prefix,
 };
 
 /*
@@ -1060,9 +541,6 @@ static void btrfs_list_setup_filter(struct btrfs_list_filter_set **filter_set,
 
 	UASSERT(set->filters[set->nfilters].filter_func == NULL);
 
-	if (filter == BTRFS_LIST_FILTER_DELETED)
-		set->only_deleted = 1;
-
 	set->filters[set->nfilters].filter_func = all_filter_funcs[filter];
 	set->filters[set->nfilters].data = data;
 	set->nfilters++;
@@ -1076,12 +554,6 @@ static int filter_root(struct root_info *ri,
 	if (!set)
 		return 1;
 
-	if (set->only_deleted && !ri->deleted)
-		return 0;
-
-	if (!set->only_deleted && ri->deleted)
-		return 0;
-
 	for (i = 0; i < set->nfilters; i++) {
 		if (!set->filters[i].filter_func)
 			break;
@@ -1092,44 +564,6 @@ static int filter_root(struct root_info *ri,
 	return 1;
 }
 
-static void filter_and_sort_subvol(struct rb_root *all_subvols,
-				    struct rb_root *sort_tree,
-				    struct btrfs_list_filter_set *filter_set,
-				    struct btrfs_list_comparer_set *comp_set,
-				    u64 top_id)
-{
-	struct rb_node *n;
-	struct root_info *entry;
-	int ret;
-
-	sort_tree->rb_node = NULL;
-
-	n = rb_last(all_subvols);
-	while (n) {
-		entry = to_root_info(n);
-
-		ret = resolve_root(all_subvols, entry, top_id);
-		if (ret == -ENOENT) {
-			if (entry->root_id != BTRFS_FS_TREE_OBJECTID) {
-				entry->full_path = strdup("DELETED");
-				entry->deleted = 1;
-			} else {
-				/*
-				 * The full path is not supposed to be printed,
-				 * but we don't want to print an empty string,
-				 * in case it appears somewhere.
-				 */
-				entry->full_path = strdup("TOPLEVEL");
-				entry->deleted = 0;
-			}
-		}
-		ret = filter_root(entry, filter_set);
-		if (ret)
-			sort_tree_insert(sort_tree, entry, comp_set);
-		n = rb_prev(n);
-	}
-}
-
 static void print_subvolume_column(struct root_info *subv,
 				   enum btrfs_list_column_enum column)
 {
@@ -1140,54 +574,58 @@ static void print_subvolume_column(struct root_info *subv,
 
 	switch (column) {
 	case BTRFS_LIST_OBJECTID:
-		pr_verbose(LOG_DEFAULT, "%llu", subv->root_id);
+		pr_verbose(LOG_DEFAULT, "%" PRIu64, subv->info.id);
 		break;
 	case BTRFS_LIST_GENERATION:
-		pr_verbose(LOG_DEFAULT, "%llu", subv->gen);
+		pr_verbose(LOG_DEFAULT, "%" PRIu64, subv->info.generation);
 		break;
 	case BTRFS_LIST_OGENERATION:
-		pr_verbose(LOG_DEFAULT, "%llu", subv->ogen);
+		pr_verbose(LOG_DEFAULT, "%" PRIu64, subv->info.otransid);
 		break;
 	case BTRFS_LIST_PARENT:
-		pr_verbose(LOG_DEFAULT, "%llu", subv->ref_tree);
-		break;
+	/*
+	 * Top level used to mean something else, but since 4f5ebb3ef553
+	 * ("Btrfs-progs: fix to make list specified directory's subvolumes
+	 * work") it was always set to the parent ID. See
+	 * https://lore.kernel.org/all/bdd9af61-b408-c8d2-6697-84230b0bcf89@gmail.com/.
+	 */
 	case BTRFS_LIST_TOP_LEVEL:
-		pr_verbose(LOG_DEFAULT, "%llu", subv->top_id);
+		pr_verbose(LOG_DEFAULT, "%" PRIu64, subv->info.parent_id);
 		break;
 	case BTRFS_LIST_OTIME:
-		if (subv->otime) {
+		if (subv->info.otime.tv_sec) {
 			struct tm tm;
 
-			localtime_r(&subv->otime, &tm);
+			localtime_r(&subv->info.otime.tv_sec, &tm);
 			strftime(tstr, 256, "%Y-%m-%d %X", &tm);
 		} else
 			strcpy(tstr, "-");
 		pr_verbose(LOG_DEFAULT, "%s", tstr);
 		break;
 	case BTRFS_LIST_UUID:
-		if (uuid_is_null(subv->uuid))
+		if (uuid_is_null(subv->info.uuid))
 			strcpy(uuidparse, "-");
 		else
-			uuid_unparse(subv->uuid, uuidparse);
+			uuid_unparse(subv->info.uuid, uuidparse);
 		pr_verbose(LOG_DEFAULT, "%-36s", uuidparse);
 		break;
 	case BTRFS_LIST_PUUID:
-		if (uuid_is_null(subv->puuid))
+		if (uuid_is_null(subv->info.parent_uuid))
 			strcpy(uuidparse, "-");
 		else
-			uuid_unparse(subv->puuid, uuidparse);
+			uuid_unparse(subv->info.parent_uuid, uuidparse);
 		pr_verbose(LOG_DEFAULT, "%-36s", uuidparse);
 		break;
 	case BTRFS_LIST_RUUID:
-		if (uuid_is_null(subv->ruuid))
+		if (uuid_is_null(subv->info.received_uuid))
 			strcpy(uuidparse, "-");
 		else
-			uuid_unparse(subv->ruuid, uuidparse);
+			uuid_unparse(subv->info.received_uuid, uuidparse);
 		pr_verbose(LOG_DEFAULT, "%-36s", uuidparse);
 		break;
 	case BTRFS_LIST_PATH:
-		BUG_ON(!subv->full_path);
-		pr_verbose(LOG_DEFAULT, "%s", subv->full_path);
+		BUG_ON(!subv->path);
+		pr_verbose(LOG_DEFAULT, "%s", subv->path);
 		break;
 	default:
 		break;
@@ -1270,35 +708,34 @@ static void print_subvol_json_key(struct format_ctx *fctx,
 	column_name = btrfs_list_columns[column].name;
 	switch (column) {
 	case BTRFS_LIST_OBJECTID:
-		fmt_print(fctx, column_name, subv->root_id);
+		fmt_print(fctx, column_name, subv->info.id);
 		break;
 	case BTRFS_LIST_GENERATION:
-		fmt_print(fctx, column_name, subv->gen);
+		fmt_print(fctx, column_name, subv->info.generation);
 		break;
 	case BTRFS_LIST_OGENERATION:
-		fmt_print(fctx, column_name, subv->ogen);
+		fmt_print(fctx, column_name, subv->info.otransid);
 		break;
 	case BTRFS_LIST_PARENT:
-		fmt_print(fctx, column_name, subv->ref_tree);
-		break;
+	/* See the comment in print_subvolume_column() about top level. */
 	case BTRFS_LIST_TOP_LEVEL:
-		fmt_print(fctx, column_name, subv->top_id);
+		fmt_print(fctx, column_name, subv->info.parent_id);
 		break;
 	case BTRFS_LIST_OTIME:
-		fmt_print(fctx, column_name, subv->otime);
+		fmt_print(fctx, column_name, subv->info.otime.tv_sec);
 		break;
 	case BTRFS_LIST_UUID:
-		fmt_print(fctx, column_name, subv->uuid);
+		fmt_print(fctx, column_name, subv->info.uuid);
 		break;
 	case BTRFS_LIST_PUUID:
-		fmt_print(fctx, column_name, subv->puuid);
+		fmt_print(fctx, column_name, subv->info.parent_uuid);
 		break;
 	case BTRFS_LIST_RUUID:
-		fmt_print(fctx, column_name, subv->ruuid);
+		fmt_print(fctx, column_name, subv->info.received_uuid);
 		break;
 	case BTRFS_LIST_PATH:
-		BUG_ON(!subv->full_path);
-		fmt_print(fctx, column_name, subv->full_path);
+		BUG_ON(!subv->path);
+		fmt_print(fctx, column_name, subv->path);
 		break;
 	default:
 		break;
@@ -1323,10 +760,10 @@ static void print_one_subvol_info_json(struct format_ctx *fctx,
 }
 
 
-static void print_all_subvol_info(struct rb_root *sorted_tree,
+static void print_all_subvol_info(struct subvol_list *subvols,
 		  enum btrfs_list_layout layout)
 {
-	struct rb_node *n;
+	size_t i;
 	struct root_info *entry;
 	struct format_ctx fctx;
 
@@ -1337,13 +774,8 @@ static void print_all_subvol_info(struct rb_root *sorted_tree,
 		fmt_print_start_group(&fctx, "subvolume-list", JSON_TYPE_ARRAY);
 	}
 
-	n = rb_first(sorted_tree);
-	while (n) {
-		entry = to_root_info_sorted(n);
-
-		/* The toplevel subvolume is not listed by default */
-		if (entry->root_id == BTRFS_FS_TREE_OBJECTID)
-			goto next;
+	for (i = 0; i < subvols->num; i++) {
+		entry = &subvols->subvols[i];
 
 		switch (layout) {
 		case BTRFS_LIST_LAYOUT_DEFAULT:
@@ -1356,8 +788,6 @@ static void print_all_subvol_info(struct rb_root *sorted_tree,
 			print_one_subvol_info_json(&fctx, entry);
 			break;
 		}
-next:
-		n = rb_next(n);
 	}
 
 	if (layout == BTRFS_LIST_LAYOUT_JSON) {
@@ -1366,61 +796,170 @@ next:
 	}
 }
 
-static int btrfs_list_subvols(int fd, struct rb_root *root_lookup)
+static void free_subvol_list(struct subvol_list *subvols)
 {
-	int ret;
-	struct rb_node *n;
+	size_t i;
 
-	ret = list_subvol_search(fd, root_lookup);
+	if (subvols) {
+		for (i = 0; i < subvols->num; i++)
+			free(subvols->subvols[i].path);
+		free(subvols);
+	}
+}
+
+static struct subvol_list *btrfs_list_deleted_subvols(int fd,
+				struct btrfs_list_filter_set *filter_set)
+{
+	struct subvol_list *subvols = NULL;
+	uint64_t *ids = NULL;
+	size_t i, n;
+	enum btrfs_util_error err;
+	int ret = -1;
+
+	err = btrfs_util_deleted_subvolumes_fd(fd, &ids, &n);
+	if (err) {
+		error_btrfs_util(err);
+		return NULL;
+	}
+
+	subvols = malloc(sizeof(*subvols) + n * sizeof(subvols->subvols[0]));
+	if (!subvols) {
+		error("out of memory");
+		goto out;
+	}
+
+	subvols->num = 0;
+	for (i = 0; i < n; i++) {
+		struct root_info *subvol = &subvols->subvols[subvols->num];
+
+		err = btrfs_util_subvolume_info_fd(fd, ids[i], &subvol->info);
+		if (err == BTRFS_UTIL_ERROR_SUBVOLUME_NOT_FOUND) {
+			/*
+			 * The subvolume might have been cleaned up since it was
+			 * returned.
+			 */
+			continue;
+		} else if (err) {
+			error_btrfs_util(err);
+			goto out;
+		}
+
+		subvol->path = strdup("DELETED");
+		if (!subvol->path)
+			goto out;
+
+		if (!filter_root(subvol, filter_set)) {
+			free(subvol->path);
+			continue;
+		}
+
+		subvols->num++;
+	}
+
+	ret = 0;
+out:
 	if (ret) {
-		error("can't perform the search: %m");
-		return ret;
+		free_subvol_list(subvols);
+		subvols = NULL;
+	}
+	free(ids);
+	return subvols;
+}
+
+static struct subvol_list *btrfs_list_subvols(int fd,
+				struct btrfs_list_filter_set *filter_set)
+{
+	struct subvol_list *subvols;
+	size_t capacity = 4;
+	struct btrfs_util_subvolume_iterator *iter;
+	enum btrfs_util_error err;
+	int ret = -1;
+
+	subvols = malloc(sizeof(*subvols) +
+			 capacity * sizeof(subvols->subvols[0]));
+	if (!subvols) {
+		error("out of memory");
+		return NULL;
+	}
+	subvols->num = 0;
+
+	err = btrfs_util_create_subvolume_iterator_fd(fd,
+						      BTRFS_FS_TREE_OBJECTID, 0,
+						      &iter);
+	if (err) {
+		iter = NULL;
+		error_btrfs_util(err);
+		goto out;
 	}
 
-	/*
-	 * now we have an rbtree full of root_info objects, but we need to fill
-	 * in their path names within the subvol that is referencing each one.
-	 */
-	n = rb_first(root_lookup);
-	while (n) {
-		struct root_info *entry;
+	for (;;) {
+		struct root_info subvol;
 
-		entry = to_root_info(n);
-		ret = lookup_ino_path(fd, entry);
-		if (ret && ret != -ENOENT)
-			return ret;
-		n = rb_next(n);
+		err = btrfs_util_subvolume_iterator_next_info(iter,
+							      &subvol.path,
+							      &subvol.info);
+		if (err == BTRFS_UTIL_ERROR_STOP_ITERATION) {
+			break;
+		} else if (err) {
+			error_btrfs_util(err);
+			goto out;
+		}
+
+		if (!filter_root(&subvol, filter_set)) {
+			free(subvol.path);
+			continue;
+		}
+
+		if (subvols->num >= capacity) {
+			struct subvol_list *new_subvols;
+			size_t new_capacity = max_t(size_t, 1, capacity * 2);
+
+			new_subvols = realloc(subvols,
+					      sizeof(*new_subvols) +
+					      new_capacity *
+					      sizeof(new_subvols->subvols[0]));
+			if (!new_subvols) {
+				error("out of memory");
+				free(subvol.path);
+				goto out;
+			}
+
+			subvols = new_subvols;
+			capacity = new_capacity;
+		}
+
+		subvols->subvols[subvols->num] = subvol;
+		subvols->num++;
 	}
 
-	return 0;
+	ret = 0;
+out:
+	if (iter)
+		btrfs_util_destroy_subvolume_iterator(iter);
+	if (ret) {
+		free_subvol_list(subvols);
+		subvols = NULL;
+	}
+	return subvols;
 }
 
 static int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 		       struct btrfs_list_comparer_set *comp_set,
-		       enum btrfs_list_layout layout, int full_path)
+		       enum btrfs_list_layout layout)
 {
-	struct rb_root root_lookup;
-	struct rb_root root_sort;
-	int ret = 0;
-	u64 top_id = 0;
+	struct subvol_list *subvols;
 
-	if (full_path) {
-		ret = lookup_path_rootid(fd, &top_id);
-		if (ret) {
-			errno = -ret;
-			error("cannot resolve rootid for path: %m");
-			return ret;
-		}
-	}
+	if (filter_set->only_deleted)
+		subvols = btrfs_list_deleted_subvols(fd, filter_set);
+	else
+		subvols = btrfs_list_subvols(fd, filter_set);
+	if (!subvols)
+		return -1;
 
-	ret = btrfs_list_subvols(fd, &root_lookup);
-	if (ret)
-		return ret;
-	filter_and_sort_subvol(&root_lookup, &root_sort, filter_set,
-				 comp_set, top_id);
+	sort_subvols(comp_set, subvols);
 
-	print_all_subvol_info(&root_sort, layout);
-	rb_free_nodes(&root_lookup, free_root_info);
+	print_all_subvol_info(subvols, layout);
+	free_subvol_list(subvols);
 
 	return 0;
 }
@@ -1555,6 +1094,7 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 	u64 flags = 0;
 	int fd = -1;
 	u64 top_id;
+	char *top_path = NULL;
 	int ret = -1, uerr = 0;
 	char *subvol;
 	bool is_list_all = false;
@@ -1588,9 +1128,7 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 			btrfs_list_setup_print_column(BTRFS_LIST_OGENERATION);
 			break;
 		case 'd':
-			btrfs_list_setup_filter(&filter_set,
-						BTRFS_LIST_FILTER_DELETED,
-						0);
+			filter_set->only_deleted = 1;
 			break;
 		case 'g':
 			btrfs_list_setup_print_column(BTRFS_LIST_GENERATION);
@@ -1686,6 +1224,19 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 		btrfs_list_setup_filter(&filter_set,
 					BTRFS_LIST_FILTER_TOPID_EQUAL,
 					top_id);
+	else if (!filter_set->only_deleted) {
+		enum btrfs_util_error err;
+
+		err = btrfs_util_subvolume_get_path_fd(fd, top_id, &top_path);
+		if (err) {
+			ret = -1;
+			error_btrfs_util(err);
+			goto out;
+		}
+		btrfs_list_setup_filter(&filter_set,
+					BTRFS_LIST_FILTER_REMOVE_PATH_PREFIX,
+					(u64)top_path);
+	}
 
 	/* by default we shall print the following columns*/
 	btrfs_list_setup_print_column(BTRFS_LIST_OBJECTID);
@@ -1696,10 +1247,10 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 	if (bconf.output_format == CMD_FORMAT_JSON)
 		layout = BTRFS_LIST_LAYOUT_JSON;
 
-	ret = btrfs_list_subvols_print(fd, filter_set, comparer_set,
-			layout, !is_list_all && !is_only_in_path);
+	ret = btrfs_list_subvols_print(fd, filter_set, comparer_set, layout);
 
 out:
+	free(top_path);
 	close(fd);
 	if (filter_set)
 		free(filter_set);
-- 
2.45.2

[PATCH 8/8] btrfs-progs: subvol list: add sane -O and -A options

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Now that we've documented the current nonsensical behavior, add a couple
of options that actually make sense: -O lists all subvolumes below a
path (which is what people think -o does), and -A lists all subvolumes
with no path munging (which is what people think the default or -a do).
-O can even be used by unprivileged users.

-O and -A also renames the "top level" in the default output to what it
actually is now: the "parent".

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 Documentation/btrfs-subvolume.rst             | 17 ++++
 cmds/subvolume-list.c                         | 77 +++++++++++++++----
 .../026-subvolume-list-path-filtering/test.sh | 38 +++++++++
 3 files changed, 117 insertions(+), 15 deletions(-)

diff --git a/Documentation/btrfs-subvolume.rst b/Documentation/btrfs-subvolume.rst
index fe84ab1c..f76c1302 100644
--- a/Documentation/btrfs-subvolume.rst
+++ b/Documentation/btrfs-subvolume.rst
@@ -136,6 +136,9 @@ list [options] [-G [\+|-]<value>] [-C [+|-]<value>] [--sort=rootid,gen,ogen,path
         updated every transaction, *parent_ID* is the same as the parent subvolume's id,
         and *path* is the path of the subvolume. The exact meaning of *path*
         depends on the **Path filtering** option used.
+
+        If -O or -A is given, "top level" is replaced by "parent".
+
         The subvolume's ID may be used by the subvolume set-default command,
         or at mount time via the *subvolid=* option.
 
@@ -143,6 +146,20 @@ list [options] [-G [\+|-]<value>] [-C [+|-]<value>] [--sort=rootid,gen,ogen,path
 
         Path filtering:
 
+        -O
+                Print <path> and all subvolumes below it, recursively. <path>
+                must be a subvolume. Paths are printed relative to <path>.
+
+                This may be used by unprivileged users, in which case this only
+                lists subvolumes that the user has access to.
+        -A
+                Print all subvolumes in the filesystem. Paths are printed
+                relative to the root of the filesystem.
+
+        You likely always want either -O or -A. The -o and -a options and the
+        default if no path filtering options are given have very confusing,
+        accidental behavior that is only kept for backwards compatibility.
+
         -o
                 Print only the immediate children subvolumes of the subvolume
                 containing <path>. Paths are printed relative to the root of
diff --git a/cmds/subvolume-list.c b/cmds/subvolume-list.c
index fa5082af..4dde9fbe 100644
--- a/cmds/subvolume-list.c
+++ b/cmds/subvolume-list.c
@@ -59,6 +59,13 @@ static const char * const cmd_subvolume_list_usage[] = {
 	"List subvolumes and snapshots in the filesystem.",
 	"",
 	"Path filtering:",
+	OPTLINE("-O", "print all subvolumes below <path> relative to <path>"),
+	OPTLINE("-A", "print all subvolumes in the filesystem relative to the "
+		"root of the filesystem"),
+	"",
+	"You likely always want either -O or -A. The -o and -a options and the",
+	"default are confusing and only kept for backwards compatibility.",
+	"",
 	OPTLINE("-o", "print only the immediate children subvolumes of the "
 		"subvolume containing <path>"),
 	OPTLINE("-a", "print all subvolumes in the filesystem other than the "
@@ -866,9 +873,11 @@ out:
 	return subvols;
 }
 
-static struct subvol_list *btrfs_list_subvols(int fd,
+static struct subvol_list *btrfs_list_subvols(int fd, bool include_top,
+				bool below,
 				struct btrfs_list_filter_set *filter_set)
 {
+	u64 top_id = below ? 0 : BTRFS_FS_TREE_OBJECTID;
 	struct subvol_list *subvols;
 	size_t capacity = 4;
 	struct btrfs_util_subvolume_iterator *iter;
@@ -883,15 +892,28 @@ static struct subvol_list *btrfs_list_subvols(int fd,
 	}
 	subvols->num = 0;
 
-	err = btrfs_util_create_subvolume_iterator_fd(fd,
-						      BTRFS_FS_TREE_OBJECTID, 0,
-						      &iter);
+	err = btrfs_util_create_subvolume_iterator_fd(fd, top_id, 0, &iter);
 	if (err) {
 		iter = NULL;
 		error_btrfs_util(err);
 		goto out;
 	}
 
+	if (include_top) {
+		err = btrfs_util_subvolume_info_fd(fd, top_id,
+						   &subvols->subvols[0].info);
+		if (err) {
+			error_btrfs_util(err);
+			goto out;
+		}
+		subvols->subvols[0].path = strdup("");
+		if (!subvols->subvols[0].path) {
+			error("out of memory");
+			goto out;
+		}
+		subvols->num++;
+	}
+
 	for (;;) {
 		struct root_info subvol;
 
@@ -945,14 +967,15 @@ out:
 
 static int btrfs_list_subvols_print(int fd, struct btrfs_list_filter_set *filter_set,
 		       struct btrfs_list_comparer_set *comp_set,
-		       enum btrfs_list_layout layout)
+		       enum btrfs_list_layout layout, bool include_top,
+		       bool below)
 {
 	struct subvol_list *subvols;
 
 	if (filter_set->only_deleted)
 		subvols = btrfs_list_deleted_subvols(fd, filter_set);
 	else
-		subvols = btrfs_list_subvols(fd, filter_set);
+		subvols = btrfs_list_subvols(fd, include_top, below, filter_set);
 	if (!subvols)
 		return -1;
 
@@ -1097,8 +1120,10 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 	char *top_path = NULL;
 	int ret = -1, uerr = 0;
 	char *subvol;
+	bool is_list_below = false;
 	bool is_list_all = false;
-	bool is_only_in_path = false;
+	bool is_old_a_option = false;
+	bool is_old_o_option = false;
 	enum btrfs_list_layout layout = BTRFS_LIST_LAYOUT_DEFAULT;
 
 	filter_set = btrfs_list_alloc_filter_set();
@@ -1113,7 +1138,7 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 		};
 
 		c = getopt_long(argc, argv,
-				    "acdgopqsurRG:C:t", long_options, NULL);
+				    "acdgopqsurRG:C:tOA", long_options, NULL);
 		if (c < 0)
 			break;
 
@@ -1122,7 +1147,7 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 			btrfs_list_setup_print_column(BTRFS_LIST_PARENT);
 			break;
 		case 'a':
-			is_list_all = true;
+			is_old_a_option = true;
 			break;
 		case 'c':
 			btrfs_list_setup_print_column(BTRFS_LIST_OGENERATION);
@@ -1134,7 +1159,7 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 			btrfs_list_setup_print_column(BTRFS_LIST_GENERATION);
 			break;
 		case 'o':
-			is_only_in_path = true;
+			is_old_o_option = true;
 			break;
 		case 't':
 			layout = BTRFS_LIST_LAYOUT_TABLE;
@@ -1187,6 +1212,12 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 				goto out;
 			}
 			break;
+		case 'O':
+			is_list_below = true;
+			break;
+		case 'A':
+			is_list_all = true;
+			break;
 
 		default:
 			uerr = 1;
@@ -1197,6 +1228,19 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 	if (check_argc_exact(argc - optind, 1))
 		goto out;
 
+	/*
+	 * The path filtering options and -d don't make sense together. For -O
+	 * and -A, we're strict about not combining them with each other or with
+	 * -o, -a, or -d. The -o, -a, and -d options are older and have never
+	 * been restricted, so although they produce dubious results when
+	 * combined, we allow it for backwards compatibility.
+	 */
+	if (is_list_below + is_list_all +
+	    (is_old_a_option || is_old_o_option || filter_set->only_deleted) > 1) {
+		error("-O, -A, -o, -a, and -d are mutually exclusive");
+		goto out;
+	}
+
 	subvol = argv[optind];
 	fd = btrfs_open_dir(subvol);
 	if (fd < 0) {
@@ -1216,15 +1260,15 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 		goto out;
 	}
 
-	if (is_list_all)
+	if (is_old_a_option)
 		btrfs_list_setup_filter(&filter_set,
 					BTRFS_LIST_FILTER_FULL_PATH,
 					top_id);
-	else if (is_only_in_path)
+	else if (is_old_o_option)
 		btrfs_list_setup_filter(&filter_set,
 					BTRFS_LIST_FILTER_TOPID_EQUAL,
 					top_id);
-	else if (!filter_set->only_deleted) {
+	else if (!is_list_below && !is_list_all && !filter_set->only_deleted) {
 		enum btrfs_util_error err;
 
 		err = btrfs_util_subvolume_get_path_fd(fd, top_id, &top_path);
@@ -1241,13 +1285,16 @@ static int cmd_subvolume_list(const struct cmd_struct *cmd, int argc, char **arg
 	/* by default we shall print the following columns*/
 	btrfs_list_setup_print_column(BTRFS_LIST_OBJECTID);
 	btrfs_list_setup_print_column(BTRFS_LIST_GENERATION);
-	btrfs_list_setup_print_column(BTRFS_LIST_TOP_LEVEL);
+	btrfs_list_setup_print_column(is_list_below || is_list_all ?
+				      BTRFS_LIST_PARENT : BTRFS_LIST_TOP_LEVEL);
 	btrfs_list_setup_print_column(BTRFS_LIST_PATH);
 
 	if (bconf.output_format == CMD_FORMAT_JSON)
 		layout = BTRFS_LIST_LAYOUT_JSON;
 
-	ret = btrfs_list_subvols_print(fd, filter_set, comparer_set, layout);
+	ret = btrfs_list_subvols_print(fd, filter_set, comparer_set, layout,
+				       is_list_below || is_list_all,
+				       is_list_below);
 
 out:
 	free(top_path);
diff --git a/tests/cli-tests/026-subvolume-list-path-filtering/test.sh b/tests/cli-tests/026-subvolume-list-path-filtering/test.sh
index 1b272ddc..a65ba91f 100755
--- a/tests/cli-tests/026-subvolume-list-path-filtering/test.sh
+++ b/tests/cli-tests/026-subvolume-list-path-filtering/test.sh
@@ -114,5 +114,43 @@ EOF
 expect_subvol_list_paths -o a/b/c/d << EOF
 EOF
 
+### -A ###
+
+# Paths are always relative to the root of the filesystem.
+for path in . a/b a/b/c; do
+	expect_subvol_list_paths -A "$path" << EOF
+
+a
+a/b/c
+a/b/c/d
+a/e
+EOF
+done
+
+### -O ###
+
+# Paths are relative to the given path.
+expect_subvol_list_paths -O . << EOF
+
+a
+a/b/c
+a/b/c/d
+a/e
+EOF
+
+expect_subvol_list_paths -O a << EOF
+
+b/c
+b/c/d
+e
+EOF
+
+expect_subvol_list_paths -O a/e << EOF
+
+EOF
+
+run_mustfail "btrfs subvol list -O allowed non-subvolume" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume list -O a/b
+
 cd ..
 run_check_umount_test_dev
-- 
2.45.2

Re: [PATCH 0/8] btrfs-progs: add subvol list options for sane path behavior

[David Sterba]

On Fri, Jun 21, 2024 at 11:53:29AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hello,
> 
> btrfs subvol list's path handling has been a constant source of
> confusion for users. None of -o, -a, or the default do what users
> expect. This has been broken for a decade; see [1].
> 
> This series adds two new options, -O and -A, which do what users
> actually want: list subvolumes below a path, or list all subvolumes,
> with minimal path shenanigans. This approach is conservative and tries
> to maintain backwards compatibility, but it's worth discussing whether
> we should instead fix the existing options/default, or more noisily
> deprecate the existing options.

I'm working on a replacement command of 'subvolume list', there seems to
be no other sane way around that. The command line options are indeed
confusing and the output is maybe easy to parse but not nice to read.
Changing meaning of the options would break too many things as everybody
got used to the bad UI and output.

We can add the two new options but I'd rather do that only in the new
command so we can let everybody migrate there.

> One additional benefit of this is that -O can be used by unprivileged
> users.

This should be the default (and is supposed to be in the new command).

> Patch 1 fixes a libbtrfsutil bug I encountered while testing this.
> Patches 2 and 3 fix libbtrfsutil's privilege checks to work in user
> namespaces. Patches 4 and 5 remove some unused subvol list code. Patch 6
> documents and tests the current, insane behavior. Patch 7 converts
> subvol list to use libbtrfsutil. This is a revival of one of my old
> patches [2], but is much easier now that libbtrfs has been pared down.
> Patch 8 adds the new options, including documentation and tests.
> 
> Thanks!
> Omar
> 
> 1: https://lore.kernel.org/all/bdd9af61-b408-c8d2-6697-84230b0bcf89@gmail.com/
> 2: https://lore.kernel.org/all/6492726d6e89bf792627e4431f7ba7691f09c3d2.1518720598.git.osandov@fb.com/
> 
> Omar Sandoval (8):
>   libbtrfsutil: fix accidentally closing fd passed to subvolume iterator

I've picked this patch now as it's a fix.

Re: [PATCH 0/8] btrfs-progs: add subvol list options for sane path behavior

[Omar Sandoval]

On Tue, Jun 25, 2024 at 05:34:38PM +0200, David Sterba wrote:
> On Fri, Jun 21, 2024 at 11:53:29AM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > Hello,
> > 
> > btrfs subvol list's path handling has been a constant source of
> > confusion for users. None of -o, -a, or the default do what users
> > expect. This has been broken for a decade; see [1].
> > 
> > This series adds two new options, -O and -A, which do what users
> > actually want: list subvolumes below a path, or list all subvolumes,
> > with minimal path shenanigans. This approach is conservative and tries
> > to maintain backwards compatibility, but it's worth discussing whether
> > we should instead fix the existing options/default, or more noisily
> > deprecate the existing options.
> 
> I'm working on a replacement command of 'subvolume list', there seems to
> be no other sane way around that.

I love this idea. Do you have a work in progress anywhere?

> The command line options are indeed
> confusing and the output is maybe easy to parse but not nice to read.
> Changing meaning of the options would break too many things as everybody
> got used to the bad UI and output.
> 
> We can add the two new options but I'd rather do that only in the new
> command so we can let everybody migrate there.
> 
> > One additional benefit of this is that -O can be used by unprivileged
> > users.
> 
> This should be the default (and is supposed to be in the new command).
> 
> > Patch 1 fixes a libbtrfsutil bug I encountered while testing this.
> > Patches 2 and 3 fix libbtrfsutil's privilege checks to work in user
> > namespaces. Patches 4 and 5 remove some unused subvol list code. Patch 6
> > documents and tests the current, insane behavior. Patch 7 converts
> > subvol list to use libbtrfsutil. This is a revival of one of my old
> > patches [2], but is much easier now that libbtrfs has been pared down.
> > Patch 8 adds the new options, including documentation and tests.
> > 
> > Thanks!
> > Omar
> > 
> > 1: https://lore.kernel.org/all/bdd9af61-b408-c8d2-6697-84230b0bcf89@gmail.com/
> > 2: https://lore.kernel.org/all/6492726d6e89bf792627e4431f7ba7691f09c3d2.1518720598.git.osandov@fb.com/
> > 
> > Omar Sandoval (8):
> >   libbtrfsutil: fix accidentally closing fd passed to subvolume iterator
> 
> I've picked this patch now as it's a fix.

Patches 2 and 3 are also fixes, so those would be nice to have, too, if
you don't mind.

My real motivation for this series is so that some internal workloads in
user namespaces can use subvol list. I'd love for the unprivileged use
case to be unblocked in the short term. Would it be better to add just
the -O option without -A and all of the big documentation changes? (This
still requires the libbtrfsutil rework.)

Thanks,
Omar

Re: [PATCH 0/8] btrfs-progs: add subvol list options for sane path behavior

[David Sterba]

On Mon, Jul 01, 2024 at 02:52:02PM -0700, Omar Sandoval wrote:
> On Tue, Jun 25, 2024 at 05:34:38PM +0200, David Sterba wrote:
> > On Fri, Jun 21, 2024 at 11:53:29AM -0700, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > > 
> > > Hello,
> > > 
> > > btrfs subvol list's path handling has been a constant source of
> > > confusion for users. None of -o, -a, or the default do what users
> > > expect. This has been broken for a decade; see [1].
> > > 
> > > This series adds two new options, -O and -A, which do what users
> > > actually want: list subvolumes below a path, or list all subvolumes,
> > > with minimal path shenanigans. This approach is conservative and tries
> > > to maintain backwards compatibility, but it's worth discussing whether
> > > we should instead fix the existing options/default, or more noisily
> > > deprecate the existing options.
> > 
> > I'm working on a replacement command of 'subvolume list', there seems to
> > be no other sane way around that.
> 
> I love this idea. Do you have a work in progress anywhere?

Yes, the initial version is in my branch dev/subvol-list-new but I have
recent updates adding more optional columns. The patches are not
polished, I'll update the branch once it's done.

> > The command line options are indeed
> > confusing and the output is maybe easy to parse but not nice to read.
> > Changing meaning of the options would break too many things as everybody
> > got used to the bad UI and output.
> > 
> > We can add the two new options but I'd rather do that only in the new
> > command so we can let everybody migrate there.
> > 
> > > One additional benefit of this is that -O can be used by unprivileged
> > > users.
> > 
> > This should be the default (and is supposed to be in the new command).
> > 
> > > Patch 1 fixes a libbtrfsutil bug I encountered while testing this.
> > > Patches 2 and 3 fix libbtrfsutil's privilege checks to work in user
> > > namespaces. Patches 4 and 5 remove some unused subvol list code. Patch 6
> > > documents and tests the current, insane behavior. Patch 7 converts
> > > subvol list to use libbtrfsutil. This is a revival of one of my old
> > > patches [2], but is much easier now that libbtrfs has been pared down.
> > > Patch 8 adds the new options, including documentation and tests.
> > > 
> > > Thanks!
> > > Omar
> > > 
> > > 1: https://lore.kernel.org/all/bdd9af61-b408-c8d2-6697-84230b0bcf89@gmail.com/
> > > 2: https://lore.kernel.org/all/6492726d6e89bf792627e4431f7ba7691f09c3d2.1518720598.git.osandov@fb.com/
> > > 
> > > Omar Sandoval (8):
> > >   libbtrfsutil: fix accidentally closing fd passed to subvolume iterator
> > 
> > I've picked this patch now as it's a fix.
> 
> Patches 2 and 3 are also fixes, so those would be nice to have, too, if
> you don't mind.

I picked only the first one for the 6.9.2 release that had little time
for testing.

> My real motivation for this series is so that some internal workloads in
> user namespaces can use subvol list. I'd love for the unprivileged use
> case to be unblocked in the short term. Would it be better to add just
> the -O option without -A and all of the big documentation changes? (This
> still requires the libbtrfsutil rework.)

I think for the time being the saner -O and -A options can be added.
Their functionality resemble the lower case options so it's still
understandable and can be released without delays. For the new command
I'd like to do a public poll and comment round that will take some time.

Re: [PATCH 0/8] btrfs-progs: add subvol list options for sane path behavior

[Omar Sandoval]

On Thu, Jul 04, 2024 at 01:40:54AM +0200, David Sterba wrote:
> On Mon, Jul 01, 2024 at 02:52:02PM -0700, Omar Sandoval wrote:
> > On Tue, Jun 25, 2024 at 05:34:38PM +0200, David Sterba wrote:
> > > On Fri, Jun 21, 2024 at 11:53:29AM -0700, Omar Sandoval wrote:
> > > > From: Omar Sandoval <osandov@fb.com>
> > > > 
> > > > Hello,
> > > > 
> > > > btrfs subvol list's path handling has been a constant source of
> > > > confusion for users. None of -o, -a, or the default do what users
> > > > expect. This has been broken for a decade; see [1].
> > > > 
> > > > This series adds two new options, -O and -A, which do what users
> > > > actually want: list subvolumes below a path, or list all subvolumes,
> > > > with minimal path shenanigans. This approach is conservative and tries
> > > > to maintain backwards compatibility, but it's worth discussing whether
> > > > we should instead fix the existing options/default, or more noisily
> > > > deprecate the existing options.
> > > 
> > > I'm working on a replacement command of 'subvolume list', there seems to
> > > be no other sane way around that.
> > 
> > I love this idea. Do you have a work in progress anywhere?
> 
> Yes, the initial version is in my branch dev/subvol-list-new but I have
> recent updates adding more optional columns. The patches are not
> polished, I'll update the branch once it's done.
> 
> > > The command line options are indeed
> > > confusing and the output is maybe easy to parse but not nice to read.
> > > Changing meaning of the options would break too many things as everybody
> > > got used to the bad UI and output.
> > > 
> > > We can add the two new options but I'd rather do that only in the new
> > > command so we can let everybody migrate there.
> > > 
> > > > One additional benefit of this is that -O can be used by unprivileged
> > > > users.
> > > 
> > > This should be the default (and is supposed to be in the new command).
> > > 
> > > > Patch 1 fixes a libbtrfsutil bug I encountered while testing this.
> > > > Patches 2 and 3 fix libbtrfsutil's privilege checks to work in user
> > > > namespaces. Patches 4 and 5 remove some unused subvol list code. Patch 6
> > > > documents and tests the current, insane behavior. Patch 7 converts
> > > > subvol list to use libbtrfsutil. This is a revival of one of my old
> > > > patches [2], but is much easier now that libbtrfs has been pared down.
> > > > Patch 8 adds the new options, including documentation and tests.
> > > > 
> > > > Thanks!
> > > > Omar
> > > > 
> > > > 1: https://lore.kernel.org/all/bdd9af61-b408-c8d2-6697-84230b0bcf89@gmail.com/
> > > > 2: https://lore.kernel.org/all/6492726d6e89bf792627e4431f7ba7691f09c3d2.1518720598.git.osandov@fb.com/
> > > > 
> > > > Omar Sandoval (8):
> > > >   libbtrfsutil: fix accidentally closing fd passed to subvolume iterator
> > > 
> > > I've picked this patch now as it's a fix.
> > 
> > Patches 2 and 3 are also fixes, so those would be nice to have, too, if
> > you don't mind.
> 
> I picked only the first one for the 6.9.2 release that had little time
> for testing.

Ok, no problem.

> > My real motivation for this series is so that some internal workloads in
> > user namespaces can use subvol list. I'd love for the unprivileged use
> > case to be unblocked in the short term. Would it be better to add just
> > the -O option without -A and all of the big documentation changes? (This
> > still requires the libbtrfsutil rework.)
> 
> I think for the time being the saner -O and -A options can be added.
> Their functionality resemble the lower case options so it's still
> understandable and can be released without delays.

Thanks! Let me know if this series needs any updates.

> For the new command
> I'd like to do a public poll and comment round that will take some time.

That makes total sense. I'm happy to chime in when it's ready.

Thanks,
Omar