| Message ID | cover.1718995160.git.osandov@fb.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs-progs: add subvol list options for sane path behavior | expand |
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.
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
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.
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
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