* [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH
@ 2016-10-25 13:03 Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 1/5] block/ssh: Add ssh_has_filename_options_conflict() Ashijeet Acharya
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Ashijeet Acharya @ 2016-10-25 13:03 UTC (permalink / raw)
To: kwolf
Cc: rjones, jcody, mreitz, eblake, berrange, pbonzini, qemu-devel,
qemu-block, Ashijeet Acharya
Previously posted series patches:
v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg03781.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg03403.html
v1: http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg02137.html
This series adds blockdev-add support for SSH block driver.
Patch 1 prepares the code for the addition of a new option prefix,
which is "server.". This is accomplished by adding a
ssh_has_filename_options_conflict() function which helps to iterate
over the various options and check for conflict.
Patch 2 makes inet_connect_saddr() in util/qemu-sockets.c public.
Patch 3 first adds InetSocketAddress compatibility to SSH block driver
and then makes it accept a InetSocketAddress under the "server" option.
The old options "host" and "port" are supported as legacy options and
then translated to the respective InetSocketAddress representation.
Patch 4 drops the usage of "host" and "port" outside of
ssh_has_filename_options_conflict() and
ssh_process_legacy_socket_options() functions in order to make them
legacy options completely.
Patch 5 helps to allow blockdev-add support for the SSH block driver
by making the SSH option available.
*** This series depends on the following patch: ***
"qdict: implement a qdict_crumple method for un-flattening a dict"
from Daniel's "QAPI/QOM work for non-scalar object properties"
series.
Changes in v4:
- remove hostport from BDRVSSHState and use s->inet->host in warning message
Changes in v3:
- reorder patch 2 and 3 from v2 (Max)
- fix coding-style issue in patch 2 (Max)
- drop testing to check for "server" as an object itself (Max)
- fix strstart() bug (Max)
- fix a segfault bug when host gets set to NULL (Max)
- revert back to using qobject_input_visitor_new() (Max & Kevin)
- use qemu_strtol() instead of atoi() for better error handling (Kevin)
- make @user an optional argument in qapi/block-core.json (Max)
- update documentation for BlockdevOptionsSsh (Max)
Changes in v2:
- Use strstart() instead of strcmp() (Kevin)
- Use qobject_input_visitor_new_autocast() instead of
qmp_input_visitor_new() and change header files accordingly (Kevin)
- Use inet_connect_saddr() instead of inet_connect() (Kevin)
- Drop the contruction of <host>:<port> string (Kevin)
- Fix the bug in ssh_process_legacy_socket_options()
Ashijeet Acharya (5):
block/ssh: Add ssh_has_filename_options_conflict()
util/qemu-sockets: Make inet_connect_saddr() public
block/ssh: Add InetSocketAddress and accept it
block/ssh: Use InetSocketAddress options
qapi: allow blockdev-add for ssh
block/ssh.c | 132 +++++++++++++++++++++++++++++++++++++++----------
include/qemu/sockets.h | 2 +
qapi/block-core.json | 26 +++++++++-
util/qemu-sockets.c | 4 +-
4 files changed, 135 insertions(+), 29 deletions(-)
--
2.6.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v4 1/5] block/ssh: Add ssh_has_filename_options_conflict()
2016-10-25 13:03 [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Ashijeet Acharya
@ 2016-10-25 13:03 ` Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 2/5] util/qemu-sockets: Make inet_connect_saddr() public Ashijeet Acharya
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ashijeet Acharya @ 2016-10-25 13:03 UTC (permalink / raw)
To: kwolf
Cc: rjones, jcody, mreitz, eblake, berrange, pbonzini, qemu-devel,
qemu-block, Ashijeet Acharya
We have 5 options plus ("server") option which is added in the next
patch that conflict with specifying a SSH filename. We need to iterate
over all the options to check whether its key has an "server." prefix.
This iteration will help us adding the new option "server" easily.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/ssh.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index 5ce12b6..75cb7bc 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -254,15 +254,30 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
return -EINVAL;
}
+static bool ssh_has_filename_options_conflict(QDict *options, Error **errp)
+{
+ const QDictEntry *qe;
+
+ for (qe = qdict_first(options); qe; qe = qdict_next(options, qe)) {
+ if (!strcmp(qe->key, "host") ||
+ !strcmp(qe->key, "port") ||
+ !strcmp(qe->key, "path") ||
+ !strcmp(qe->key, "user") ||
+ !strcmp(qe->key, "host_key_check"))
+ {
+ error_setg(errp, "Option '%s' cannot be used with a file name",
+ qe->key);
+ return true;
+ }
+ }
+
+ return false;
+}
+
static void ssh_parse_filename(const char *filename, QDict *options,
Error **errp)
{
- if (qdict_haskey(options, "user") ||
- qdict_haskey(options, "host") ||
- qdict_haskey(options, "port") ||
- qdict_haskey(options, "path") ||
- qdict_haskey(options, "host_key_check")) {
- error_setg(errp, "user, host, port, path, host_key_check cannot be used at the same time as a file option");
+ if (ssh_has_filename_options_conflict(options, errp)) {
return;
}
--
2.6.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v4 2/5] util/qemu-sockets: Make inet_connect_saddr() public
2016-10-25 13:03 [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 1/5] block/ssh: Add ssh_has_filename_options_conflict() Ashijeet Acharya
@ 2016-10-25 13:03 ` Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 3/5] block/ssh: Add InetSocketAddress and accept it Ashijeet Acharya
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ashijeet Acharya @ 2016-10-25 13:03 UTC (permalink / raw)
To: kwolf
Cc: rjones, jcody, mreitz, eblake, berrange, pbonzini, qemu-devel,
qemu-block, Ashijeet Acharya
Make inet_connect_saddr() in util/qemu-sockets.c public in order to be
able to use it with InetSocketAddress sockets outside of
util/qemu-sockets.c independently.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
include/qemu/sockets.h | 2 ++
util/qemu-sockets.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 9eb2470..5589e68 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -34,6 +34,8 @@ typedef void NonBlockingConnectHandler(int fd, Error *err, void *opaque);
InetSocketAddress *inet_parse(const char *str, Error **errp);
int inet_connect(const char *str, Error **errp);
+int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
+ NonBlockingConnectHandler *callback, void *opaque);
NetworkAddressFamily inet_netfamily(int family);
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 4cef549..31f7fc6 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -412,8 +412,8 @@ static struct addrinfo *inet_parse_connect_saddr(InetSocketAddress *saddr,
* function succeeds, callback will be called when the connection
* completes, with the file descriptor on success, or -1 on error.
*/
-static int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
- NonBlockingConnectHandler *callback, void *opaque)
+int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
+ NonBlockingConnectHandler *callback, void *opaque)
{
Error *local_err = NULL;
struct addrinfo *res, *e;
--
2.6.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v4 3/5] block/ssh: Add InetSocketAddress and accept it
2016-10-25 13:03 [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 1/5] block/ssh: Add ssh_has_filename_options_conflict() Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 2/5] util/qemu-sockets: Make inet_connect_saddr() public Ashijeet Acharya
@ 2016-10-25 13:03 ` Ashijeet Acharya
2016-10-25 13:04 ` [Qemu-devel] [PATCH v4 4/5] block/ssh: Use InetSocketAddress options Ashijeet Acharya
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ashijeet Acharya @ 2016-10-25 13:03 UTC (permalink / raw)
To: kwolf
Cc: rjones, jcody, mreitz, eblake, berrange, pbonzini, qemu-devel,
qemu-block, Ashijeet Acharya
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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 82 insertions(+), 16 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index 75cb7bc..d814006 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,8 +78,9 @@ typedef struct BDRVSSHState {
*/
LIBSSH2_SFTP_ATTRIBUTES attrs;
+ InetSocketAddress *inet;
+
/* Used to warn if 'flush' is not supported. */
- char *hostport;
bool unsafe_flush_warning;
} BDRVSSHState;
@@ -89,7 +94,6 @@ static void ssh_state_init(BDRVSSHState *s)
static void ssh_state_free(BDRVSSHState *s)
{
- g_free(s->hostport);
if (s->sftp_handle) {
libssh2_sftp_close(s->sftp_handle);
}
@@ -263,7 +267,8 @@ 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") ||
+ strstart(qe->key, "server.", NULL))
{
error_setg(errp, "Option '%s' cannot be used with a file name",
qe->key);
@@ -555,14 +560,69 @@ 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) {
+ 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 = qobject_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;
- int port;
+ const char *user, *path, *host_key_check;
+ long port = 0;
opts = qemu_opts_create(&ssh_runtime_opts, NULL, 0, &error_abort);
qemu_opts_absorb_qdict(opts, options, &local_err);
@@ -572,15 +632,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,12 +659,21 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
host_key_check = "yes";
}
- /* Construct the host:port name for inet_connect. */
- g_free(s->hostport);
- s->hostport = g_strdup_printf("%s:%d", host, port);
+ /* Pop the config into our state object, Exit if invalid */
+ s->inet = ssh_config(s, options, errp);
+ if (!s->inet) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ if (qemu_strtol(s->inet->port, NULL, 10, &port) < 0) {
+ error_setg(errp, "Use only numeric port value");
+ ret = -EINVAL;
+ goto err;
+ }
/* Open the socket and connect. */
- s->sock = inet_connect(s->hostport, errp);
+ s->sock = inet_connect_saddr(s->inet, errp, NULL, NULL);
if (s->sock < 0) {
ret = -EIO;
goto err;
@@ -634,7 +699,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;
}
@@ -1055,7 +1121,7 @@ static void unsafe_flush_warning(BDRVSSHState *s, const char *what)
{
if (!s->unsafe_flush_warning) {
error_report("warning: ssh server %s does not support fsync",
- s->hostport);
+ s->inet->host);
if (what) {
error_report("to support fsync, you need %s", what);
}
--
2.6.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v4 4/5] block/ssh: Use InetSocketAddress options
2016-10-25 13:03 [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Ashijeet Acharya
` (2 preceding siblings ...)
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 3/5] block/ssh: Add InetSocketAddress and accept it Ashijeet Acharya
@ 2016-10-25 13:04 ` Ashijeet Acharya
2016-10-25 13:04 ` [Qemu-devel] [PATCH v4 5/5] qapi: allow blockdev-add for ssh Ashijeet Acharya
2016-10-27 19:54 ` [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Kevin Wolf
5 siblings, 0 replies; 10+ messages in thread
From: Ashijeet Acharya @ 2016-10-25 13:04 UTC (permalink / raw)
To: kwolf
Cc: rjones, jcody, mreitz, eblake, berrange, pbonzini, qemu-devel,
qemu-block, Ashijeet Acharya
Drop the use of legacy options in favour of the InetSocketAddress
options.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/ssh.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index d814006..6e6966d 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -197,6 +197,7 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
{
URI *uri = NULL;
QueryParams *qp;
+ char *port_str;
int i;
uri = uri_parse(filename);
@@ -229,11 +230,11 @@ static int parse_uri(const char *filename, QDict *options, Error **errp)
qdict_put(options, "user", qstring_from_str(uri->user));
}
- qdict_put(options, "host", qstring_from_str(uri->server));
+ qdict_put(options, "server.host", qstring_from_str(uri->server));
- if (uri->port) {
- qdict_put(options, "port", qint_from_int(uri->port));
- }
+ port_str = g_strdup_printf("%d", uri->port ?: 22);
+ qdict_put(options, "server.port", qstring_from_str(port_str));
+ g_free(port_str);
qdict_put(options, "path", qstring_from_str(uri->path));
--
2.6.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v4 5/5] qapi: allow blockdev-add for ssh
2016-10-25 13:03 [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Ashijeet Acharya
` (3 preceding siblings ...)
2016-10-25 13:04 ` [Qemu-devel] [PATCH v4 4/5] block/ssh: Use InetSocketAddress options Ashijeet Acharya
@ 2016-10-25 13:04 ` Ashijeet Acharya
2016-10-25 19:58 ` Eric Blake
2016-10-27 19:54 ` [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Kevin Wolf
5 siblings, 1 reply; 10+ messages in thread
From: Ashijeet Acharya @ 2016-10-25 13:04 UTC (permalink / raw)
To: kwolf
Cc: rjones, jcody, mreitz, eblake, berrange, pbonzini, qemu-devel,
qemu-block, Ashijeet Acharya
Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
support blockdev-add for SSH network protocol driver. Use only 'struct
InetSocketAddress' since SSH only supports connection over TCP.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..689dc0a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1716,7 +1716,8 @@
'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
- 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
+ 'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
+ 'vvfat' ] }
##
# @BlockdevOptionsFile
@@ -1953,6 +1954,27 @@
'*vport': 'int',
'*segment': 'str' } }
+##
+# @BlockdevOptionsSsh
+#
+# @server: host address
+#
+# @path: path to the image on the host
+#
+# @user: #optional user as which to connect, defaults to current
+# local user name
+#
+# @host_key_check #optional defines how and what to check the host
+# key against, defaults to "yes"
+#
+# Since 2.8
+##
+{ 'struct': 'BlockdevOptionsSsh',
+ 'data': { 'server': 'InetSocketAddress',
+ 'path': 'str',
+ '*user': 'str',
+ '*host_key_check': 'str' } }
+
##
# @BlkdebugEvent
@@ -2281,7 +2303,7 @@
# TODO rbd: Wait for structured options
'replication':'BlockdevOptionsReplication',
# TODO sheepdog: Wait for structured options
-# TODO ssh: Should take InetSocketAddress for 'host'?
+ 'ssh': 'BlockdevOptionsSsh',
'tftp': 'BlockdevOptionsCurl',
'vdi': 'BlockdevOptionsGenericFormat',
'vhdx': 'BlockdevOptionsGenericFormat',
--
2.6.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/5] qapi: allow blockdev-add for ssh
2016-10-25 13:04 ` [Qemu-devel] [PATCH v4 5/5] qapi: allow blockdev-add for ssh Ashijeet Acharya
@ 2016-10-25 19:58 ` Eric Blake
2016-10-26 8:31 ` Kevin Wolf
0 siblings, 1 reply; 10+ messages in thread
From: Eric Blake @ 2016-10-25 19:58 UTC (permalink / raw)
To: Ashijeet Acharya, kwolf
Cc: rjones, jcody, mreitz, berrange, pbonzini, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]
On 10/25/2016 08:04 AM, Ashijeet Acharya wrote:
> Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
> support blockdev-add for SSH network protocol driver. Use only 'struct
> InetSocketAddress' since SSH only supports connection over TCP.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-core.json | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
Sorry for not noticing this when I finally replied to v4;
> +##
> +# @BlockdevOptionsSsh
> +#
> +# @server: host address
> +#
> +# @path: path to the image on the host
> +#
> +# @user: #optional user as which to connect, defaults to current
> +# local user name
> +#
> +# @host_key_check #optional defines how and what to check the host
> +# key against, defaults to "yes"
I still have reservations about this parameter. I think we have time to
fix it as followups during soft freeze if Kevin would rather get your
initial patches in now, if that's what it takes to meet soft freeze
deadlines, but I do not want to bake it into the actual 2.8 release
without addressing those concerns.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/5] qapi: allow blockdev-add for ssh
2016-10-25 19:58 ` Eric Blake
@ 2016-10-26 8:31 ` Kevin Wolf
2016-10-26 13:27 ` Eric Blake
0 siblings, 1 reply; 10+ messages in thread
From: Kevin Wolf @ 2016-10-26 8:31 UTC (permalink / raw)
To: Eric Blake
Cc: Ashijeet Acharya, rjones, jcody, mreitz, berrange, pbonzini,
qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]
Am 25.10.2016 um 21:58 hat Eric Blake geschrieben:
> On 10/25/2016 08:04 AM, Ashijeet Acharya wrote:
> > Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
> > support blockdev-add for SSH network protocol driver. Use only 'struct
> > InetSocketAddress' since SSH only supports connection over TCP.
> >
> > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/block-core.json | 26 ++++++++++++++++++++++++--
> > 1 file changed, 24 insertions(+), 2 deletions(-)
>
> Sorry for not noticing this when I finally replied to v4;
>
> > +##
> > +# @BlockdevOptionsSsh
> > +#
> > +# @server: host address
> > +#
> > +# @path: path to the image on the host
> > +#
> > +# @user: #optional user as which to connect, defaults to current
> > +# local user name
> > +#
> > +# @host_key_check #optional defines how and what to check the host
> > +# key against, defaults to "yes"
>
> I still have reservations about this parameter.
It doesn't seem to be as easy as an enum. The real thing is structured
because some we support colon-separated mode/key, like this:
host_key_check=md5:xx:yy:zz:...
The description for the real thing (to which we would have to
translate the existing string) seems to be something like this:
{ 'enum': 'HostKeyCheckMode', 'data': [ 'yes', 'no', 'md5', 'sha1' ] }
{ 'union': 'HostKeyCheck',
'base': {
'mode': 'HostKeyCheckMode',
},
'discriminator': 'mode',
'data': {
'yes': {},
'no': {},
'md5': { 'key': 'str' },
'sha1': { 'key': 'str' }
}
(Hm, are inline struct definitions even allowed for union branches, or
do we have to create a named type there?)
> I think we have time to fix it as followups during soft freeze if
> Kevin would rather get your initial patches in now, if that's what it
> takes to meet soft freeze deadlines, but I do not want to bake it into
> the actual 2.8 release without addressing those concerns.
We don't really have a soft freeze like before any more, we're rather
going into something that is still called soft freeze, but is really a
hard freeze in disguise. So I'm not sure if we can change this during
the freeze.
If we can't work it out this week, I'd drop host_key_check from the
schema and leave a TODO comment instead so that it could be added in the
2.9 timeframe.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/5] qapi: allow blockdev-add for ssh
2016-10-26 8:31 ` Kevin Wolf
@ 2016-10-26 13:27 ` Eric Blake
0 siblings, 0 replies; 10+ messages in thread
From: Eric Blake @ 2016-10-26 13:27 UTC (permalink / raw)
To: Kevin Wolf
Cc: Ashijeet Acharya, rjones, jcody, mreitz, berrange, pbonzini,
qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]
On 10/26/2016 03:31 AM, Kevin Wolf wrote:
>>> +# @host_key_check #optional defines how and what to check the host
>>> +# key against, defaults to "yes"
>>
>> I still have reservations about this parameter.
>
> It doesn't seem to be as easy as an enum. The real thing is structured
> because some we support colon-separated mode/key, like this:
>
> host_key_check=md5:xx:yy:zz:...
>
> The description for the real thing (to which we would have to
> translate the existing string) seems to be something like this:
>
> { 'enum': 'HostKeyCheckMode', 'data': [ 'yes', 'no', 'md5', 'sha1' ] }
>
> { 'union': 'HostKeyCheck',
> 'base': {
> 'mode': 'HostKeyCheckMode',
> },
> 'discriminator': 'mode',
> 'data': {
> 'yes': {},
> 'no': {},
> 'md5': { 'key': 'str' },
> 'sha1': { 'key': 'str' }
> }
Okay, that does look like a better definition.
>
> (Hm, are inline struct definitions even allowed for union branches, or
> do we have to create a named type there?)
I have a patch for making them inline struct definitions, but it
probably won't be in 2.8 unless it solves problems that we can't work
around in other ways, due to Markus' review backlog on other qapi
patches. Creating a named type now may be verbose, but we could always
simplify later with an inline struct without breaking back-compat once
my patch does make it in.
>
>> I think we have time to fix it as followups during soft freeze if
>> Kevin would rather get your initial patches in now, if that's what it
>> takes to meet soft freeze deadlines, but I do not want to bake it into
>> the actual 2.8 release without addressing those concerns.
>
> We don't really have a soft freeze like before any more, we're rather
> going into something that is still called soft freeze, but is really a
> hard freeze in disguise. So I'm not sure if we can change this during
> the freeze.
>
> If we can't work it out this week, I'd drop host_key_check from the
> schema and leave a TODO comment instead so that it could be added in the
> 2.9 timeframe.
That's also a reasonable idea. Better to avoid a rush that ends up
baking in an API we can't support, even if the initial release is less
powerful as a result.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH
2016-10-25 13:03 [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Ashijeet Acharya
` (4 preceding siblings ...)
2016-10-25 13:04 ` [Qemu-devel] [PATCH v4 5/5] qapi: allow blockdev-add for ssh Ashijeet Acharya
@ 2016-10-27 19:54 ` Kevin Wolf
5 siblings, 0 replies; 10+ messages in thread
From: Kevin Wolf @ 2016-10-27 19:54 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: rjones, jcody, mreitz, eblake, berrange, pbonzini, qemu-devel,
qemu-block
Am 25.10.2016 um 15:03 hat Ashijeet Acharya geschrieben:
> Previously posted series patches:
> v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg03781.html
> v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg03403.html
> v1: http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg02137.html
>
> This series adds blockdev-add support for SSH block driver.
Thanks, dropped the "host_key_check" option and applied to the block
branch.
Kevin
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-10-27 19:54 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-25 13:03 [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 1/5] block/ssh: Add ssh_has_filename_options_conflict() Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 2/5] util/qemu-sockets: Make inet_connect_saddr() public Ashijeet Acharya
2016-10-25 13:03 ` [Qemu-devel] [PATCH v4 3/5] block/ssh: Add InetSocketAddress and accept it Ashijeet Acharya
2016-10-25 13:04 ` [Qemu-devel] [PATCH v4 4/5] block/ssh: Use InetSocketAddress options Ashijeet Acharya
2016-10-25 13:04 ` [Qemu-devel] [PATCH v4 5/5] qapi: allow blockdev-add for ssh Ashijeet Acharya
2016-10-25 19:58 ` Eric Blake
2016-10-26 8:31 ` Kevin Wolf
2016-10-26 13:27 ` Eric Blake
2016-10-27 19:54 ` [Qemu-devel] [PATCH v4 0/5] Allow blockdev-add for SSH Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).