[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it |
Date: |
Wed, 12 Oct 2016 17:51:34 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> ---
> 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=<service-name>).
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
- [Qemu-devel] [PATCH 0/4] Allow blockdev-add for SSH, Ashijeet Acharya, 2016/10/11
- [Qemu-devel] [PATCH 1/4] block/ssh: Add ssh_has_filename_options_conflict(), Ashijeet Acharya, 2016/10/11
- [Qemu-devel] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it, Ashijeet Acharya, 2016/10/11
- [Qemu-devel] [PATCH 3/4] block/ssh: Use InetSocketAddress options, Ashijeet Acharya, 2016/10/11
- [Qemu-devel] [PATCH 4/4] qapi: allow blockdev-add for ssh, Ashijeet Acharya, 2016/10/11
- Re: [Qemu-devel] [PATCH 0/4] Allow blockdev-add for SSH, no-reply, 2016/10/11
- Re: [Qemu-devel] [PATCH 0/4] Allow blockdev-add for SSH, Ashijeet Acharya, 2016/10/12
- Re: [Qemu-devel] [PATCH 0/4] Allow blockdev-add for SSH, Ashijeet Acharya, 2016/10/12