From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47857) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buMiK-00040A-5F for qemu-devel@nongnu.org; Wed, 12 Oct 2016 12:49:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buMiH-0008UV-Qq for qemu-devel@nongnu.org; Wed, 12 Oct 2016 12:49:07 -0400 MIME-Version: 1.0 In-Reply-To: <20161012155134.GO5544@noname.redhat.com> References: <1476171437-11830-1-git-send-email-ashijeetacharya@gmail.com> <1476171437-11830-3-git-send-email-ashijeetacharya@gmail.com> <20161012155134.GO5544@noname.redhat.com> From: Ashijeet Acharya Date: Wed, 12 Oct 2016 22:19:04 +0530 Message-ID: Content-Type: text/plain; charset=UTF-8 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: Kevin Wolf Cc: "Richard W.M. Jones" , jcody@redhat.com, Eric Blake , mreitz@redhat.com, armbru@redhat.com, QEMU Developers , qemu-block@nongnu.org On Wed, Oct 12, 2016 at 9:21 PM, Kevin Wolf wrote: > 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). Okay, I will use that. > >> { >> 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. Hmm... I will remove it. > >> + 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? In InetSocketAddress 'port' is of the type 'char*' but now I think using qobject_input_visitor_new_autocast() will be right since there are other fields as well. > >> + 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'. Yes, this is what I mentioned above. > > 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). Okay, I will convert the driver to use one of the options you suggested above. > >> /* 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. Nice! I will try to post v2 soon. Ashijeet > > Kevin