Details

Reviewers

Commits

rG6bce3f23d0aa: vfs: Export get_next_dirent() as vn_dir_next_dirent()

Summary

Move internal-to-'vfs_default.c' get_next_dirent() to 'vfs_vnops.c' and export
it for use by other parts of the VFS. This is a preparatory change for using it
in vfs_emptydir().

No functional change.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

olce created this revision.Apr 21 2023, 9:14 PM

Herald added a subscriber: imp. · View Herald TranscriptApr 21 2023, 9:14 PM

olce requested review of this revision.Apr 21 2023, 9:14 PM

Harbormaster completed remote builds in B51110: Diff 120838.Apr 21 2023, 9:14 PM

freebsd_igalic.co added a subscriber: freebsd_igalic.co.Apr 21 2023, 9:52 PM

The patch definitely needs a split into several. An immediate candidate is the code refactoring without a change in behavior, then introduce the fixes on top of it.

sys/kern/vfs_subr.c
6416 ↗	(On Diff #120838)	The args should start on the previous line
6423 ↗	(On Diff #120838)	int_fast16_t is not fast. I do not see why not use plain int there.
sys/sys/dirent.h
129 ↗	(On Diff #120838)	Static_assert should not be used in app-visible headers, there is no point to add such asserts to the third party code. Placing them in kernel sources should be enough.

… no reviewers specified.

To raise awareness: https://lists.freebsd.org/archives/freebsd-fs/2023-April/002153.html

Keep just the get_next_dirent() => vfs_next_dirent() refactoring

And update according to feedback.

Harbormaster completed remote builds in B51120: Diff 120856.Apr 22 2023, 4:28 PM

olce marked 3 inline comments as done.Apr 22 2023, 4:30 PM

I've also changed a bit the initial diff so as to minimize differences (order of declarations).

(@kib You said that uint_fast16_t is not fast. While I understand the requested change from an aesthetic point of view, I don't think uint_fast16_t is not fast, it is in fact as fast as int, since it expands to __uint32_t. But yes, it's not faster than an int, which you probably meant?)

Change summary

Harbormaster completed remote builds in B51129: Diff 120876.Apr 22 2023, 9:23 PM

olce added a child revision: D39764: vfs: Simplify and harden vn_dir_next_dirent().Apr 22 2023, 9:28 PM

Can we split it even more? The move of vfs_next_dirent() and its rename, then the changes you do over the moved code.
I expect the changes would be easier to grasp this way.

sys/kern/vfs_subr.c
6421 ↗	(On Diff #120876)	Call it vn_dir_next_dirent() and put at the end of vfs_vnops.c instead of vfs_subr.c.
6517 ↗	(On Diff #120876)	The blank line is not needed
sys/sys/dirent.h
68 ↗	(On Diff #120876)	This should be a separate change. Please mail me a git format-patch with proper commit message and metadata (mostly the author name/mail) correct.