Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make userspace libs more threadsafe #15793

Closed
wants to merge 2 commits into from
Closed

Make userspace libs more threadsafe #15793

wants to merge 2 commits into from

Conversation

rkojedzinszky
Copy link
Contributor

@rkojedzinszky rkojedzinszky commented Jan 19, 2024 •
edited
Loading

Motivation and Context

Improves the userspace lib to be threadsafe.

Description

Replaced non-threadsafe functions with their threadsafe counterparts in lib/.

How Has This Been Tested?

Compiled the code on linux/freebsd.

Compiled on Debian bookworm, FreeBSD 13.2.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@rkojedzinszky rkojedzinszky mentioned this pull request Jan 19, 2024
Open
@rkojedzinszky rkojedzinszky force-pushed the threadsafe-lib branch 2 times, most recently from 706ac95 to 085f500 Compare January 19, 2024 21:56
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jan 19, 2024
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 26, 2024
@rkojedzinszky
Copy link
Contributor Author

Thanks for the reviews! Do anyone has an idea on how to prevent future modifications to lib/ which would break thread safety? How to detect calls to libc functions that are not threadsafe, or how to detect non thread-local global variable usage?

@behlendorf
Copy link
Contributor

One idea would be to add a codeql checker for any libc functions which are not threadsafe. This way any PRs attempting to use these functions will be caught by the CI. We already do something similar for deprecated functions we want to keep out of the code base. That of course won't handle everything, but it's a start.

https://github.com/openzfs/zfs/blob/master/.github/codeql/custom-queries/cpp/deprecatedFunctionUsage.ql

behlendorf pushed a commit that referenced this pull request Jan 29, 2024
@rkojedzinszky @behlendorf
libzfs: use zfs_strerror() in place of strerror()
401c356 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tino Reichardt <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes #15793
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@rkojedzinszky @lundman
libzfs: make userquota_propname_decode threadsafe
b59b39c 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tino Reichardt <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@rkojedzinszky @lundman
libzfs: use zfs_strerror() in place of strerror()
74c846c 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tino Reichardt <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@rkojedzinszky @lundman
libzfs: make userquota_propname_decode threadsafe
153264b 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tino Reichardt <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
@rkojedzinszky @lundman
libzfs: use zfs_strerror() in place of strerror()
94fc8f3 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tino Reichardt <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
@yocalebo yocalebo mentioned this pull request Sep 6, 2024
Closed
13 tasks
ptr1337 pushed a commit to CachyOS/zfs that referenced this pull request Nov 14, 2024
@rkojedzinszky @tonyhutter
libzfs: use zfs_strerror() in place of strerror()
a1ea40a 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tino Reichardt <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
@amotin amotin mentioned this pull request Jan 2, 2025
Closed
13 tasks
rkojedzinszky added a commit to rkojedzinszky/zfs that referenced this pull request Jan 3, 2025
@rkojedzinszky
fix: make zfs_strerror really thread-safe and portable
29e1aaf 
Fixes openzfs#15793
Fixes openzfs#16640
rkojedzinszky added a commit to rkojedzinszky/zfs that referenced this pull request Jan 3, 2025
@rkojedzinszky
fix: make zfs_strerror really thread-safe and portable
675724c 
Fixes openzfs#15793
Fixes openzfs#16640
rkojedzinszky added a commit to rkojedzinszky/zfs that referenced this pull request Jan 3, 2025
@rkojedzinszky
fix: make zfs_strerror really thread-safe and portable
07ad792 
Fixes openzfs#15793
Fixes openzfs#16640

Signed-off-by: Richard Kojedzinszky <[email protected]>
@rkojedzinszky rkojedzinszky mentioned this pull request Jan 3, 2025
Merged
13 tasks
rkojedzinszky added a commit to rkojedzinszky/zfs that referenced this pull request Jan 3, 2025
@rkojedzinszky
fix: make zfs_strerror really thread-safe and portable
cfb5106 
Fixes openzfs#15793
Fixes openzfs#16640

Signed-off-by: Richard Kojedzinszky <[email protected]>
behlendorf pushed a commit that referenced this pull request Jan 4, 2025
@rkojedzinszky
fix: make zfs_strerror really thread-safe and portable
dc0324b 
#15793 wanted to make zfs_strerror threadsafe, unfortunately, it
turned out that strerror_l() usage was wrong, and also, some libc 
implementations dont have strerror_l().

zfs_strerror() now simply calls original strerror() and copies the 
result to a thread-local buffer, then returns that.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes #15793
Closes #16640
Closes #16923
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 4, 2025
@rkojedzinszky @behlendorf
fix: make zfs_strerror really thread-safe and portable
5ba50c8 
openzfs#15793 wanted to make zfs_strerror threadsafe, unfortunately, it
turned out that strerror_l() usage was wrong, and also, some libc 
implementations dont have strerror_l().

zfs_strerror() now simply calls original strerror() and copies the 
result to a thread-local buffer, then returns that.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
Closes openzfs#16640
Closes openzfs#16923
rkojedzinszky added a commit to dravanet/openzfs-zfs that referenced this pull request Jan 5, 2025
@rkojedzinszky
fix: make zfs_strerror really thread-safe and portable
5b402c2 
openzfs#15793 wanted to make zfs_strerror threadsafe, unfortunately, it
turned out that strerror_l() usage was wrong, and also, some libc 
implementations dont have strerror_l().

zfs_strerror() now simply calls original strerror() and copies the 
result to a thread-local buffer, then returns that.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
Closes openzfs#16640
Closes openzfs#16923
rkojedzinszky added a commit to dravanet/openzfs-zfs that referenced this pull request Jan 6, 2025
@rkojedzinszky
fix: make zfs_strerror really thread-safe and portable
e6a975a 
turned out that strerror_l() usage was wrong, and also, some libc
implementations dont have strerror_l().

zfs_strerror() now simply calls original strerror() and copies the
result to a thread-local buffer, then returns that.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
Closes openzfs#16640
Closes openzfs#16923

Upstream commit dc0324b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmilk mcmilk mcmilk approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rkojedzinszky @behlendorf @mcmilk