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

Add VERIFY0P() and ASSERT0P() macros #15225

Closed
wants to merge 3 commits into from
Closed

Add VERIFY0P() and ASSERT0P() macros #15225

wants to merge 3 commits into from

Conversation

dag-erling
Copy link
Contributor

@dag-erling dag-erling commented Aug 30, 2023 •
edited
Loading

Motivation and Context

As pointed out in #15224, ASSERT0() cannot be safely used to check a pointer.

Description

This pull request adds VERIFY0P() and ASSERT0P() macros which can safely be used to verify that a pointer is null. It also addresses some code quality issues in the existing VERIFY*() macros.

How Has This Been Tested?

The patch has been tested in a FreeBSD 15 tree.

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:

amotin
amotin reviewed Aug 30, 2023
View reviewed changes
include/os/linux/spl/sys/debug.h Show resolved Hide resolved
lib/libspl/include/assert.h Outdated Show resolved Hide resolved
lib/libspl/include/assert.h Show resolved Hide resolved
@dag-erling dag-erling force-pushed the des/assert0p branch 4 times, most recently from 4b2880a to a80731c Compare August 30, 2023 15:13
amotin
amotin reviewed Aug 30, 2023
View reviewed changes
include/os/freebsd/spl/sys/debug.h Show resolved Hide resolved
lib/libspl/include/assert.h Outdated Show resolved Hide resolved
lib/libspl/include/assert.h Show resolved Hide resolved
include/os/linux/spl/sys/debug.h Show resolved Hide resolved
@dag-erling dag-erling marked this pull request as ready for review August 30, 2023 16:09
oromenahar
oromenahar approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@oromenahar oromenahar left a comment

Choose a reason for hiding this comment

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

check out this as well: #15224 module/zfs/dbuf.c both solution would work, I would prefer the other solution. But nice work.

@dag-erling
Clean up existing VERIFY*() macros.
c9c8599 
Chiefly:

- Remove unnecessary parentheses around variable names.
- Remove spaces between the type and variable in casts.
- Make the panic message for VERIFY0() reflect how the macro is used.
- Use %p to format pointers, except in Linux kernel code.

Signed-off-by: Dag-Erling Smørgrav <[email protected]>
@dag-erling
Add VERIFY0P() and ASSERT0P() macros.
21699b5 
These macros are similar to VERIFY0() and ASSERT0() but are intended
for pointers, and therefore use uintptr_t instead of int64_t.

Signed-off-by: Dag-Erling Smørgrav <[email protected]>
@dag-erling
Use ASSERT0P() to check that a pointer is NULL.
1b33d90 
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
@dag-erling
Copy link
Contributor Author

@behlendorf could you take a look at this? a better solution for the problem fixed by #15224 plus some additional cleanup and bugfixes for the ASSERT and VERIFY macros.

behlendorf
behlendorf approved these changes Sep 20, 2023
View reviewed changes
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 good to me. Thanks for the cleanup and improvement!

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Sep 20, 2023
behlendorf pushed a commit that referenced this pull request Sep 20, 2023
@dag-erling @behlendorf
Add VERIFY0P() and ASSERT0P() macros.
506fe78 
These macros are similar to VERIFY0() and ASSERT0() but are intended
for pointers, and therefore use uintptr_t instead of int64_t.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes #15225
behlendorf pushed a commit that referenced this pull request Sep 20, 2023
@dag-erling @behlendorf
Use ASSERT0P() to check that a pointer is NULL.
5f1479d 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes #15225
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@dag-erling @lundman
Clean up existing VERIFY*() macros.
b363d08 
Chiefly:

- Remove unnecessary parentheses around variable names.
- Remove spaces between the type and variable in casts.
- Make the panic message for VERIFY0() reflect how the macro is used.
- Use %p to format pointers, except in Linux kernel code.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes openzfs#15225
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@dag-erling @lundman
Add VERIFY0P() and ASSERT0P() macros.
860a3a1 
These macros are similar to VERIFY0() and ASSERT0() but are intended
for pointers, and therefore use uintptr_t instead of int64_t.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes openzfs#15225
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@dag-erling @lundman
Use ASSERT0P() to check that a pointer is NULL.
0236426 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes openzfs#15225
ixhamza pushed a commit to truenas/zfs that referenced this pull request Dec 13, 2023
@dag-erling @ixhamza
Add VERIFY0P() and ASSERT0P() macros.
d44f561 
These macros are similar to VERIFY0() and ASSERT0() but are intended
for pointers, and therefore use uintptr_t instead of int64_t.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes openzfs#15225
ixhamza pushed a commit to truenas/zfs that referenced this pull request Dec 13, 2023
@dag-erling @ixhamza
Add VERIFY0P() and ASSERT0P() macros.
03255b0 
These macros are similar to VERIFY0() and ASSERT0() but are intended
for pointers, and therefore use uintptr_t instead of int64_t.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes openzfs#15225
ixhamza pushed a commit to truenas/zfs that referenced this pull request Dec 13, 2023
@dag-erling @ixhamza
Add VERIFY0P() and ASSERT0P() macros.
4b8ff95 
These macros are similar to VERIFY0() and ASSERT0() but are intended
for pointers, and therefore use uintptr_t instead of int64_t.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes openzfs#15225
dag-erling referenced this pull request Mar 18, 2024
@amotin @behlendorf
dbuf: Set dr_data when unoverriding after clone
9c40ae0 
Block cloning normally creates dirty record without dr_data.  But if
the block is read after cloning, it is moved into DB_CACHED state and
receives the data buffer.  If after that we call dbuf_unoverride()
to convert the dirty record into normal write, we should give it the
data buffer from dbuf and release one.

Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Alexander Motin <[email protected]>
Sponsored by: iXsystems, Inc.
Closes #15654
Closes #15656
tonyhutter pushed a commit that referenced this pull request May 2, 2024
@dag-erling @tonyhutter
Clean up existing VERIFY*() macros.
d6da6cb 
Chiefly:

- Remove unnecessary parentheses around variable names.
- Remove spaces between the type and variable in casts.
- Make the panic message for VERIFY0() reflect how the macro is used.
- Use %p to format pointers, except in Linux kernel code.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes #15225
tonyhutter pushed a commit that referenced this pull request May 2, 2024
@dag-erling @tonyhutter
Add VERIFY0P() and ASSERT0P() macros.
5dda8c0 
These macros are similar to VERIFY0() and ASSERT0() but are intended
for pointers, and therefore use uintptr_t instead of int64_t.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes #15225
tonyhutter pushed a commit that referenced this pull request May 2, 2024
@dag-erling @tonyhutter
Use ASSERT0P() to check that a pointer is NULL.
5972bb8 
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Kay Pedersen <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dag-Erling Smørgrav <[email protected]>
Closes #15225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oromenahar oromenahar oromenahar approved these changes

@amotin amotin amotin 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.
4 participants
@dag-erling @behlendorf @amotin @oromenahar