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

libspl: declare aok extern in header #9752

Merged
merged 1 commit into from
Dec 26, 2019
Merged

Conversation

dankamongmen
Copy link
Contributor

@dankamongmen dankamongmen commented Dec 20, 2019

Motivation and Context

aok is declared and defined in assert.h. This causes anyone who ends up including libspl's assert.h to pick up a definition of aok, resulting in a situation like:

/usr/bin/ld: src/aggregate.o and src/growlight.o: warning: multiple common of `aok'
/usr/bin/ld: src/crypt.o and src/growlight.o: warning: multiple common of `aok'
/usr/bin/ld: src/nvme.o and src/growlight.o: warning: multiple common of `aok'
/usr/bin/ld: src/stats.o and src/growlight.o: warning: multiple common of `aok'

Maybe this is intended, so that independent files can set their assert settings up independently, but I don't see any kind of use like that within ZoL (and would be questionable IMHO in any case). Instead, I create a single definition in zone.c.

Description

How Has This Been Tested?

Recompiled and reinstalled ZoL on my Debian Unstable machine, and reboooted. All zpools were discovered and filesystems mounted. I then ran make check in a checkout, and it succeeded.

[schwarzgerat](0) $ zpool --version
zfs-0.8.0-1
zfs-kmod-0.8.0-1
[schwarzgerat](0) $ 

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

@PrivatePuffin
Copy link
Contributor

@dankamongmen It would be awesome if you would restore formatting on your PR description.

If you haven't run ztest and zloop yourself, it might be prefered if you submitted it as a draft PR next time. No biggy, but it keeps clear which PR is expected to work and what isn't. :)

Good luck with getting this working 👍

@dankamongmen
Copy link
Contributor Author

@dankamongmen It would be awesome if you would restore formatting on your PR description.

I believe this is done, thanks.

@dankamongmen
Copy link
Contributor Author

Having looked at some of the build failures above, it appears that some of the testing objects need their own definitions of aok, suggesting that they're not getting it from libzfs. I'm looking into this, but would love to hear from someone more informed, thanks!

@PrivatePuffin
Copy link
Contributor

I believe this is done, thanks.

I see, and believe. Thank you!

@PrivatePuffin
Copy link
Contributor

Having looked at some of the build failures above, it appears that some of the testing objects need their own definitions of aok, suggesting that they're not getting it from libzfs. I'm looking into this, but would love to hear from someone more informed, thanks!

I think @jwk404 should be suited to answer this...

@behlendorf behlendorf requested a review from PaulZ-98 December 20, 2019 17:34
@behlendorf
Copy link
Contributor

For additional background information see #9610, but the intended purpose of this variable it so allow zdb to conditionally disable asserts.

I agree, it really should be extern in the header and then declared globally in libspl as you've done. The tricky bit would be that not everything in user space links against libspl, for example libicp only includes some of the headers (hence the build failure). It may be that just lib/libicp/ needs it, in which case adding the full library would be the easiest way to handle this.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 20, 2019
@PaulZ-98
Copy link
Contributor

In addition to the changes above, I also added

diff --git a/tests/zfs-tests/tests/functional/checksum/edonr_test.c b/tests/zfs-tests/tests/functional/checksum/edonr_test.c
index a887560..5e82a85 100644
--- a/tests/zfs-tests/tests/functional/checksum/edonr_test.c
+++ b/tests/zfs-tests/tests/functional/checksum/edonr_test.c
@@ -40,6 +40,7 @@
 #include <sys/time.h>
 #include <sys/stdtypes.h>
 
+int aok;
 /*
  * Test messages from:
  * http://csrc.nist.gov/groups/ST/toolkit/documents/Examples/SHA_All.pdf

to get it built, and the new -A feature still worked in zdb.

@dankamongmen
Copy link
Contributor Author

Looks like most of the tests are passing with this latest, thanks @PaulZ-98 . PTAL.

@codecov
Copy link

codecov bot commented Dec 21, 2019

Codecov Report

Merging #9752 into master will decrease coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #9752    +/-   ##
========================================
- Coverage      80%      79%   -<1%     
========================================
  Files         385      385            
  Lines      121470   121470            
========================================
- Hits        96787    96472   -315     
- Misses      24683    24998   +315
Flag Coverage Δ
#kernel 80% <ø> (ø) ⬇️
#user 67% <ø> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fb2771...8071501. Read the comment docs.

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.

Since it looks like we only have a single exception with the edonr test code I'm good with this. Let's just explicitly initialize the global.

lib/libspl/zone.c Outdated Show resolved Hide resolved
Rather than defining a new instance of 'aok' in every compilation
unit which includes this header, there is a single instance
defined in zone.c, and the header now only declares an extern.

Signed-off-by: Nick Black <[email protected]>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 26, 2019
@behlendorf behlendorf merged commit 8cda5c5 into openzfs:master Dec 26, 2019
allanjude pushed a commit to allanjude/zfs that referenced this pull request Dec 28, 2019
Rather than defining a new instance of 'aok' in every compilation
unit which includes this header, there is a single instance
defined in zone.c, and the header now only declares an extern.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Paul Zuchowski <[email protected]>
Signed-off-by: Nick Black <[email protected]>
Closes openzfs#9752
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jan 2, 2020
Rather than defining a new instance of 'aok' in every compilation
unit which includes this header, there is a single instance
defined in zone.c, and the header now only declares an extern.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Paul Zuchowski <[email protected]>
Signed-off-by: Nick Black <[email protected]>
Closes openzfs#9752
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jan 7, 2020
Rather than defining a new instance of 'aok' in every compilation
unit which includes this header, there is a single instance
defined in zone.c, and the header now only declares an extern.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Paul Zuchowski <[email protected]>
Signed-off-by: Nick Black <[email protected]>
Closes openzfs#9752
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
Rather than defining a new instance of 'aok' in every compilation
unit which includes this header, there is a single instance
defined in zone.c, and the header now only declares an extern.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Paul Zuchowski <[email protected]>
Signed-off-by: Nick Black <[email protected]>
Closes #9752
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