From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwAE2-0005IQ-Oy for qemu-devel@nongnu.org; Mon, 17 Oct 2016 11:53:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwAE1-0004Im-OC for qemu-devel@nongnu.org; Mon, 17 Oct 2016 11:53:18 -0400 Date: Mon, 17 Oct 2016 17:53:07 +0200 From: Kevin Wolf Message-ID: <20161017155307.GH4821@noname.redhat.com> References: <1476522280-23211-1-git-send-email-ashijeetacharya@gmail.com> <1476522280-23211-3-git-send-email-ashijeetacharya@gmail.com> <20161017125724.GG4821@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [v2 2/5] block/ssh: Add InetSocketAddress and accept it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ashijeet Acharya Cc: Max Reitz , "Richard W.M. Jones" , jcody@redhat.com, Eric Blake , Markus Armbruster , "Daniel P. Berrange" , Gerd Hoffmann , Paolo Bonzini , QEMU Developers , qemu-block@nongnu.org Am 17.10.2016 um 17:44 hat Ashijeet Acharya geschrieben: > On Mon, Oct 17, 2016 at 6:27 PM, Kevin Wolf wrote: > > Am 17.10.2016 um 14:33 hat Ashijeet Acharya geschrieben: > >> On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz 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 > >> >> --- > >> >> 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. Kevin > 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