| Message ID | 1476171437-11830-3-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <ashijeetacharya@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). > { > 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
On Wed, Oct 12, 2016 at 9:21 PM, Kevin Wolf <kwolf@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 <ashijeetacharya@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
On Tue, Oct 11, 2016 at 1:07 PM, Ashijeet Acharya <ashijeetacharya@gmail.com> 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 | 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)) > { > 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; > + } > + > + if (!host) { > + error_setg(errp, "No hostname was specified"); > + return false; I was debugging this part with gdb while making the changes for v2, and I hit something very strange. The code always gives the error of "No hostname was specified". To be more clear, I reverted back to original driver state and the problem did not seem to appear for the same qemu command line and I can't find the bug. Command I used: $ ./bin/qemu-system-x86_64 -m 512 -drive file=ssh://ashijeet@127.0.0.1/home/ashijeet/qemu_build/test.qcow2, if=virtio Is there something wrong with the command line? Ashijeet
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)) { 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; + } + + 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); + 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); /* 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; }
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 | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 78 insertions(+), 9 deletions(-)