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

Many private symbols exported which are not part of the public API #918

Open
mbiebl opened this issue Jun 29, 2023 · 13 comments
Open

Many private symbols exported which are not part of the public API #918

mbiebl opened this issue Jun 29, 2023 · 13 comments

Comments

@mbiebl
Copy link
Contributor

mbiebl commented Jun 29, 2023

This is a follow-up for #366
Rerunning the check for 3.0-1, lists the following exported symbols, which are missing from the header files:

bd_btrfs_device_info_get_type
bd_btrfs_filesystem_info_get_type
bd_btrfs_subvolume_info_get_type
bd_crypto_bitlk_info_get_type
bd_crypto_integrity_extra_get_type
bd_crypto_integrity_info_get_type
bd_crypto_keyslot_context_get_type
bd_crypto_luks_extra_get_type
bd_crypto_luks_info_get_type
bd_crypto_luks_pbkdf_get_type
bd_crypto_luks_token_info_get_type
bd_fs_btrfs_info_get_type
bd_fs_exfat_info_get_type
bd_fs_ext2_info_get_type
bd_fs_ext3_info_get_type
bd_fs_ext4_info_get_type
bd_fs_f2fs_info_get_type
bd_fs_features_copy
bd_fs_features_free
bd_fs_features_get_type
bd_fs_mkfs_options_copy
bd_fs_mkfs_options_free
bd_fs_mkfs_options_get_type
bd_fs_nilfs2_info_get_type
bd_fs_ntfs_info_get_type
bd_fs_udf_info_get_type
bd_fs_vfat_info_get_type
bd_fs_xfs_info_get_type
bd_loop_get_autoclear
bd_loop_info_get_type
bd_lvm_cache_stats_get_type
bd_lvm_lvdata_get_type
bd_lvm_pvdata_get_type
bd_lvm_segdata_copy
bd_lvm_segdata_free
bd_lvm_segdata_get_type
bd_lvm_vdolvpoolname
bd_lvm_vdopooldata_get_type
bd_lvm_vdo_stats_get_type
bd_lvm_vgdata_get_type
bd_md_detail_data_get_type
bd_md_examine_data_get_type
bd_nvdimm_namespace_info_get_type
bd_nvme_connect
bd_nvme_controller_info_copy
bd_nvme_controller_info_free
bd_nvme_controller_info_get_type
bd_nvme_device_self_test
bd_nvme_disconnect
bd_nvme_disconnect_by_path
bd_nvme_error_log_entry_copy
bd_nvme_error_log_entry_free
bd_nvme_error_log_entry_get_type
bd_nvme_error_quark
bd_nvme_find_ctrls_for_ns
bd_nvme_format
bd_nvme_generate_host_nqn
bd_nvme_get_controller_info
bd_nvme_get_error_log_entries
bd_nvme_get_host_id
bd_nvme_get_host_nqn
bd_nvme_get_namespace_info
bd_nvme_get_sanitize_log
bd_nvme_get_self_test_log
bd_nvme_get_smart_log
bd_nvme_lba_format_copy
bd_nvme_lba_format_free
bd_nvme_lba_format_get_type
bd_nvme_namespace_info_copy
bd_nvme_namespace_info_free
bd_nvme_namespace_info_get_type
bd_nvme_sanitize
bd_nvme_sanitize_log_copy
bd_nvme_sanitize_log_free
bd_nvme_sanitize_log_get_type
bd_nvme_self_test_log_copy
bd_nvme_self_test_log_entry_copy
bd_nvme_self_test_log_entry_free
bd_nvme_self_test_log_entry_get_type
bd_nvme_self_test_log_free
bd_nvme_self_test_log_get_type
bd_nvme_self_test_result_to_string
bd_nvme_set_host_id
bd_nvme_set_host_nqn
bd_nvme_smart_log_copy
bd_nvme_smart_log_free
bd_nvme_smart_log_get_type
bd_part_disk_spec_get_type
bd_part_spec_get_type
_vglock_start_stop
@vojtechtrefny
Copy link
Member

I knew I wanted to check one more thing before doing a release... I fixed some of the issues in #920, the remaining problematic functions should be the _get_type functions and the NVMe plugin functions.

NVMe plugin: I am not sure what's happening here -- are you compiling the library --without-nvme? The nvme.h header file is placed in a subdirectory so maybe this doesn't work on Debian. I'll wait for @tbzatek to look at this.

_get_type functions: We need these just for GObject Introspection so I don't want to add them to our header files, I am not sure if we can hide these, I'll check how other GI projects deal with this.

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 29, 2023

Atm I don't enable nvme, correct

