Bug 260921 - /bin/df: make -l and -t nonexclusive
Summary: /bin/df: make -l and -t nonexclusive
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Stefan Eßer
URL: https://reviews.freebsd.org/D33748
Keywords: feature
Depends on:
Blocks:
 
Reported: 2022-01-04 12:19 UTC by Pau Amma
Modified: 2022-04-18 11:32 UTC (History)
2 users (show)

See Also:


Attachments
Allow -l and -t to be used together (3.51 KB, patch)
2022-01-04 13:12 UTC, Stefan Eßer
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pau Amma 2022-01-04 12:19:59 UTC
df currently rejects using both -l and -t, thus making it impossible to ask for free space on some but not all local filesystems as the sysinfo plugin in irc/hexchat tries to.

Following an IRC discussion with kevans, the following tentative specification emerged:

1- make it possible to use -l and -t nofoo together to select all local filesystems except those of type foo.
2- (suggested by kevans) make it possible to use -l and -t foo together to select all local filesystems and those of type foo (eg, local and NFS but not Samba).

kevans also suggested allowing multiple -t options, both additive and subtractive) but I think the meaning of that needs more discussion (and it arguably breaks the POLA) so I'm not including it in this change request.
Comment 1 Stefan Eßer freebsd_committer freebsd_triage 2022-01-04 13:12:49 UTC
Created attachment 230685 [details]
Allow -l and -t to be used together

The attached patch allows -l and -t to both be specified.

It is tested to work, but maybe the desired semantics need to be better specified.

The patch makes -l and additional and independent condition that can be applied to the list generated by -t <type,...>.

An alternative concept would be to allow for multiple -t options and to make each operate on the list of file systems selected at this point.

The traditional behavior of e.g. "df -t notmpfs,noufs" would be to list all file systems, since a set union is performed for each parameters of -t (i.e. all file systems that are not tmpfs are added to the list, then all that are not ufs ...).

With multiple -t options allowed, -l could just be equivalent to the list of all local file systems specified with -l. In that case, a later "noufs" could delete file systems from the list (and a -l would delete all currently in the list that are not local).

But that would be a much more complex change and not that useful since it is not expected to work by currently existing scripts.
Comment 2 Stefan Eßer freebsd_committer freebsd_triage 2022-01-04 13:17:25 UTC
I have just noticed that my description of the semantics of "df -t notmpfs,noufs"  in the previous comment was wrong.

Only the first "no" is parsed to switch to "exclude the following types" mode.
The correct option to display no tmpfs and no ufs file systems therefore is "df -t notmpfs,ufs".
Comment 3 Pau Amma 2022-01-05 00:25:42 UTC
(In reply to Stefan Eßer from comment #1)
Thanks. That was fast. If you need me to test it more before committing, I can do that but it may be a week or 3 until I have time to install a -current snapshot in a VM, and I may (likely will) need some handholding to apply the patch and compile and install the changed df. (If my spec needs clarification or elaboration, let me know as well.)
Comment 4 Stefan Eßer freebsd_committer freebsd_triage 2022-01-05 00:44:21 UTC
The patch is tested on -CURRENT and should cleanly apply to 13-STABLE, if you happen to use that. I did not check whether the df.c file in 12-STABLE differs from the one in the newer versions, though.

I do not think that any further testing is needed, since the modifications are quite trivial, and I have tested it on my system with a large number of different file system types (including non-local file systems).

The only question is, whether this implementation offers all features that are required, or whether a more complex solution that supports multiple -t options and possibly even --exclude-type options should be implemented.

The behavior with multiple -t options is unchanged (i.e. the last one wins).
Comment 5 Stefan Eßer freebsd_committer freebsd_triage 2022-01-05 01:00:50 UTC
I have created a Phabricator review https://reviews.freebsd.org/D33748 based on a slightly modified patch.

I had mistakenly assumed that multiple -t options could be used before, but in fact I had deleted the protection against multiple -t options together with the protection against use of -l with -t.
Comment 6 commit-hook freebsd_committer freebsd_triage 2022-02-10 20:31:01 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f0fd4a32c4f7ab5eb008ab08a1a13cb20d4b30d2

commit f0fd4a32c4f7ab5eb008ab08a1a13cb20d4b30d2
Author:     Stefan Eßer <se@FreeBSD.org>
AuthorDate: 2022-02-10 20:09:34 +0000
Commit:     Stefan Eßer <se@FreeBSD.org>
CommitDate: 2022-02-10 20:09:34 +0000

    bin/df: allow -t option to be used together with -l

    The df command provides a -l option to exclude all non-local file
    systems and a -t option with a (positive or negative) list of file
    system types to display.

    This commit adds support for a combination of -l and -t. If both are
    specified, the parameter list of the -t option is applied on top of
    the selection of öocal file systems (independently of the order of
    the -l and -t options).

    E.g., "df -t noprocfs,sysfs -l" will select all local file systems
    except those of type procfs and sysfs.

    PR:             260921
    Approved by:    imp
    Relnotes:       yes
    MFC after:      1 month
    Differential Revision:  https://reviews.freebsd.org/D33748

 bin/df/Makefile |   6 +---
 bin/df/df.1     |  16 +++++++--
 bin/df/df.c     | 109 +++++++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 103 insertions(+), 28 deletions(-)
Comment 7 Pau Amma 2022-03-30 10:49:57 UTC
(In reply to commit-hook from comment #6)
MFC'd to stable/13 in https://cgit.freebsd.org/src/commit/?h=stable/13&id=c888fc2d97ee3e25269ca8218f80769bca930705. No MFC to stable/12 (yet). Closable if there won't be one.
Comment 8 Pau Amma 2022-04-17 03:41:19 UTC
(In reply to PauAmma from comment #7)

Was the MFC supposed be to 13-stable only and not 12-stable?
Comment 9 Stefan Eßer freebsd_committer freebsd_triage 2022-04-17 11:12:12 UTC
(In reply to PauAmma from comment #8)

I did not expect this change to be relevant for 12-STABLE, under the assumptions, that users of that version are not going to change their scripts and procedures to take advantage of the feature.

If there is interest (and your comment implies there is ...), I'll merge the change to 12-STABLE ...
Comment 10 Pau Amma 2022-04-18 02:38:14 UTC
(In reply to Stefan Eßer from comment #9)
It may not be - I have no opinion on this point and no basis for one. I just need to know, both so this bug can be closed if work on it is complete including all MFCs and so the ports bug that led to it (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260296) can be fixed for all supported FreeBSD versions. (Fixing it the same way for 12.3 and 13.1 would be nice IMO, but that may not be a compelling enough reason to MFC to 12. I'll wait for re@ to weigh in when they have the time if they see fit.)
Comment 11 Stefan Eßer freebsd_committer freebsd_triage 2022-04-18 11:32:18 UTC
(In reply to PauAmma from comment #10)
There have been 3 unrelated changes by other authors before and 1 commit after the commit to -CURRENT for this feature request.

I have committed 2 older changes to reduce the differences to HEAD and then the support for the combination of -d and -t on top of them.

1 commit removed support for listing of unmounted file systems, and it had not been MFCed to 12 - I have therefore omitted that commit, too.

Another newer commit (2eee44bd5e by bapt) could be cleanly applied on top of the sources in 12/stable, but it is only for a cosmetic feature (suppress 100% full display for synthetic file systems).

I'm going to close the PR now that all relevant FreeBSD versions have been patched (I have forgotten to add the PR number to the commit message to let the PR be automatically closed).