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

[Max Reitz]

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 <address@hidden>
> ---
>  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.

> +            !strstart(qe->key, "server.", NULL))

It should be just "strstart", not "!strstart", because the function
returns 1 if the prefix matches.

>          {
>              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.

> +        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.

> +    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.

>  
>      /* 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. :-/

Max

> +                         errp);
>      if (ret < 0) {
>          goto err;
>      }
>

signature.asc
Description: OpenPGP digital signature