On Wed, Oct 12, 2016 at 9:21 PM, Kevin Wolf <kw...@redhat.com> 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 <ashijeetacha...@gmail.com>
>> ---
>> 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=<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).
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
