Enable -Wmissing-prototypes #10470

nivedita76 · 2020-06-17T00:03:08Z

These changes are done to allow for enabling -Wmissing-prototypes (and -Wstrict-prototypes). It marks a bunch of functions static, removes dead code, and adds prototypes for what's left if they were not there before.

Motivation and Context

The original issue is fixed in the first patch of the series: libzpool/kernel.c had stub definitions for a couple of functions with bad prototypes, which wasn't caught because -Wmissing-prototypes isn't enabled. This series will help us avoid such issues in the future.

Description

The patch adding static should be fairly safe. The dead code removal patch should get some detailed review as we may want to retain some of these functions for debugging or in some cases to reduce divergence from wherever the code was originally pulled from. Eg there may be functions that would only be useful on Solaris that got deleted.

How Has This Been Tested?

Build-tested on linux/FreeBSD x86-64.

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:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

Commit ec21397 ("async zvol minor node creation interferes with receive") replaced zvol_create_minors with zvol_create_minor and zvol_create_minors_recursive, changing the prototype at the same time. However the stub functions in libzpool/kernel.c were defined with the old prototype. As the definitions are empty, this doesn't cause any runtime issues, but an LTO build shows warnings because of the mismatched prototypes. Commit a0bd735 ("Add support for asynchronous zvol minor operations") removed the real zvol_remove_minor, but for some reason added a stub implementation in libzpool/kernel.c with no references. Delete this dead code. Commit 196bee4 ("Remove deduplicated send/receive code") removed zfs_onexit_del_cb and zfs_onexit_cb_data. Drop the stubs as well. Add zvol.h include to provide prototypes, and sort the include directives. Fixes: ec21397 ("async zvol minor node creation interferes with receive") Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

ghost · 2020-06-17T00:50:18Z

I would expect for zfs_context.h being included before other headers to be generally important.
Most of this looks good, let's get it building now.

codecov · 2020-06-17T06:26:44Z

Codecov Report

Merging #10470 into master will decrease coverage by 13.94%.
The diff coverage is 57.50%.

@@             Coverage Diff             @@
##           master   #10470       +/-   ##
===========================================
- Coverage   79.24%   65.29%   -13.95%     
===========================================
  Files         393      309       -84     
  Lines      123859   106514    -17345     
===========================================
- Hits        98149    69552    -28597     
- Misses      25710    36962    +11252

Flag	Coverage Δ
#kernel	`?`
#user	`65.29% <57.50%> (+0.95%)`	⬆️

Impacted Files	Coverage Δ
cmd/raidz_test/raidz_bench.c	`0.00% <ø> (ø)`
cmd/zed/agents/zfs_mod.c	`78.21% <ø> (ø)`
cmd/zed/zed_event.c	`61.22% <ø> (ø)`
cmd/zed/zed_exec.c	`82.92% <ø> (ø)`
cmd/zed/zed_file.c	`33.80% <ø> (ø)`
cmd/zed/zed_strings.c	`81.25% <ø> (ø)`
cmd/zfs/zfs_main.c	`82.69% <ø> (+0.05%)`	⬆️
cmd/zfs_ids_to_path/zfs_ids_to_path.c	`60.00% <ø> (ø)`
cmd/zinject/zinject.c	`37.99% <ø> (ø)`
cmd/zpool/zpool_main.c	`80.94% <ø> (+0.10%)`	⬆️
... and 296 more

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 fccaea4...d7be42d. Read the comment docs.

nivedita76 · 2020-06-17T13:24:09Z

I would expect for zfs_context.h being included before other headers to be generally important.
Most of this looks good, let's get it building now.

I can take out the header resorting if that's safer. I started doing it then realized that would be a project on its own so its only done in a couple places.

behlendorf · 2020-06-17T20:33:24Z

Thanks for tackling this cleanup! When reviewing this all the changes looked reasonable though I realize you're still iterating on this to resolve the remaining build issues. Thanks for breaking it up in to multiple commits to make it easier to review. I'm happy to give it another look when it's building everywhere.

module/zfs/zil.c

ghost · 2020-06-17T21:53:01Z

Things all look good on the FreeBSD side. I only skimmed Linux-specific code.

Mark functions used only in the same translation unit as static. This only includes functions that do not have a prototype in a header file either. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Delete unused functions. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Include the header with prototypes in the file that provides definitions as well, to catch any mismatch between prototype and definition. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Add prototypes/move prototypes to header files. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Turn the generic versions into inline functions and avoid SKEIN_PORT_CODE trickery. Also drop the PLATFORM_MUST_ALIGN check for using the fast bcopy variants. bcopy doesn't assume alignment, and the userspace version is currently different because the _ALIGNMENT_REQUIRED macro is only defined by the kernelspace headers. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

