libzfs: -fvisibility=hidden

[nabijaczleweli]

Motivation and Context

libzfs exports a lot of internal-use-only symbols that it really doesn't need to (or shouldn't).

Description

The first patch hides all symbols noted in libzfs_impl.h (and marks printf-likes as printf-likes).

The second patch moves an API from libzfs_impl.h to libzfs.h, fixing zfs(8).

The third patch fixes all warnings produced by questionable uses of the printf-likes.

The fourth patch adds zfs_get_underlying_type (like zfs_get_type, but returns zfs_head_type), and removes all usages of libzfs_impl.h from cmd/

The fifth patch moves libzfs_impl.h into lib/libzfs/.

How Has This Been Tested?

I uhhhh looked at it. The third patch is the only one with real semantic changes and it's (probably) fine.

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) – zfs_mount_delegation_check disappeared and that was used by zfs(8), so previous zfs(8)s will not work, everything else is additive
• Documentation (a change to man pages or other documentation)

Checklist:

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

[jwk404]

I ran make storeabi after building these bits, and expected the visibility of functions given the hidden visibility attributes to say as much, but they still show default visibility. Is that expected?

[nabijaczleweli]

Hmm? They aren't exported anymore (as seen in <elf-function-symbols> and nm -D) – that's the intended result.

The <function-decl name='zprop_expand_list' filepath='../../include/libzfs_impl.h' line='159' column='1' visibility='default' binding='global' size-in-bits='64'> sexion probably attempts to mean something, but who knows what (the naming implies the visibility of the declaration, but that doesn't mean anything, especially in C), I've given up on trying to make sense of abigail output.

[jwk404]

In talking with @ahrens about these changes, he had several questions about the use of attributes here in terms of maintainability and possible existing consumers, so I've asked him to elaborate on those.

That said, it looks like patches 2, 3 and 4 of the series in this PR could move ahead, if you'd like to break those out into another PR.

[nabijaczleweli]

In terms of maintainability "hiding internal APIs" is generally preferred, I'd think, and this is one of a few breaking(ish) changes I've made to the libzfs ABIs for 2.1 (I think Rich had/has at least one too) – the sover needs a bump anyway, but I hope to, by doing a pass like this over libzfs (and others, time permitting), (a) sanitise the global symbol namespace a bit and (b) enforce the privacy boundary to (c) make further changes less likely to break users (but if they were using an internal API they were kinda asking for it). This will, for example, enable us to modify the handle at will, because there will be exactly 0 users that could possibly see it.

Moved middle three to #12116, will rebase this on that when it finishes building

[ahrens]

libzfs is kind of a free-for-all, in that we don't commit to any of its interfaces, and the interface is not especially well designed (for example, the value of ZPOOL_STATUS_OK changes whenever a new status value is added). That said, I agree that it's valuable to make it harder to misuse, by not exporting symbols that should not be used by consumers.

I think it would be more maintainable to do that by defaulting to NOT export any symbols, and then explicitly annotating the symbols that we do want to make available. That way, when adding a new symbol we can't forget to add the "don't export" annotation and then have to remove the export in a later release. We can't forget to add the "yes export" annotation if it's used in our own libzfs consumers (/sbin/zfs, etc). If there's a symbol that we don't consume outside the library but it's useful to others, we can always add the "yes export" annotation later.

From reading the gcc documentation, it looks like this is done by compiling with -fvisibility=hidden, and then annotating symbols that we want to export with __attribute__ ((visibility ("default"))) (preferably via a macro). Of course we would want to find a way to do this on all supported build targets, including llvm (which is used on FreeBSD).

[nabijaczleweli]

You raise some good points, and that's definitely a better approach than what I wrote originally – see new HEAD (it's on top of #12116 by necessity).

The symbol table changed as such:

diff --git a/pre b/post
index be55f21..dc2e21f 100755
--- a/pre
+++ b/post
@@ -205,7 +205,6 @@ U mmap64
 U mount
 T mountpoint_namecheck
 U munmap
-D nfs_only
 U nvlist_add_boolean
 U nvlist_add_nvlist
 U nvlist_add_nvlist_array
@@ -280,17 +279,9 @@ T sa_errorstr
 T sa_is_shared
 T sa_validate_shareopts
 U sched_yield
-T SHA256Init
-T SHA2Final
-T SHA2Init
-T SHA2Update
-T SHA384Init
-T SHA512Init
-D share_all_proto
 U sigaction
 U sigemptyset
 U sleep
-D smb_only
 B smb_shares
 T snapshot_namecheck
 U snprintf
@@ -418,7 +409,6 @@ T zfs_ioctl
 T zfs_is_mounted
 T zfs_is_shared
 T zfs_is_shared_nfs
-T zfs_is_shared_proto
 T zfs_is_shared_smb
 T zfs_iter_bookmarks
 T zfs_iter_children
@@ -578,7 +568,6 @@ T zpool_label_disk
 U zpool_label_disk_wait
 T zpool_load_compat
 T zpool_log_history
-W zpool_mount_datasets
 T zpool_name_to_prop
 T zpool_obj_to_path
 T zpool_obj_to_path_ds
@@ -615,7 +604,6 @@ T zpool_skip_pool
 T zpool_state_to_name
 T zpool_sync_one
 T zpool_trim
-W zpool_unmount_datasets
 T zpool_upgrade
 T zpool_vdev_attach
 T zpool_vdev_clear

This is much cleaner implementation-wise, too!

There's some symbol leakage from libshare et al, but that could probably go in a PR after this one and #12050.

I was very conservative when it came to trimming out symbols – the crypto code (and headers that export one symbol) isn't necessarily consistent internally, as much as it is consistent with What Happened To Be Exported Before.

[ahrens]

I didn't review the code in its entirety, but this seems like a good approach.