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.

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

FreeBSD: Clean up ASSERT/VERIFY use in module #11971

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 29, 2021

Motivation and Context

I wanted to improve a few asserts to use typed variants for easier debugging, and then ended up cleaning up all of module/os/freebsd.

The typed assert/verify are more helpful because they show the values that failed, rather than just saying the checks failed.

This work has been done in the process of implementing xattr=sa support for FreeBSD.

Description

Convert use of ASSERT() to ASSERT0(), ASSERT3U(), ASSERT3S(), ASSERT3P(), and likewise for VERIFY().
In some cases it ended up making more sense to change the code, such as VERIFY on nvlist operations that I have converted to use fnvlist instead.
In one place I changed an internal struct member from int to boolean_t to match its use.
Some asserts that combined multiple checks with && in a single assert have been split to separate asserts, to make it apparent which check fails.

How Has This Been Tested?

Ran ZTS on FreeBSD

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:

@ghost ghost added the Status: Code Review Needed Ready for review and testing label Apr 29, 2021
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Looks like you could have used ASSERT0() in a few of places but that's just a matter of preference so this LGTM! I know it's tedious but I wouldn't mind seeing this kind of mechanical cleanup applied to other areas of the code.

@ghost
Copy link
Author

ghost commented Apr 29, 2021

Looks like something went awry. I had tested this along with a batch of other commits, I'll have to take another look at what ended up in the PR. I'm going to have to rebase this after the the other PRs are merged, too.

@ghost ghost force-pushed the better-asserts branch from a3ed94f to 085d5d7 Compare April 29, 2021 23:05
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
@ghost ghost force-pushed the better-asserts branch from 085d5d7 to 3c839f5 Compare April 30, 2021 15:17
@ghost
Copy link
Author

ghost commented Apr 30, 2021

  • Fixed inverted NULL check
  • Rebased

@behlendorf behlendorf merged commit e4efb70 into openzfs:master Apr 30, 2021
@ghost ghost deleted the better-asserts branch May 1, 2021 01:52
ghost pushed a commit to truenas/zfs that referenced this pull request May 14, 2021
Convert use of ASSERT() to ASSERT0(), ASSERT3U(), ASSERT3S(), 
ASSERT3P(), and likewise for VERIFY().  In some cases it ended up 
making more sense to change the code, such as VERIFY on nvlist 
operations that I have converted to use fnvlist instead.  In one 
place I changed an internal struct member from int to boolean_t to 
match its use.  Some asserts that combined multiple checks with && 
in a single assert have been split to separate asserts, to make it 
apparent which check fails.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11971
ghost pushed a commit to truenas/zfs that referenced this pull request May 27, 2021
Convert use of ASSERT() to ASSERT0(), ASSERT3U(), ASSERT3S(), 
ASSERT3P(), and likewise for VERIFY().  In some cases it ended up 
making more sense to change the code, such as VERIFY on nvlist 
operations that I have converted to use fnvlist instead.  In one 
place I changed an internal struct member from int to boolean_t to 
match its use.  Some asserts that combined multiple checks with && 
in a single assert have been split to separate asserts, to make it 
apparent which check fails.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11971
ghost pushed a commit to truenas/zfs that referenced this pull request May 28, 2021
Convert use of ASSERT() to ASSERT0(), ASSERT3U(), ASSERT3S(), 
ASSERT3P(), and likewise for VERIFY().  In some cases it ended up 
making more sense to change the code, such as VERIFY on nvlist 
operations that I have converted to use fnvlist instead.  In one 
place I changed an internal struct member from int to boolean_t to 
match its use.  Some asserts that combined multiple checks with && 
in a single assert have been split to separate asserts, to make it 
apparent which check fails.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11971
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 15, 2021
Convert use of ASSERT() to ASSERT0(), ASSERT3U(), ASSERT3S(),
ASSERT3P(), and likewise for VERIFY().  In some cases it ended up
making more sense to change the code, such as VERIFY on nvlist
operations that I have converted to use fnvlist instead.  In one
place I changed an internal struct member from int to boolean_t to
match its use.  Some asserts that combined multiple checks with &&
in a single assert have been split to separate asserts, to make it
apparent which check fails.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#11971
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant