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

[Ashijeet Acharya]

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?

Ashijeet
> Alternatively it could be possible to use getservbyname() to get the
> port number from the name, but maybe that's a bit too much for a feature
> that most people don't even know of.
>
> I'm also not completely opposed to simply requiring a numeric argument
> for SSH. There is no real use to support service names here other than
> being consistent with other places in qemu.
>
> Kevin