Re: [Qemu-devel] [v2 2/5] block/ssh: Add InetSocketAddress and accept it

[Ashijeet Acharya]

On Mon, Oct 17, 2016 at 9:23 PM, Kevin Wolf <address@hidden> wrote:
> Am 17.10.2016 um 17:44 hat Ashijeet Acharya geschrieben:
>> On Mon, Oct 17, 2016 at 6:27 PM, Kevin Wolf <address@hidden> wrote:
>> > Am 17.10.2016 um 14:33 hat Ashijeet Acharya geschrieben:
>> >> On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz <address@hidden> wrote:
>> >> > On 15.10.2016 11:04, Ashijeet Acharya wrote:
>> >> >> Add InetSocketAddress compatibility to SSH driver.
>> >> >>
>> >> >> Add a new option "server" to the SSH block driver which then accepts
>> >> >> a InetSocketAddress.
>> >> >>
>> >> >> "host" and "port" are supported as legacy options and are mapped to
>> >> >> their InetSocketAddress representation.
>> >> >>
>> >> >> Signed-off-by: Ashijeet Acharya <address@hidden>
>> >> >> ---
>> >> >>  block/ssh.c | 83 
>> >> >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>> >> >>  1 file changed, 74 insertions(+), 9 deletions(-)
>> >> >>
>> >> >>
>> >> >>      /* Open the socket and connect. */
>> >> >>      s->sock = inet_connect(s->hostport, errp);
>> >> >> @@ -634,7 +698,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
>> >> >> *options,
>> >> >>      }
>> >> >>
>> >> >>      /* Check the remote host's key against known_hosts. */
>> >> >> -    ret = check_host_key(s, host, port, host_key_check, errp);
>> >> >> +    ret = check_host_key(s, s->inet->host, port, host_key_check,
>> >> >
>> >> > But then you're still using the port here... And I can't come up with a
>> >> > way (not even a bad one) to get the numeric port. Maybe interpret the
>> >> > addrinfo in inet_connect_saddr()? But getting that information out would
>> >> > be ugly, if even possible...
>> >> >
>> >> > So maybe the best is to keep it this way and put a FIXME above the
>> >> > atoi() call. :-/
>> >>
>> >> Kevin, I believe (after talking with Max) that regarding the atoi()
>> >> issue, I can't use any string to integer function since it won't
>> >> succeed for cases like port = 'ssh' and putting a FIXME over it seems
>> >> to be the only option. But Max did warn me, though, to get everybody's
>> >> opinion before I do so. So I am awaiting your response on this one.
>> >> Much better will be if you have a workaround solution in mind!! :-)
>> >
>> > The integer port is only needed for libssh2_knownhost_checkp(). One
>> > option could be to consider passing -1 instead:
>> >
>> >     port is the port number used by the host (or a negative number to
>> >     check the generic host). If the port number is given, libssh2 will
>> >     check the key for the specific host + port number combination in
>> >     addition to the plain host name only check.
>> >
>> > In 99% of the cases, this shouldn't make any difference.
>>
>> I think, its better to keep using atoi() and check if it returns a '0'
>>  value and display the error to the user to give the input as numeric.
>> This is possible since this will not clash with the possibility that
>> user gives the port input as port = '0' for no such port number exists
>> as far as I know. Will this work?
>
> It's fair enough. We will have a little inconsistency between ssh and
> other users of SocketAddress, but the driver never supported service
> names, so it isn't a regression either.

Great, I will do this change and post the patches shortly.

Ashijeet
>
> Kevin