[libvirt] [PATCH v3 0/4] rpc: fixing compilation error due to deprecated functions.
After 0.8.0 release, libssh deprecated some functions like: ssh_is_server_known() and ssh_write_knownhost(). They were replaced by ssh_session_is_known_server() and ssh_session_update_known_hosts() respectively. This serie creates the alias to keep the compatibility and create an auxiliar enum to help it because ssh_session_update_known_hosts() introduced new state returns. v1-v2: Rebasing ssh_session_is_known_server() return states. v2-v3: Only code syntax fixes. Julio Faracco (4): m4: checking if ssh_session_is_known_server() exists. rpc: replacing ssh_is_server_known() by ssh_session_is_known_server(). m4: checking if ssh_session_update_known_hosts() exists. rpc: replacing ssh_write_knownhost() by ssh_session_update_known_hosts(). m4/virt-libssh.m4 | 12 ++++++++++++ src/rpc/virnetlibsshsession.c | 18 +++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) -- 2.19.1
This commit adds some checks inside libssh m4 checking to verify if ssh_session_is_known_server function is available. This new function scope replaces the old ssh_is_server_known() from libssh 0.8.0 and below versions. Another auxiliar enumerator was added to keep the compatibility with the old standard used by ssh_is_server_known() function. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- m4/virt-libssh.m4 | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/m4/virt-libssh.m4 b/m4/virt-libssh.m4 index 01c3b75c72..52bd4d3639 100644 --- a/m4/virt-libssh.m4 +++ b/m4/virt-libssh.m4 @@ -33,6 +33,14 @@ AC_DEFUN([LIBVIRT_CHECK_LIBSSH],[ [], [AC_DEFINE_UNQUOTED([ssh_get_server_publickey], [ssh_get_publickey], [ssh_get_publickey is deprecated and replaced by ssh_get_server_publickey.])]) + AC_CHECK_FUNC([ssh_session_is_known_server], + [], + [AC_DEFINE_UNQUOTED([ssh_session_is_known_server], [ssh_is_server_known], + [ssh_is_server_known is deprecated and replaced by ssh_session_is_known_server.])]) + AC_CHECK_TYPES([enum ssh_known_hosts_e], + [AC_DEFINE([HAVE_SSH_KNOWN_HOSTS_E], [1], + [Defined if enum ssh_known_hosts_e exists in libssh.h])], + [], [[#include <libssh/libssh.h>]]) CFLAGS="$old_CFLAGS" LIBS="$old_LIBS" fi -- 2.19.1
On Sat, 2018-11-24 at 03:52 +0800, Julio Faracco wrote:
This commit adds some checks inside libssh m4 checking to verify if ssh_session_is_known_server function is available. This new function scope replaces the old ssh_is_server_known() from libssh 0.8.0 and below versions.
Another auxiliar enumerator was added to keep the compatibility with the old standard used by ssh_is_server_known() function.
You only have the check for the enum, not the definition of the replacement one here. I guess you want to squash patch 2 in here? [...]
+ AC_CHECK_FUNC([ssh_session_is_known_server], + [], + [AC_DEFINE_UNQUOTED([ssh_session_is_known_server], [ssh_is_server_known], + [ssh_is_server_known is deprecated and replaced by ssh_session_is_known_server.])]) + AC_CHECK_TYPES([enum ssh_known_hosts_e], + [AC_DEFINE([HAVE_SSH_KNOWN_HOSTS_E], [1], + [Defined if enum ssh_known_hosts_e exists in libssh.h])], + [], [[#include <libssh/libssh.h>]])
We check for the availability of the new function and the enum containing its possible return values separately: is there a chance we might find one but not the other? I'm hoping not, but it would be nice if you could confirm that's indeed never going to happen, as it would be quite problematic if it did... -- Andrea Bolognani / Red Hat / Virtualization
After version 0.8.0, libssh deprecated the function scope ssh_is_server_known() and moved to ssh_session_is_known_server(). So, libvirt is failing to compile using this new function name. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/rpc/virnetlibsshsession.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 7c5f158f4d..eb6d6843a2 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -80,6 +80,22 @@ struct _virNetLibsshAuthMethod { int tries; }; +#ifndef HAVE_SSH_KNOWN_HOSTS_E +/* This is an auxiliar enum to help libssh migration to version 0.8.0 + * or higher. This enum associates the enumerator ssh_server_known_e + * with new ssh_known_hosts_e enum. In other words, it can be removed + * in the future. ERROR was moved from -1 to -2 and NOT_FOUND from 4 + * to -1. */ +enum _vir_ssh_known_hosts_e { + SSH_KNOWN_HOSTS_ERROR = SSH_SERVER_ERROR, + SSH_KNOWN_HOSTS_UNKNOWN = SSH_SERVER_NOT_KNOWN, + SSH_KNOWN_HOSTS_OK, + SSH_KNOWN_HOSTS_CHANGED, + SSH_KNOWN_HOSTS_OTHER, + SSH_KNOWN_HOSTS_NOT_FOUND, +}; +#endif + struct _virNetLibsshSession { virObjectLockable parent; virNetLibsshSessionState state; -- 2.19.1
On Sat, 2018-11-24 at 03:52 +0800, Julio Faracco wrote:
After version 0.8.0, libssh deprecated the function scope ssh_is_server_known() and moved to ssh_session_is_known_server(). So, libvirt is failing to compile using this new function name.
Based on the commit message, I imagine this patch was supposed to look a lot like patch 4 but the corresponding hunk somehow got lost, and without it the whole thing doesn't make a whole lot of sense :) [...]
+#ifndef HAVE_SSH_KNOWN_HOSTS_E +/* This is an auxiliar enum to help libssh migration to version 0.8.0 + * or higher. This enum associates the enumerator ssh_server_known_e + * with new ssh_known_hosts_e enum. In other words, it can be removed + * in the future. ERROR was moved from -1 to -2 and NOT_FOUND from 4 + * to -1. */
The specific values are irrelevant, all we care about is that the new function returns values from a new enum and the old one from the old enum, so we need to create a bit of compatibility gunk ourselves in order for internal callers not to care which version of libssh we're building against; in the same way, the fact that we're going to be able to drop the compatibility is not very interesting if the condition for dropping it is omitted. Please reword the comment accordingly.
+enum _vir_ssh_known_hosts_e {
Since we only get here if libssh's own enum is not present, I don't see why we wouldn't just use ssh_known_hosts_e here.
+ SSH_KNOWN_HOSTS_ERROR = SSH_SERVER_ERROR, + SSH_KNOWN_HOSTS_UNKNOWN = SSH_SERVER_NOT_KNOWN, + SSH_KNOWN_HOSTS_OK, + SSH_KNOWN_HOSTS_CHANGED, + SSH_KNOWN_HOSTS_OTHER, + SSH_KNOWN_HOSTS_NOT_FOUND, +}; +#endif
Okay, this works, but it's not extremely clear... I would very much prefer if you explicitly assigned values taken from the old enum to the new enum every single time instead of just for the first two. Looks good if you address the comments above and squash it into the previous patch. -- Andrea Bolognani / Red Hat / Virtualization
This commit adds some checks inside libssh m4 checking to verify if ssh_session_update_known_hosts function is available. This new function scope replaces the old ssh_write_knownhost() from libssh 0.8.0 and below versions. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- m4/virt-libssh.m4 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/m4/virt-libssh.m4 b/m4/virt-libssh.m4 index 52bd4d3639..f6d307f8d1 100644 --- a/m4/virt-libssh.m4 +++ b/m4/virt-libssh.m4 @@ -41,6 +41,10 @@ AC_DEFUN([LIBVIRT_CHECK_LIBSSH],[ [AC_DEFINE([HAVE_SSH_KNOWN_HOSTS_E], [1], [Defined if enum ssh_known_hosts_e exists in libssh.h])], [], [[#include <libssh/libssh.h>]]) + AC_CHECK_FUNC([ssh_session_update_known_hosts], + [], + [AC_DEFINE_UNQUOTED([ssh_session_update_known_hosts], [ssh_write_knownhost], + [ssh_write_knownhost is deprecated and replaced by ssh_session_update_known_hosts.])]) CFLAGS="$old_CFLAGS" LIBS="$old_LIBS" fi -- 2.19.1
On Sat, 2018-11-24 at 03:52 +0800, Julio Faracco wrote: [...]
+ AC_CHECK_FUNC([ssh_session_update_known_hosts], + [], + [AC_DEFINE_UNQUOTED([ssh_session_update_known_hosts], [ssh_write_knownhost], + [ssh_write_knownhost is deprecated and replaced by ssh_session_update_known_hosts.])])
Looks good. -- Andrea Bolognani / Red Hat / Virtualization
After version 0.8.0, libssh deprecated the function scope ssh_write_knownhost() and moved to ssh_session_update_known_hosts(). So, libvirt is failing to compile using this new function name. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/rpc/virnetlibsshsession.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index eb6d6843a2..5a7ca04dec 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -397,7 +397,7 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess) /* write the host key file, if specified */ if (sess->knownHostsFile) { - if (ssh_write_knownhost(sess->session) < 0) { + if (ssh_session_update_known_hosts(sess->session) < 0) { errmsg = ssh_get_error(sess->session); virReportError(VIR_ERR_LIBSSH, _("failed to write known_host file '%s': %s"), -- 2.19.1
On Sat, 2018-11-24 at 03:52 +0800, Julio Faracco wrote: [...]
/* write the host key file, if specified */ if (sess->knownHostsFile) { - if (ssh_write_knownhost(sess->session) < 0) { + if (ssh_session_update_known_hosts(sess->session) < 0) { errmsg = ssh_get_error(sess->session); virReportError(VIR_ERR_LIBSSH, _("failed to write known_host file '%s': %s"),
Looks good. -- Andrea Bolognani / Red Hat / Virtualization
On Sat, 2018-11-24 at 03:52 +0800, Julio Faracco wrote:
After 0.8.0 release, libssh deprecated some functions like: ssh_is_server_known() and ssh_write_knownhost().
This is not accurate: 0.8.5 was released less than two months ago and does not consider any of the functions you listed as deprecated; the deprecation is indeed in place on the master branch, which will eventually be released as 0.9.0 or whatever. Speaking of which, I'm not entirely convinced being *this* proactive with respect to libssh deprecations is useful when we could at least wait for the next stable release to be out. I'll review the patches in more detail regardless :) -- Andrea Bolognani / Red Hat / Virtualization
I agree about versioning and stability. The idea behind the patches is just predict the future problems related to libssh. Specially to discuss changes associated with deprecated functions. And also specially because we have several Linux OSes keeping its own package versioning. We would not like to break anything. I will split this serie and wait for 0.9.0 stable release. Thanks to review, Andrea. ;-) Em ter, 18 de dez de 2018 às 12:01, Andrea Bolognani <abologna@redhat.com> escreveu:
On Sat, 2018-11-24 at 03:52 +0800, Julio Faracco wrote:
After 0.8.0 release, libssh deprecated some functions like: ssh_is_server_known() and ssh_write_knownhost().
This is not accurate: 0.8.5 was released less than two months ago and does not consider any of the functions you listed as deprecated; the deprecation is indeed in place on the master branch, which will eventually be released as 0.9.0 or whatever.
Speaking of which, I'm not entirely convinced being *this* proactive with respect to libssh deprecations is useful when we could at least wait for the next stable release to be out. I'll review the patches in more detail regardless :)
-- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Julio Faracco