Verify symbol visibility

[tasleson]

This shouldn't pass CI until PR: https://github.com/intel/ledmon/pull/162 gets merged.

[mtkaczyk]

Hi @tasleson Please provide reference of the check method you implemented. I cannot determine if it is standard way to check it.

[tasleson]

Hi @tasleson Please provide reference of the check method you implemented. I cannot determine if it is standard way to check it.

This is using nm, ref. https://www.man7.org/linux/man-pages/man1/nm.1.html We could also leverage readelf, but historically, I've used nm. I think the important part is that whatever we are using is available on the CI systems.

[mtkaczyk]

Hi @tasleson, I'm confused because when I started to analyze the new flags we want to add in #145 I realized that we are complying libled as dynamic library. It suppressed me because I assumed that we agreed to procced with static one. I'm trying to understand the automakes makefiles and I still don't understand why internal lib is compiled to liblecinternal,a, same as lib common (noinst_ matters?) Could you clarify?

Well, we have it now, I just would like to understand it. It is important to me to determine which flags are applicable (even if I allready determined that libtools adds -fPIC automatically.

I would prefer to have it directly defined in makefiles rather than rely on automake.

[tasleson]

@mtkaczyk

Hi @tasleson, I'm confused because when I started to analyze the new flags we want to add in #145 I realized that we are complying libled as dynamic library. It suppressed me because I assumed that we agreed to procced with static one. I'm trying to understand the automakes makefiles and I still don't understand why internal lib is compiled to liblecinternal,a, same as lib common (noinst_ matters?) Could you clarify?

If you run ldd on ledmon or ledctl you will find that they are not linked dynamically to libled.

$ ldd src/ledctl/ledctl
    linux-vdso.so.1 (0x00007ffe715d1000)
    libpci.so.3 => /lib64/libpci.so.3 (0x00007f20f4a19000)
    libsgutils2-1.46.so.2 => /lib64/libsgutils2-1.46.so.2 (0x00007f20f49d5000)
    libc.so.6 => /lib64/libc.so.6 (0x00007f20f47f7000)
    /lib64/ld-linux-x86-64.so.2 (0x00007f20f4a48000)

I structured the code so that we build the internal library which is used by ledctl and ledmon to link against and is also used for creating the shared library. I very much followed your wishes in that ledmon and ledctl not be linked against a shared library. This should also explain why some files are also no install, they aren't needed after things get linked.

This arrangement allows for packages to be built where you can just install the shared library or just install the command line tools and not require the the other package.

Well, we have it now, I just would like to understand it. It is important to me to determine which flags are applicable (even if I allready determined that libtools adds -fPIC automatically.

I would prefer to have it directly defined in makefiles rather than rely on automake.

We should be able to place any specific flags for the library in: src/lib/Makefile.am

[tasleson]

This PR ensures that we are building the shared library with -fvisibility=hidden. If we don't have this in use we end up with exported symbols like:

$ nm -D src/lib/.libs/libled.so | grep " T "
00000000000086c0 T ahci_get_port_path
0000000000008580 T ahci_sgpio_write
0000000000009450 T alloc_host
000000000000e250 T amd_em_enabled
000000000000e3d0 T amd_get_path
00000000000102f0 T _amd_ipmi_em_enabled
0000000000010590 T _amd_ipmi_get_path
0000000000010430 T _amd_ipmi_write
000000000000f320 T amd_sgpio_cache_free
000000000000f380 T _amd_sgpio_em_enabled
000000000000fab0 T _amd_sgpio_get_path
000000000000f810 T _amd_sgpio_write
000000000000e370 T amd_write
0000000000008e80 T block_compare
0000000000008d90 T block_device_duplicate
0000000000008d40 T block_device_fini
0000000000008920 T block_device_init
00000000000087c0 T block_get_controller
0000000000008850 T block_get_host
000000000000a540 T buf_read
000000000000a680 T buf_read_to_dest
000000000000a4a0 T buf_write
0000000000009b20 T cntrl_device_fini
00000000000095b0 T cntrl_device_init
000000000000d480 T cntrl_init_smp
000000000000a800 T _common_log
000000000000d7a0 T dellssd_get_path
000000000000d7b0 T dellssd_write
00000000000087a0 T dev_directly_attached
00000000000116a0 T device_allow_pattern_add
0000000000011890 T device_blink_behavior_set
0000000000011720 T device_exclude_pattern_add
0000000000009c30 T enclosure_device_fini
0000000000009d90 T enclosure_device_init
0000000000009b50 T enclosure_get_state
0000000000009c70 T enclosure_open
0000000000009ca0 T enclosure_reload
0000000000009ff0 T enclosure_set_state
000000000000a0d0 T enclosure_slot_property_init
000000000000e0f0 T _find_file_path
00000000000094e0 T _find_host
000000000000b8c0 T find_raid_device
0000000000011580 T find_slot_by_device_name
0000000000011600 T find_slot_by_slot_path
00000000000094a0 T free_hosts
000000000000d150 T get_bdev_ibpi_buffer
0000000000008890 T get_block_device_from_sysfs_path
000000000000a760 T get_bool
000000000000b160 T get_by_bits
000000000000b1b0 T get_by_ibpi
000000000000b200 T get_by_value
000000000000d6b0 T get_dell_server_type
000000000000aa90 T get_id
000000000000aa50 T get_int
000000000000ac90 T get_log_fd
000000000000b010 T get_option_id
000000000000ab80 T get_path_hostN
0000000000011690 T get_slot_pattern
000000000000a640 T get_text
000000000000a710 T get_text_to_dest
000000000000a960 T get_uint64
000000000000b140 T ibpi2str
000000000000b100 T ibpi2str_table
0000000000010610 T ipmicmd
0000000000010bd0 T is_npem_capable
0000000000008450 T led_cntrl_list_free
00000000000083c0 T led_cntrl_list_reset
00000000000083d0 T led_cntrl_next
0000000000008430 T led_cntrl_path
0000000000008400 T led_cntrl_prev
0000000000008480 T led_cntrls_get
0000000000008440 T led_cntrl_type
00000000000083b0 T led_cntrl_type_to_string
0000000000008180 T led_controller_slot_support
0000000000007f20 T led_device_name_lookup
00000000000080f0 T led_flush
0000000000007e60 T led_free
0000000000008030 T led_is_management_supported
0000000000007ec0 T led_log_fd_set
0000000000007ed0 T led_log_level_set
0000000000011f50 T ledmon_free_conf
0000000000011b10 T ledmon_init_conf
0000000000011f90 T ledmon_read_conf
0000000000012380 T ledmon_remove_shared_conf
00000000000120b0 T ledmon_write_shared_conf
0000000000007de0 T led_new
0000000000007ee0 T led_scan
0000000000008080 T led_set
0000000000008240 T led_slot_cntrl
0000000000008200 T led_slot_device
0000000000008150 T led_slot_find_by_device_name
0000000000008130 T led_slot_find_by_slot
0000000000008230 T led_slot_id
0000000000008120 T led_slot_list_entry_free
0000000000008260 T led_slot_list_free
0000000000008360 T led_slot_list_reset
00000000000081a0 T led_slot_next
00000000000081d0 T led_slot_prev
0000000000008170 T led_slot_set
0000000000008290 T led_slots_get
0000000000008250 T led_slot_state
0000000000008370 T led_string_to_cntrl_type
00000000000117a0 T lib_log
000000000000b400 T list_append_ctx
000000000000b270 T __list_erase
000000000000b340 T list_insert
000000000000b2e0 T __list_remove
000000000000bce0 T locate_block_by_sas_addr
000000000000acb0 T _log
000000000000a7a0 T log_close
000000000000a7d0 T log_open
0000000000011b60 T _map_log_level
000000000000abf0 T match_string
0000000000010c90 T npem_get_path
0000000000010ad0 T npem_get_state
0000000000010ca0 T npem_set_slot
0000000000010f20 T npem_set_state
0000000000010f80 T npem_slot_property_init
0000000000010f40 T npem_write
0000000000011850 T off_all
000000000000db00 T pci_get_state
000000000000daf0 T pci_set_slot
000000000000db90 T pci_slot_fini
000000000000db10 T pci_slot_init
000000000000dbc0 T pci_slot_property_init
000000000000ad80 T print_opt
000000000000b850 T raid_device_duplicate
000000000000b820 T raid_device_fini
000000000000b420 T raid_device_init
000000000000a380 T scan_dir
000000000000ba00 T scsi_get_enclosure
000000000000bc50 T scsi_get_host_path
000000000000bbd0 T scsi_ses_flush
000000000000bb80 T scsi_ses_flush_enclosure
000000000000baa0 T scsi_ses_write
000000000000bb20 T scsi_ses_write_enclosure
000000000000d170 T scsi_smp_fill_buffer
000000000000d3c0 T scsi_smp_write_buffer
0000000000011410 T ses_get_slots
0000000000011060 T ses_load_pages
00000000000113d0 T ses_send_diag
0000000000011210 T ses_write_msg
000000000000a820 T set_invocation_name
0000000000011ba0 T _set_log_level
000000000000ada0 T set_log_path
000000000000d010 T set_raw_pattern
0000000000011680 T set_slot_pattern
000000000000aec0 T setup_options
000000000000b070 T set_verbose_level
000000000000c000 T slave_device_fini
000000000000be30 T slave_device_init
000000000000d0d0 T smp_write_gpio
000000000000a850 T str_cpy
000000000000b250 T str_map
000000000000a9a0 T str_toi
000000000000a870 T str_tol
000000000000ab20 T str_toui
000000000000a910 T str_toul
000000000000cbb0 T sysfs_check_driver
000000000000cb30 T sysfs_enclosure_attached_to_cntrl
000000000000c400 T sysfs_get_block_devices
000000000000c3e0 T sysfs_get_cntrl_devices
000000000000c3d0 T sysfs_get_enclosure_devices
000000000000c410 T sysfs_get_pci_slots
000000000000cb20 T sysfs_get_slots
000000000000c3f0 T sysfs_get_volumes
000000000000c2c0 T sysfs_init
000000000000c350 T sysfs_reset
000000000000c420 T sysfs_scan
000000000000cfb0 T try_clear_sas_gpio_gp_bit
000000000000cf50 T try_set_sas_gpio_gp_bit
000000000000cef0 T try_test_sas_gpio_gp_bit
000000000000dca0 T vmdssd_check_slot_module
000000000000dd40 T vmdssd_find_pci_slot
000000000000de60 T vmdssd_get_attention
000000000000dc30 T vmdssd_get_domain
000000000000e0e0 T vmdssd_get_path
000000000000e020 T vmdssd_write
000000000000dea0 T vmdssd_write_attention_buf

When is should only look like:

$ nm -D src/lib/.libs/libled.so | grep " T "
0000000000003e20 T led_cntrl_list_free
0000000000003d90 T led_cntrl_list_reset
0000000000003da0 T led_cntrl_next
0000000000003e00 T led_cntrl_path
0000000000003dd0 T led_cntrl_prev
0000000000003e50 T led_cntrls_get
0000000000003e10 T led_cntrl_type
0000000000003d80 T led_cntrl_type_to_string
0000000000003b50 T led_controller_slot_support
00000000000038c0 T led_device_name_lookup
0000000000003ac0 T led_flush
0000000000003800 T led_free
0000000000003a00 T led_is_management_supported
0000000000003860 T led_log_fd_set
0000000000003870 T led_log_level_set
0000000000003780 T led_new
0000000000003880 T led_scan
0000000000003a50 T led_set
0000000000003c10 T led_slot_cntrl
0000000000003bd0 T led_slot_device
0000000000003b20 T led_slot_find_by_device_name
0000000000003b00 T led_slot_find_by_slot
0000000000003c00 T led_slot_id
0000000000003af0 T led_slot_list_entry_free
0000000000003c30 T led_slot_list_free
0000000000003d30 T led_slot_list_reset
0000000000003b70 T led_slot_next
0000000000003ba0 T led_slot_prev
0000000000003b40 T led_slot_set
0000000000003c60 T led_slots_get
0000000000003c20 T led_slot_state
0000000000003d40 T led_string_to_cntrl_type

Any exported symbols can be called by library users, so it's very important we don't give visibility to things we don't want them using. Otherwise they become part of the API we need to support.

[mtkaczyk]

Yup, I know. I want to fix that soon too. We added #166 to fix it ASAP. Please take a look.

[mtkaczyk]

So why we need static libled after installation:

# ll /usr/lib64 | grep libled
-rw-r--r--   1 root root   904860 Feb 23  2023 libled.a
-rwxr-xr-x   1 root root      922 Aug 29 11:08 libled.la
lrwxrwxrwx   1 root root       16 Aug 29 11:08 libled.so -> libled.so.0.97.0
lrwxrwxrwx   1 root root       16 Aug 29 11:08 libled.so.0 -> libled.so.0.97.0
-rwxr-xr-x   1 root root   444208 Aug 29 11:08 libled.so.0.97.0

[tasleson]

@mtkaczyk

So why we need static libled after installation:

# ll /usr/lib64 | grep libled
-rw-r--r--   1 root root   904860 Feb 23  2023 libled.a
-rwxr-xr-x   1 root root      922 Aug 29 11:08 libled.la
lrwxrwxrwx   1 root root       16 Aug 29 11:08 libled.so -> libled.so.0.97.0
lrwxrwxrwx   1 root root       16 Aug 29 11:08 libled.so.0 -> libled.so.0.97.0
-rwxr-xr-x   1 root root   444208 Aug 29 11:08 libled.so.0.97.0

The default for libraries is to build and install both the static and dynamic. If you pass --disable-static to ./configure you will only get the shared library installed. This is what we will be doing when we package this for fedora.

See ./configure --help for all the options.

[mtkaczyk]

Ok, got it thanks!

Sorry for bothering you with such basic question, it is my first experience with automake generated libraries. It is obvious now, all clear. I will merge this after #166

[mtkaczyk]

@tasleson Please rebase it now, I would like to have checks passed.

[tasleson]

@mtkaczyk

@tasleson Please rebase it now, I would like to have checks passed.

Rebased with tests passing after I added a commit to remove visibility for device_blink_behavior_set