spl-generic.c defines some of the libgcc integer library functions on 32-bit. Don't bother checking -Wmissing-prototypes since nothing should directly call these functions from C code. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Switch on warning flags to detect mismatch between declaration and definition. Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

nivedita76 · 2020-06-18T17:27:39Z

@behlendorf builds are finally working now.

Let me know if I should rebase it.

behlendorf

The Linux bits look good too. I don't think there's a need to rebase this since it's only behind by six commits. I can verify it still builds cleanly locally which applied to master, and if that looks good I'll get it merged.

nivedita76 · 2020-06-18T19:19:27Z

The Linux bits look good too. I don't think there's a need to rebase this since it's only behind by six commits. I can verify it still builds cleanly locally which applied to master, and if that looks good I'll get it merged.

Ok thanks.

Mark functions used only in the same translation unit as static. This only includes functions that do not have a prototype in a header file either. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Delete unused functions. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Include the header with prototypes in the file that provides definitions as well, to catch any mismatch between prototype and definition. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Add prototypes/move prototypes to header files. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Turn the generic versions into inline functions and avoid SKEIN_PORT_CODE trickery. Also drop the PLATFORM_MUST_ALIGN check for using the fast bcopy variants. bcopy doesn't assume alignment, and the userspace version is currently different because the _ALIGNMENT_REQUIRED macro is only defined by the kernelspace headers. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

spl-generic.c defines some of the libgcc integer library functions on 32-bit. Don't bother checking -Wmissing-prototypes since nothing should directly call these functions from C code. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Switch on warning flags to detect mismatch between declaration and definition. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Commit ec21397 ("async zvol minor node creation interferes with receive") replaced zvol_create_minors with zvol_create_minor and zvol_create_minors_recursive, changing the prototype at the same time. However the stub functions in libzpool/kernel.c were defined with the old prototype. As the definitions are empty, this doesn't cause any runtime issues, but an LTO build shows warnings because of the mismatched prototypes. Commit a0bd735 ("Add support for asynchronous zvol minor operations") removed the real zvol_remove_minor, but for some reason added a stub implementation in libzpool/kernel.c with no references. Delete this dead code. Commit 196bee4 ("Remove deduplicated send/receive code") removed zfs_onexit_del_cb and zfs_onexit_cb_data. Drop the stubs as well. Add zvol.h include to provide prototypes, and sort the include directives. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Mark functions used only in the same translation unit as static. This only includes functions that do not have a prototype in a header file either. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Delete unused functions. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Include the header with prototypes in the file that provides definitions as well, to catch any mismatch between prototype and definition. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Add prototypes/move prototypes to header files. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Turn the generic versions into inline functions and avoid SKEIN_PORT_CODE trickery. Also drop the PLATFORM_MUST_ALIGN check for using the fast bcopy variants. bcopy doesn't assume alignment, and the userspace version is currently different because the _ALIGNMENT_REQUIRED macro is only defined by the kernelspace headers. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

spl-generic.c defines some of the libgcc integer library functions on 32-bit. Don't bother checking -Wmissing-prototypes since nothing should directly call these functions from C code. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

Switch on warning flags to detect mismatch between declaration and definition. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes #10470

spl-generic.c defines some of the libgcc integer library functions on 32-bit. Don't bother checking -Wmissing-prototypes since nothing should directly call these functions from C code. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470

Commit ec21397 ("async zvol minor node creation interferes with receive") replaced zvol_create_minors with zvol_create_minor and zvol_create_minors_recursive, changing the prototype at the same time. However the stub functions in libzpool/kernel.c were defined with the old prototype. As the definitions are empty, this doesn't cause any runtime issues, but an LTO build shows warnings because of the mismatched prototypes. Commit a0bd735 ("Add support for asynchronous zvol minor operations") removed the real zvol_remove_minor, but for some reason added a stub implementation in libzpool/kernel.c with no references. Delete this dead code. Commit 196bee4 ("Remove deduplicated send/receive code") removed zfs_onexit_del_cb and zfs_onexit_cb_data. Drop the stubs as well. Add zvol.h include to provide prototypes, and sort the include directives. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470

Mark functions used only in the same translation unit as static. This only includes functions that do not have a prototype in a header file either. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470

Delete unused functions. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470

Include the header with prototypes in the file that provides definitions as well, to catch any mismatch between prototype and definition. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470

Add prototypes/move prototypes to header files. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470

Turn the generic versions into inline functions and avoid SKEIN_PORT_CODE trickery. Also drop the PLATFORM_MUST_ALIGN check for using the fast bcopy variants. bcopy doesn't assume alignment, and the userspace version is currently different because the _ALIGNMENT_REQUIRED macro is only defined by the kernelspace headers. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470

spl-generic.c defines some of the libgcc integer library functions on 32-bit. Don't bother checking -Wmissing-prototypes since nothing should directly call these functions from C code. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470

Switch on warning flags to detect mismatch between declaration and definition. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Closes openzfs#10470