@tbzatek
Copy link
Member

tbzatek commented Jun 29, 2023

Atm I don't enable nvme, correct

Yeah, that explains, the header file is not installed if not configured yet the library contains stub funcs.

@tbzatek
Copy link
Member

tbzatek commented Jun 29, 2023

The _get_type() functions might be worth having in public header files for the sake of compleness and to follow GObject rules. There might be scenarios where somebody would want to extend the type, although we use it primarily for structs.

I don't think this is a pressing issue at the moment, thanks for reporting this anyway.

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 29, 2023

Nod, certainly not pressing.
Thanks for the clarification regarding the nvme plugin.
I had disabled it for the time being waiting for https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1039858 to be fixed.
But maybe I'll just add those dependencies manually to libblockdev for now.

@tbzatek
Copy link
Member

tbzatek commented Jun 29, 2023

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1039858

I guess we're the first independent consumers of libnvme as nvme-cli may require these deps themselves :-)

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 29, 2023

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1039858

I guess we're the first independent consumers of libnvme as nvme-cli may require these deps themselves :-)

Do you consider libnvme stable enough for the plugin to be enabled by default?

@tbzatek
Copy link
Member

tbzatek commented Jun 29, 2023

Do you consider libnvme stable enough for the plugin to be enabled by default?

libnvme has been stable for some time (and is well tested by nvme-cli). The libblockdev nvme plugin + udisks interfaces have been around for some time too, used in blivet and Anaconda. We intentionally left NVMe discovery out of these releases as that's where API stability was questionable, for the rest we need first adopters that were waiting for a release. Still, the APIs are fairly minimal and I don't expect any need for a change.

FYI, libblockdev-nvme is a hard dep for udisks-2.10.0

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 29, 2023

I ran my script on git master with nvme enabled:

bd_btrfs_device_info_get_type
bd_btrfs_filesystem_info_get_type
bd_btrfs_subvolume_info_get_type
bd_crypto_bitlk_info_get_type
bd_crypto_integrity_extra_get_type
bd_crypto_integrity_info_get_type
bd_crypto_keyslot_context_get_type
bd_crypto_luks_extra_get_type
bd_crypto_luks_info_get_type
bd_crypto_luks_pbkdf_get_type
bd_crypto_luks_token_info_get_type
bd_fs_btrfs_info_get_type
bd_fs_exfat_info_get_type
bd_fs_ext2_info_get_type
bd_fs_ext3_info_get_type
bd_fs_ext4_info_get_type
bd_fs_f2fs_info_get_type
bd_fs_features_get_type
bd_fs_mkfs_options_get_type
bd_fs_nilfs2_info_get_type
bd_fs_ntfs_info_get_type
bd_fs_udf_info_get_type
bd_fs_vfat_info_get_type
bd_fs_xfs_info_get_type
bd_loop_info_get_type
bd_lvm_cache_stats_get_type
bd_lvm_lvdata_get_type
bd_lvm_pvdata_get_type
bd_lvm_segdata_get_type
bd_lvm_vdopooldata_get_type
bd_lvm_vdo_stats_get_type
bd_lvm_vgdata_get_type
bd_md_detail_data_get_type
bd_md_examine_data_get_type
bd_nvdimm_namespace_info_get_type
bd_nvme_controller_info_get_type
bd_nvme_error_log_entry_get_type
bd_nvme_lba_format_get_type
bd_nvme_namespace_info_get_type
bd_nvme_sanitize_log_get_type
bd_nvme_self_test_log_entry_get_type
bd_nvme_self_test_log_get_type
bd_nvme_smart_log_get_type
bd_part_disk_spec_get_type
bd_part_spec_get_type
_nvme_fabrics_errno_to_gerror
_nvme_status_to_error
_open_dev

@mbiebl
Copy link
Contributor Author

mbiebl commented Jun 29, 2023

With #923 applied, this looks much better now, thanks!

@mbiebl
Copy link
Contributor Author

mbiebl commented Jul 3, 2023

Should we close this or do you want to keep it open as a reminder how to deal with the remaining _get_type symbols?

@mbiebl
Copy link
Contributor Author

mbiebl commented Jul 3, 2023

Btw, are these errors to be expected on s390(x)
https://buildd.debian.org/status/fetch.php?pkg=libblockdev&arch=s390x&ver=3.0-1&stamp=1688370506&raw=0

The Debian package is built using --without-s390.
Why are those symbols exported in this case?

@mbiebl
Copy link
Contributor Author

mbiebl commented Jul 4, 2023

The symbol fs_info slipped into the 3.0.1 release.
It's a side-effect of 0d97224 (dropping the static)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants