open-iscsi / open-isns

iSNS server and client for Linux
GNU Lesser General Public License v2.1
26 stars 22 forks source link

Add support for building a shared library #4

Closed chris-se closed 8 years ago

chris-se commented 8 years ago

I'm in the process of packaging open-isns for Debian (because I co-maintain open-iscsi there), and have noticed that open-isns only builds a static library. This is frowned upon in Debian (and many other distributions) for non-internal libraries, because every update (even security updates) would require a rebuild of all the software that uses them.

I've hence added support for optionally building a shared library to the open-isns build system. This is off by default, so without any additional options to ./configure, the build will produce the same result as previously. Now, however, --enable-shared can be specified, which will build a shared library. The diff is a quite large because I had to regenerate configure, and I don't have the really old autotools version installed with which it was initially generated, inflating the changes.

In following best practices for shared libraries, I've added a version script for symbol versioning. This has three advantages for the shared library (as compared to no symbol versioning):

  1. It allows to hide library-internal symbols (i.e. functions and global variables), so that they are available from code within the library, but don't pollute the namespace.
  2. It allows to clearly tag external symbols (that are also exported in the public headers) with the version they first appeared in the shared library (I've used 0.96 for that, as I assume that'll be the next release and the first one that produces a shared library). If you ever did ABI-incompatible changes to a given symbol, you could still provide a second version that is compatible with the old ABI, without having to bump the soname. This allows for a large amount of backwards-compatibility.
  3. It allows to tag symbols that are not in the public headers, but used by the open-isns binaries (isnsd etc.) as internal (I've used the version tag LIBISNS_INTERNAL), so that you advertise that these symbols shouldn't be used by anyone else and are not part of the official ABI (even though they are exported to be available from the open-isns binaries). This way it's easily possible to modify the ABI of those symbols without breaking external binary compatibility.

The linker script I've added explicitly exports functions that are available in the public headers (with the LIBISNS_0.96 version tag), plus the functions that are used by other isns binaries (with the LIBISNS_INTERNAL version tag). All other functions are not exported (equivalent to -fvisibility=hidden or __attribute__((visibility("hidden")))) by using a catch-all rule in the linker script.

Non-exported symbols

For completeness sake the following is a list of all functions and global variables that are not in the linker script of the shared library, and hence not exported. I've listed them here because it's maybe good to have an overview at the point in time when the shared library is added. At first, I'll list three categories where I think some improvements could be made (even though it's not strictly necessary).

Defined, but not used: The following symbols are defined in internal headers (or source files), but not used anywhere - and are not declared in external headers. Some of them could maybe be declared in public headers, while others could possibly be removed?

isns_attr_list_get_ipaddr
isns_db_create_relation
isns_db_relation_exists
isns_net_error
isns_object_remove_member
isns_object_set_value
isns_print_stderr
isns_relation_halfspace
isns_simple_decode_response

Static candidates: The following symbols are only used within a single source file, and should probably be declared as static (they appear to be helper functions called by other functions in the same source file):

__isns_bitvector_find_word
__isns_db_insert
__isns_db_keystore_change_notify
__isns_db_keystore_lookup
__isns_recv_message
buf_fill
isns_bitvector_dump
isns_db_build_pg_relation
isns_dsa_store_public
isns_object_list_sort
isns_policy_validate_node_name
isns_scope_get
isns_socket_release
parser_punctuation

Publicly exported, but internal: This function is declared in a public header, yet is so clearly internal that I've marked it as hidden and not exported it from the shared library anyway. In my opinion, it should probably be removed from the public header:

__isns_alloc_message

Internal symbols: These symbols really are internal, because they are only used within the library, but not from the outside. Anything in this category is supposed to be a hidden symbol in the shared library, so these are all where they are supposed to be:

__isns_db_get_next
__isns_policy_alloc
isns_authblock_decode
isns_authblock_encode
isns_create_default_domain
isns_create_file_db_backend
isns_create_relation
isns_create_simple_keystore
isns_db_get_relationship_object
isns_db_get_relationship_objects
isns_dsa_decode_public
isns_dsa_init_key
isns_dsa_init_params
isns_get_principal
isns_object_reference_drop
isns_object_reference_set
isns_object_state_string
isns_object_template_by_name
isns_policy_bind
isns_policy_default
isns_policy_default_entity
isns_policy_release
isns_policy_server
isns_policy_validate_entity
isns_policy_validate_function
isns_policy_validate_node_type
isns_policy_validate_object_access
isns_policy_validate_object_creation
isns_policy_validate_object_type
isns_policy_validate_object_update
isns_policy_validate_scn_bitmap
isns_policy_validate_source
isns_principal_set_policy
isns_relation_add
isns_relation_exists
isns_relation_find_edge
isns_relation_get_edge_objects
isns_relation_get_other
isns_relation_is_dead
isns_relation_release
isns_relation_remove
isns_relation_sever
isns_relation_soup_alloc
isns_scope_add
isns_scope_alloc
isns_scope_for_call
isns_scope_gang_lookup
isns_scope_get_next
isns_scope_get_related
isns_scope_release
isns_scope_remove
isns_security_sign
isns_security_verify
isns_slp_find
isns_socket_submit

As a follow-up, if you wish, I can open a pull request to remove __isns_alloc_message from public headers and declare all the candidates I've identified as static. For the declared-but-not-used case one would have to go through the functions on a case-by-case basis and decide what to do, and I don't really know the code base well enough to make a judgment call on them. (But they are only a few, so it's perfectly fine to keep them around as hidden symbols for now.)

But even without the possible additional changes of cleaning up some symbols (which isn't strictly required, just nice-to-have), it would be great if you could merge this pull request, so that building a shared library is now possible. (This is the only blocker that I have before packaging it for Debian.) Thanks!

gonzoleeman commented 8 years ago

I will test your branch, but as long as it works fine I have no problem merging this request. Thank you for all your work!

chris-se commented 8 years ago

Thanks a lot! While packaging open-isns (which I only started after sending this pull request) I found a couple of minor other things (typos in manpages, some minor build system things, default pid file of the discovery daemon, isnsd not handling isns not being in /etc/services), should I send another pull request (then I'd have to wait until you've merged this one, as the build system stuff would otherwise conflict with this patch) or add them to this pull request?

gonzoleeman commented 8 years ago

I suggest a separate pull request. I'll merge this one soon. It builds for me with and without shared library.