From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52281) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bvhCU-0004xb-Np for qemu-devel@nongnu.org; Sun, 16 Oct 2016 04:53:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bvhCT-0005hU-2T for qemu-devel@nongnu.org; Sun, 16 Oct 2016 04:53:46 -0400 MIME-Version: 1.0 In-Reply-To: References: <1476522280-23211-1-git-send-email-ashijeetacharya@gmail.com> <1476522280-23211-3-git-send-email-ashijeetacharya@gmail.com> From: Ashijeet Acharya Date: Sun, 16 Oct 2016 14:23:43 +0530 Message-ID: Content-Type: text/plain; charset=UTF-8 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: Max Reitz Cc: Kevin Wolf , "Richard W.M. Jones" , jcody@redhat.com, Eric Blake , Markus Armbruster , "Daniel P. Berrange" , Gerd Hoffmann , Paolo Bonzini , QEMU Developers , qemu-block@nongnu.org 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(-) >> >> diff --git a/block/ssh.c b/block/ssh.c >> index 75cb7bc..3b18907 100644 >> --- a/block/ssh.c >> +++ b/block/ssh.c >> @@ -30,10 +30,14 @@ >> #include "block/block_int.h" >> #include "qapi/error.h" >> #include "qemu/error-report.h" >> +#include "qemu/cutils.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/qobject-input-visitor.h" >> +#include "qapi/qobject-output-visitor.h" >> >> /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in >> * this block driver code. >> @@ -74,6 +78,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 +269,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") || > > I know I've done the same in my series, but I'll actually drop this > condition from the next version; I'm not actually handling the case > where the destination address is not flattened, and neither are you > (we're both using qdict_extract_subqdict() instead of testing whether > "server" is an object on its own), so I think you should drop it as well > and just test for the prefix. > > It doesn't harm to test for "server" itself, but I think it's a bit > confusing still, since you (we) are not dealing with that possibility > anywhere else. Yes, I have dropped this. >> + !strstart(qe->key, "server.", NULL)) > > It should be just "strstart", not "!strstart", because the function > returns 1 if the prefix matches. Fixed. > >> { >> error_setg(errp, "Option '%s' cannot be used with a file name", >> qe->key); >> @@ -555,13 +563,66 @@ 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; >> + } else { >> + qdict_put(output_opts, "server.host", qstring_from_str(host)); > > This will segfault if host is NULL. You shouldn't go into this branch at > all if host is not set. Yes, I have put this in a different 'if' like: if (host) { qdict_put(); ... } > >> + 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 = qobject_input_visitor_new_autocast(crumpled_addr, true, 1, true); > > In contrast to what Kevin said in v1, I think you do not want to use > autocast here. > > Or, to be more specific, it's difficult. The thing is that the autocast > documentation says: "Any scalar values in the @obj input data structure > should always be represented as strings". > > So if you do use the autocast version, command line works great because > from there everything comes as a string. But blockdev-add no longer > works because from there everything comes with the correct type (and you > cannot give it the wrong type). Case in point, this happens if you try > to create an SSH BDS with "'ipv4': true": > > {"error": {"class": "GenericError", "desc": "Invalid parameter type for > 'ipv4', expected: string"}} > > OK, let's try "'ipv4': 'true'" then: > > {"error": {"class": "GenericError", "desc": "Invalid parameter type for > 'ipv4', expected: boolean"}} > > Too bad. The former is a message from the visit_type_InetSocketAddress() > call below, the latter from the QMP parsing code. > > In contrast, if you do not use the autocast version, blockdev-add will > work just fine, but you can no longer specify non-string values from the > command line. > > I don't think this is your problem, though. There should be a way for > the command line options to be converted to the correct types while we > continue to use strict type-checking for blockdev-add. > > Therefore, I think you'll have to sacrifice one or the other here. All > of the non-string options are optional, so it won't be too bad in any case. > > As I've said, I would choose the non-autocast version since I think it > is more important to get this new parameter completely working with > blockdev-add than it is to get it fully working with the command line, > for two reasons: > > First, in theory one should be able to emulate everything on the command > line with QMP anyway (e.g. emulate -drive through blockdev-add and maybe > drive_add). Therefore, if you want to do something on the command line > but it doesn't work, you can (in theory...) always fall back to QMP. It > doesn't work the other way around, though. > > Second, I don't think command line users will actually use the new > syntax anyway. It's mostly for blockdev-add. And even if they do, they > can probably live without @to, @ipv4 and @ipv6. > I thought the same when I saw your patches and tried to guess the same reason behind you using qobject_input_visitor_new(). Surely blockdev-add seems to be the priority here I believe. I will change 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 +633,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 +660,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) { > > You need to set ret here (at least my gcc complains about this). > >> + 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->inet->port is an arbitrary string, so this is not guaranteed to work. > For instance, it can be "ssh" or "http". This will return 0 in that case. > >> + s->hostport = g_strdup_printf("%s:%d", s->inet->host, port); > > In order to fix at least this issue here you should pull the part of > patch 3 that makes inet_connect_saddr() public in front of this patch > and squash the rest into this patch here. Yes I have reordered the patches. > >> >> /* 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... > Will using strtol() do any good? > So maybe the best is to keep it this way and put a FIXME above the > atoi() call. :-/ Yeah, that will be my last option. Ashijeet > Max >