Re: [Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it
On Tue, Oct 11, 2016 at 1:07 PM, Ashijeet Acharyawrote: > 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 > --- > 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
Re: [Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it
On Wed, Oct 12, 2016 at 9:21 PM, Kevin Wolfwrote: > 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 >> --- >> 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, , "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, , _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(_runtime_opts, NULL, 0, _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
Re: [Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it
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> --- > 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, , "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, , _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(_runtime_opts, NULL, 0, _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, >
[Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it
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--- 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; +} 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, , "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, , _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(_runtime_opts, NULL, 0, _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