libnvme: enforce ABI boundary with -fvisibility=hidden#3191
libnvme: enforce ABI boundary with -fvisibility=hidden#3191martin-belanger wants to merge 1 commit intolinux-nvme:masterfrom
Conversation
Add -fvisibility=hidden to the libnvme library build and introduce a
LIBNVME_PUBLIC macro (defined as __attribute__((visibility("default")))
in lib-types.h) to explicitly mark each exported function definition.
With this change the compiler enforces the ABI boundary rather than
relying solely on the linker version scripts (libnvme.ld, libnvmf.ld,
accessors.ld). The immediate benefits are:
- Any function accidentally omitted from the version script but lacking
LIBNVME_PUBLIC will be hidden at compile time, making the omission
visible as a link error rather than a silent symbol leak.
- nm -D on the installed .so gives an authoritative, minimal symbol
list suitable for CI checks and abidiff.
The annotation is placed on function *definitions* in the .c files only.
Installed public headers are left unannotated so that third-party code
that includes them is not exposed to a build-internal attribute.
The generate-accessors tool is updated to emit LIBNVME_PUBLIC on the
generated function definitions in accessors.c.
While auditing the version scripts, 34 stale symbol entries that had no
corresponding definition in the source tree were removed from libnvme.ld
and libnvmf.ld.
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
| * by default. Annotating a function with LIBNVME_PUBLIC overrides that and | ||
| * makes the symbol visible in the shared library ABI. | ||
| */ | ||
| #define LIBNVME_PUBLIC __attribute__((visibility("default"))) |
There was a problem hiding this comment.
Why does need to be here? I hope we don't have to expose compile/linker time information to the user.
Also the name is a bit long (after looking at what systemd does), _public_. Given that we already have similiar attributes names in cleanup.h I think we should follow this pattern too.
There was a problem hiding this comment.
It doesn’t strictly have to live here. I placed it in lib-types.h mainly because that header is already included by all the files that require this definition. If there’s a more appropriate location, I’m happy to move it.
Agreed on the naming as well — I will update the name to _public_ to align with the existing pattern in cleanup.h.
| bool (*decide_retry)( | ||
| struct nvme_transport_handle *hdl, | ||
| struct nvme_passthru_cmd *cmd, | ||
| int err)) |
There was a problem hiding this comment.
There are more whitespace changes like this, where unnecessary formatting changes are done. E.g. the decide_retry is formatting change is not necessary IMO.
There was a problem hiding this comment.
Let me look into this. I was trying to keep checkpatch happy (80 chars max), but maybe I went a bit overboard.
There was a problem hiding this comment.
No worries. Ideally we get clang-format eventually working. I know that some people are using in
kernel context. The trick is not to use it on the compete file as it doesn't really work.
| * Defaults to enabled, which results in some initial messaging with the | ||
| * endpoint to determine model-specific details. | ||
| */ | ||
| void nvme_set_probe_enabled(struct nvme_global_ctx *ctx, bool enabled); |
There was a problem hiding this comment.
Could you move the dropping of the unused/stale symbols into a preparation patch?
| int hmac, | ||
| unsigned char *configured_key, | ||
| int key_len, | ||
| char **ident) |
There was a problem hiding this comment.
This reformatting should be avoided, doesn't match with the style used through this file
|
|
||
| __attribute__((weak)) void *nvme_mi_submit_entry(__u8 type, const struct nvme_mi_msg_hdr *hdr, | ||
| size_t hdr_len, const void *data, size_t data_len) | ||
| LIBNVME_PUBLIC __attribute__((weak)) void *nvme_mi_submit_entry( |
There was a problem hiding this comment.
Ah we should also introduce an attribute the for the weak, e.g. __weak. but then this would look a bit
funny _public_ __weak.
We already have _cleanup_, following the systemd style. Thus _weak_ would fit more. But we could also go full kernel mode here and do the __name style which is more inline what the C spec says about the attributes:
https://cppreference.net/c/language/attributes.html
I'd say it's it makes more sense to follow the kernel style here as this project is really close to kernel hackers :)
|
Closing. Will submit antother PR shortly. |
2 participants
Add -fvisibility=hidden to the libnvme library build and introduce a LIBNVME_PUBLIC macro (defined as attribute((visibility("default"))) in lib-types.h) to explicitly mark each exported function definition.
With this change the compiler enforces the ABI boundary rather than relying solely on the linker version scripts (libnvme.ld, libnvmf.ld, accessors.ld). The immediate benefits are:
The annotation is placed on function definitions in the .c files only. Installed public headers are left unannotated so that third-party code that includes them is not exposed to a build-internal attribute.
The generate-accessors tool is updated to emit LIBNVME_PUBLIC on the generated function definitions in accessors.c.
While auditing the version scripts, 34 stale symbol entries that had no corresponding definition in the source tree were removed from libnvme.ld and libnvmf.ld.