Re: [PATCH] Introduce symbol versioning

[Anderson Sasaki <ansasaki@xxxxxxxxxx>]

----- Original Message -----
> From: "Aris Adamantiadis" <aris@xxxxxxxxxxxx>
> To: libssh@xxxxxxxxxx
> Sent: Monday, March 12, 2018 12:49:07 PM
> Subject: Re: [PATCH] Introduce symbol versioning
> 
> I Agree.
> 
> Also I though we already had a mechanism to build that list at build
> time. Isn't this the purpose of the SSH_API prefix in function
> prototypes in libssh.h ? I thought we had SONAME dealt with already.
> 

Yes, the mechanism which builds the API is working properly, as well as the SONAME handling. But this is not sufficient to keep the ABI stable.
And there are other problems that using only SONAME can't solve, like the problems I listed in my first email. Just to make it easier, I'll copy the list:

* in an ABI change, there is no way to keep the old version for compatibility and switch to the new version due to symbol clashes
* in an API change, there is no way to keep the old symbol for old applications, and introduce a new symbol for the newly compiled applications
* an application cannot have two or more dependencies which are linked with different versions of libssh

Please, refer to the chapter 3 in:

https://www.akkadia.org/drepper/dsohowto.pdf

> On 12/03/18 11:46, Jakub Jelen wrote:
> > On Fri, 2018-03-09 at 07:37 -0500, Anderson Sasaki wrote:
> >> [...]
> >>
> >> From 3e38ba07df7667300714771dfbe72bbd3077f582 Mon Sep 17 00:00:00
> >> 2001
> >> From: Anderson Toshiyuki Sasaki <ansasaki@xxxxxxxxxx>
> >> Date: Thu, 8 Mar 2018 13:15:34 +0100
> >> Subject: [PATCH] Introduce symbol versioning
> >>
> >> This adds a linker map, which adds version information for exported
> >> symbols, and the option to compile with symbol versioning. The symbol
> >> versioning is enabled by default, but disabled in non UNIX like
> >> platforms. It can be disabled by passing "-
> >> DWITH_SYMBOL_VERSIONING=OFF"
> >> option to cmake or "-withoutsymbolversioning" to "obj/build_make.sh".
> > There should be two dashes, isn't it? "--withoutsymbolversioning"

Yes, you are right. The option in the script is correct, the commit message is wrong.

> >
> >> [...]
> >> +
> >> +LIBSSH_4_5_0
> > Why the version 4_5_0 ? This will be the first symbol release? Or is
> > this some convention?

This will be the first symbol release. I kept the current release version to make it easier for the following releases and to make it less confusing.
The string which identifies a set of symbols in a release can be anything, but it is nice to have it tied with the releases.

> >
> >> +{
> >> +    global:
> >> +        buffer_free;
> >> +        buffer_get;
> >> +        buffer_get_len;
> >> +        buffer_new;
> >> +        channel_accept_x11;
> >> +        channel_change_pty_size;
> >> +        channel_close;
> >> +        channel_forward_accept;
> >> +        channel_forward_cancel;
> >> +        channel_forward_listen;
> >> +        channel_free;
> >> +        channel_get_exit_status;
> >> +        channel_get_session;
> >> +        channel_is_closed;
> >> +        channel_is_eof;
> >> +        channel_is_open;
> >> +        channel_new;
> >> +        channel_open_forward;
> >> +        channel_open_session;
> >> +        channel_poll;
> >> +        channel_read;
> >> +        channel_read_buffer;
> >> +        channel_read_nonblocking;
> >> +        channel_request_env;
> >> +        channel_request_exec;
> >> +        channel_request_pty;
> >> +        channel_request_pty_size;
> >> +        channel_request_send_signal;
> >> +        channel_request_sftp;
> >> +        channel_request_shell;
> >> +        channel_request_subsystem;
> >> +        channel_request_x11;
> >> +        channel_select;
> >> +        channel_send_eof;
> >> +        channel_set_blocking;
> >> +        channel_write;
> >> +        channel_write_stderr;
> >> +        privatekey_free;
> >> +        privatekey_from_file;
> >> +        publickey_free;
> >> +        publickey_from_file;
> >> +        publickey_from_privatekey;
> >> +        publickey_to_string;
> >> [...]
> >> +        _ssh_log;
> >> [...]
> >> +        string_burn;
> >> +        string_copy;
> >> +        string_data;
> >> +        string_fill;
> >> +        string_free;
> >> +        string_from_char;
> >> +        string_len;
> >> +        string_new;
> >> +        string_to_char;
> >> +    local:
> >> +        *;
> >> +};
> > Should the buffer_*, channel_*, string_*, publicky_*, privatekey_*
> > functions and obviously-internal _ssh_log() be covered in the API? I
> > don't think so. I would cover only the ssh_* and sftp_*, which is to my
> > understanding intended API.

I kept all the current exported symbols in the map to avoid breaking compatibility with the current unversioned release.
Which means that applications linked with the version prior to the introduction of versioned symbols would work with the new release with the versioning and without recompiling.

Removing something that is currently exported would make the new binary incompatible with the previous versions.
This would require applications linked with previous versions to be recompiled and linked again to work with the new.

To make the map I used the list of symbols marked with LIBSSH_API and confirmed it by comparing with the list of exported symbols in the binary given by readelf.
If there are exported symbols that shouldn't be part of the API, it is a good opportunity to fix this by removing them from the set of exported symbols.
If it is acceptable to break the compatibility with the previous versions, then the internal functions which are being exported should be removed from the API.