From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59688) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buLom-0001z7-2I for qemu-devel@nongnu.org; Wed, 12 Oct 2016 11:51:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buLoj-0005Gn-Hg for qemu-devel@nongnu.org; Wed, 12 Oct 2016 11:51:43 -0400 Date: Wed, 12 Oct 2016 17:51:34 +0200 From: Kevin Wolf Message-ID: <20161012155134.GO5544@noname.redhat.com> References: <1476171437-11830-1-git-send-email-ashijeetacharya@gmail.com> <1476171437-11830-3-git-send-email-ashijeetacharya@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1476171437-11830-3-git-send-email-ashijeetacharya@gmail.com> Subject: Re: [Qemu-devel] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ashijeet Acharya Cc: rjones@redhat.com, jcody@redhat.com, eblake@redhat.com, mreitz@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben: > 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 | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 78 insertions(+), 9 deletions(-) > > diff --git a/block/ssh.c b/block/ssh.c > index 75cb7bc..702871a 100644 > --- a/block/ssh.c > +++ b/block/ssh.c > @@ -32,8 +32,11 @@ > #include "qemu/error-report.h" > #include "qemu/sockets.h" > #include "qemu/uri.h" > +#include "qapi-visit.h" > #include "qapi/qmp/qint.h" > #include "qapi/qmp/qstring.h" > +#include "qapi/qmp-input-visitor.h" > +#include "qapi/qmp-output-visitor.h" > > /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in > * this block driver code. > @@ -74,6 +77,8 @@ typedef struct BDRVSSHState { > */ > LIBSSH2_SFTP_ATTRIBUTES attrs; > > + InetSocketAddress *inet; > + > /* Used to warn if 'flush' is not supported. */ > char *hostport; > bool unsafe_flush_warning; > @@ -263,7 +268,9 @@ static bool ssh_has_filename_options_conflict(QDict *options, Error **errp) > !strcmp(qe->key, "port") || > !strcmp(qe->key, "path") || > !strcmp(qe->key, "user") || > - !strcmp(qe->key, "host_key_check")) > + !strcmp(qe->key, "host_key_check") || > + !strcmp(qe->key, "server") || > + !strncmp(qe->key, "server.", 7)) There is strstart() from cutils.c which looks a bit nicer (you don't have to specify the string length then). > { > error_setg(errp, "Option '%s' cannot be used with a file name", > qe->key); > @@ -555,13 +562,71 @@ static QemuOptsList ssh_runtime_opts = { > }, > }; > > +static bool ssh_process_legacy_socket_options(QDict *output_opts, > + QemuOpts *legacy_opts, > + Error **errp) > +{ > + const char *host = qemu_opt_get(legacy_opts, "host"); > + const char *port = qemu_opt_get(legacy_opts, "port"); > + > + if (!host && port) { > + error_setg(errp, "port may not be used without host"); > + return false; > + } This check is rather pointless if !host causes an error anyway. > + if (!host) { > + error_setg(errp, "No hostname was specified"); > + return false; > + } else { > + qdict_put(output_opts, "server.host", qstring_from_str(host)); > + qdict_put(output_opts, "server.port", > + qstring_from_str(port ?: stringify(22))); > + } > + > + return true; > +} > + > +static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options, > + Error **errp) > +{ > + InetSocketAddress *inet = NULL; > + QDict *addr = NULL; > + QObject *crumpled_addr = NULL; > + Visitor *iv = NULL; > + Error *local_error = NULL; > + > + qdict_extract_subqdict(options, &addr, "server."); > + if (!qdict_size(addr)) { > + error_setg(errp, "SSH server address missing"); > + goto out; > + } > + > + crumpled_addr = qdict_crumple(addr, true, errp); > + if (!crumpled_addr) { > + goto out; > + } > + > + iv = qmp_input_visitor_new(crumpled_addr, true); I believe you need qobject_input_visitor_new_autocast() here. Do integer properties like port work for you without it? > + visit_type_InetSocketAddress(iv, NULL, &inet, &local_error); > + if (local_error) { > + error_propagate(errp, local_error); > + goto out; > + } > + > +out: > + QDECREF(addr); > + qobject_decref(crumpled_addr); > + visit_free(iv); > + return inet; > +} > + > static int connect_to_ssh(BDRVSSHState *s, QDict *options, > int ssh_flags, int creat_mode, Error **errp) > { > int r, ret; > QemuOpts *opts = NULL; > Error *local_err = NULL; > - const char *host, *user, *path, *host_key_check; > + const char *user, *path, *host_key_check; > int port; > > opts = qemu_opts_create(&ssh_runtime_opts, NULL, 0, &error_abort); > @@ -572,15 +637,11 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, > goto err; > } > > - host = qemu_opt_get(opts, "host"); > - if (!host) { > + if (!ssh_process_legacy_socket_options(options, opts, errp)) { > ret = -EINVAL; > - error_setg(errp, "No hostname was specified"); > goto err; > } > > - port = qemu_opt_get_number(opts, "port", 22); > - > path = qemu_opt_get(opts, "path"); > if (!path) { > ret = -EINVAL; > @@ -603,9 +664,16 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, > host_key_check = "yes"; > } > > + /* Pop the config into our state object, Exit if invalid */ > + s->inet = ssh_config(s, options, errp); > + if (!s->inet) { > + goto err; > + } > + > /* Construct the host:port name for inet_connect. */ > g_free(s->hostport); > - s->hostport = g_strdup_printf("%s:%d", host, port); > + port = atoi(s->inet->port); > + s->hostport = g_strdup_printf("%s:%d", s->inet->host, port); Oh, it isn't even an integer. I never realised that we support things like 'port=ssh', but apparently we do! That explains why your testing didn't show the need for the other visitor type. You'd still see it for 'to', 'ipv4' and 'ipv6'. Anyway, I believe that constructing a string here is backwards (and doing atoi() first before converting it back to a string would actually break port=). The choices that I see is making inet_connect_saddr() public, which directly takes the InetSocketAddress, or using QIOChannelSocket like NBD (though that looks like a larger change). > /* Open the socket and connect. */ > s->sock = inet_connect(s->hostport, errp); > @@ -634,7 +702,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, > + errp); > if (ret < 0) { > goto err; > } The rest of the patch looks good to me. Kevin