The df command currently allows only one of -l (local file systems only) or -t <fstype, ...> to be used.
This restriction is not necessary and has lead to PR 260921 being created. (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260921)
The patch allows both to be used simultaneously, with -l being an additional restriction if -t is used (i.e. -l -t nodevfs lists all local file systems except for devfs).
Details
Details
Apply patch and verify that all previously valid options are still working as before.
Test combinations of -l and -t and verify that -l restricts the list to local file systems.
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Mentioning the effect of using both -l and -t in df(1) would be good. (Feel free to reuse or draw inspiration from my spec in the PR. Or I can suggest something when I have a moment, but it may be a week or 3.)
Comment Actions
Fix df.c line 190 - it had sneaked in from the unpatched version of this file due to a last second change ...
Comment Actions
Further testing revealed that some the patch does not work correctly, since it uses the makevfslist() function defined in sbin/mount/vfslist.c and that function must not be called more than once. It sets a static variable "skipvfs" depending on the first 2 characters of the passed string and references this variable in subsequent checkvfsname() calls.
A possible solution is the duplication of the 2 functions from vfslist.c in df.c, but with additional explizit skipvfs parameters, it will be offered for review in an updated patch.
Comment Actions
Update the patch to include 2 functions from sbin/mount/vfslist.c to allow them to be invoked more than once.
An alternative to this approach could be to add the extra parameter "int *skip" to the two functions in sbin/mount/vfslist.c and to make sbin/mount and sbin/umount use that function with the modified signature.
Comment Actions
Alternative implementation, based on a modified sbin/mount/vfslist.c.
Functionally identical to the previous patch, but using functions from vfslist.c that have been extended to not use hidden state but an explicit "skip" argument instead.
The functions used by mount and umount call the extended functions with the static variable as skip argument.
The previous patch has the advantage of not modifying files in sbin/mount, but at the cost of source lines from vfslist.c being duplicated (with small modifications) in df.c.
I'm undecided whether the duplication is preferable to the modification of vfslist.c ...
| bin/df/df.1 | ||
|---|---|---|
| 121–123 | IMHO the effect of -l is clear: it further restricts the output beyond the selection made by -t, and it does not matter whether -t was used with a positive or negative selector. It does not make much sense to use "df -t nfs -l" and I did not want to return an error code for that case, it will return with exit code 0 but produce no output. | |
| bin/df/df.1 | ||
|---|---|---|
| 121–123 | That's not what kevans and I settled on for -l and positive -t. See point 2 of https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260921#c0. | |
| bin/df/df.1 | ||
|---|---|---|
| 121–123 | I do not like the semantics suggested in point 2 of PR 260921. IMHO it is not intuitive and I do not see much value in being able to specifically select only local file systems and then add back the remote file systems just removed from the list. But if there is consensus that this is the way it should behave, I can implement it. I'll update the patch, but do still think that "-l" should restrict the output to locally-mounted file systems, not just select a default list of file system types that is then added to or removed from by the parameters of -t. | |
Comment Actions
As requested in point 2 of PR 260291 change the semantics of a combination of -l and -t to make -l select locally-mounted file systems, with the parameter of -t modifying that list (remove entries if used with "no", add to it, else).
Comment Actions
After trying to get this review approved for more than a month including requests sent to the current@ list and individual committers, I'm updating the diff to what I consider the optimal approach.
The previous patch-set extended the functions in sbin/mount/vfslist.c that are used by df.c to have an explicit skipvfs flag.
While that patch resulted in less lines of code changing, it did affect files in the separate directory sbin/mount.
This patch-set contains copies of the two functions that were referenced in vfslist.c, extended to have the explicit skipvfs reference.
By using local copies, the .PATH directive and other references to the files in sbin/mount can be removed from the Makefile, untangling this port from sbin/mount.
Comment Actions
On the whole I like this better. I didn't dislike the prior version, so let me explain.
This keeps the code within the same file, so it's easier to understand. I like that.
It does, however, copy code from elsewhere that could be shared. I don't like that.
The code copied, though, is small, and keeps df and umount decoupled, so I like the decoupling.
If done from scratch, it would have been better to have different libraries that df, mount, umount, etc used, but that would be a much bigger change.
So, on the whole, this is less bad than the prior patches and looks good to my eye. Though often
the filesystem type is either local or remote so I'm not sure of the point of doing this... But that's
not enough for me to object to what's proffered here.
Comment Actions
Thanks a lot for the review and approval of the patch-set!
I'm updating the patch a final time to fix style issues I noticed preparing the commit.
This patch contains a few white-space adjustments, a move of the 2 functions copied from vfslist.c to a more appropriate place, and forward declarations of these functions.
I did not want to change the indentation of all function declarations to account for the extra space required for the "static const char **" return type of makevfslist() - not sure whether this was the best choice, though.
I'll adjust the white-space in a follow-up commit if asked to ...
Since the code is functionality unchanged and contains style fixes, I assume that the approval is carried over to this updated version of the patch.