[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 2/4] rpc: replacing ssh_is_server_known() by ssh_session_is_known_server().



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]