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

[Ashijeet Acharya <ashijeetacharya@gmail.com>]

On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz <mreitz@redhat.com> 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 <ashijeetacharya@gmail.com>
>> ---
>>  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
>