Skip to content

FreeBSD: Use new SYSCTL_SIZEOF() #17309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 8, 2025
Merged

FreeBSD: Use new SYSCTL_SIZEOF() #17309

merged 1 commit into from
May 8, 2025

Conversation

OlCe2
Copy link
Contributor

@OlCe2 OlCe2 commented May 7, 2025

SYSCTL_SIZEOF() has been introduced in FreeBSD by commit "sysctl(9): Ease exporting struct sizes; Discourage doing that" (713abc9880aa) in branch 'main', and is the preferred way to create leaves under the debug.sizeof sysctl node.

This is simply a FreeBSD-specific code update to use it, where available, with backward compatibility (which is needed as long as stable/13 is supported, so April 30, 2026 in theory).

SYSCTL_SIZEOF() has been introduced in FreeBSD by commit "sysctl(9):
Ease exporting struct sizes; Discourage doing that" (713abc9880aa) in
branch 'main'.  It will soon be backported to 'stable/14'.  We will thus
be able to remove the old, alternate version left in the '#else' branch
as soon as 'stable/13' goes out of support (April 30, 2026).

Sponsored-by:   The FreeBSD Foundation
Signed-off-by:  Olivier Certner <[email protected]>
@OlCe2
Copy link
Contributor Author

OlCe2 commented May 7, 2025

While here, could you add a line for me in .mailmap, section "# Signed-off-by: overriding Author:":

?

Thanks!

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is somewhat surprising to see an improvement for a discouraged API, but since it is already in FreeBSD, sure.

@amotin amotin added the Status: Code Review Needed Ready for review and testing label May 7, 2025
@OlCe2
Copy link
Contributor Author

OlCe2 commented May 7, 2025

It is somewhat surprising to see an improvement for a discouraged API, but since it is already in FreeBSD, sure.

I understand... :-) This started out as code factorization for adding more uses, but then it appeared we rather wanted to limit the use of debug.sizeof, as there are easy means for developers to get this information, and in general we don't want to expose KBI that are allowed to change.

In fact, it's not the API that's discouraged, but its use for such purposes. However, there are still legitimate temporary use cases, such as what ZFS is doing here (communicating the size as a kind of KBI versioning for userland), pending designing a better interface (which we all know sometimes never comes... or very late).

Here's for the rationale.

@amotin amotin added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 8, 2025
@amotin amotin merged commit 78628a5 into openzfs:master May 8, 2025
23 of 24 checks passed
@OlCe2
Copy link
Contributor Author

OlCe2 commented May 8, 2025

@amotin Thanks!

While here, if you could add a .mailmap line for me, as explained above, that would be great.

@amotin
Copy link
Member

amotin commented May 8, 2025

@OlCe2 Lately it was updated by @robn, I have never touched it. But I am not sure there is some special magic, so it would need a PR as any other commit.

@OlCe2
Copy link
Contributor Author

OlCe2 commented May 8, 2025

Ok, I'll do that.

@robn
Copy link
Member

robn commented May 8, 2025

@OlCe2 I'm doing an AUTHORS update PR right now. I can take care of it there if you like, no problem.

(I know you said mailmap, it's all part of the same thing)

@OlCe2
Copy link
Contributor Author

OlCe2 commented May 12, 2025

@robn Oh, great! It seems it has fallen on you to do these kinds of updates recently. :-) The change in #17316 for my address is fine. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants