From 9588c5897b83a43c80b810ec44e575e1032378d4 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Tue, 28 Mar 2017 19:12:46 +0300 Subject: [PATCH 01/15] block: add missed aio_context_acquire into release_drive Recently we expirience hang with iothreads enabled with the following call trace: Thread 1 (Thread 0x7fa95efebc80 (LWP 177117)): 0 ppoll () from /lib64/libc.so.6 2 qemu_poll_ns () at qemu-timer.c:313 3 aio_poll () at aio-posix.c:457 4 bdrv_flush () at block/io.c:2641 5 bdrv_close () at block.c:2143 6 bdrv_delete () at block.c:2352 7 bdrv_unref () at block.c:3429 8 blk_remove_bs () at block/block-backend.c:427 9 blk_delete () at block/block-backend.c:178 10 blk_unref () at block/block-backend.c:226 11 object_property_del_all () at qom/object.c:399 12 object_finalize () at qom/object.c:461 13 object_unref () at qom/object.c:898 14 object_property_del_child () at qom/object.c:422 15 qmp_marshal_device_del () at qmp-marshal.c:1145 16 handle_qmp_command () at /usr/src/debug/qemu-2.6.0/monitor.c:3929 Technically bdrv_flush() stucks in while (rwco.ret == NOT_DONE) { aio_poll(aio_context, true); } but rwco.ret is equal to 0 thus we have missed wakeup. Code investigation reveals that we do not have performed aio_context_acquire() on this call stack. This patch adds missed lock. Signed-off-by: Denis V. Lunev CC: Kevin Wolf CC: Max Reitz CC: Eric Blake CC: Markus Armbruster Message-id: 1490717566-25516-1-git-send-email-den@openvz.org Reviewed-by: Fam Zheng Signed-off-by: Max Reitz --- hw/core/qdev-properties-system.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index c34be1c1ba..e885e650fb 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -124,8 +124,12 @@ static void release_drive(Object *obj, const char *name, void *opaque) BlockBackend **ptr = qdev_get_prop_ptr(dev, prop); if (*ptr) { + AioContext *ctx = blk_get_aio_context(*ptr); + + aio_context_acquire(ctx); blockdev_auto_del(*ptr); blk_detach_dev(*ptr, dev); + aio_context_release(ctx); } } From ca0b64e5ed5c60543ff4758c858f65cd090512f0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:09 +0200 Subject: [PATCH 02/15] nbd sockets vnc: Mark problematic address family tests TODO Certain features make sense only with certain address families. For instance, passing file descriptors requires AF_UNIX. Testing SocketAddress's saddr->type == SOCKET_ADDRESS_KIND_UNIX is obvious, but problematic: it can't recognize AF_UNIX when type == SOCKET_ADDRESS_KIND_FD. Mark such tests of saddr->type TODO. We may want to check the address family with getsockname() there. Cc: Paolo Bonzini Cc: Gerd Hoffmann Cc: Daniel P. Berrange Signed-off-by: Markus Armbruster Reviewed-by: Max Reitz Reviewed-by: Eric Blake Message-id: 1490895797-29094-2-git-send-email-armbru@redhat.com Signed-off-by: Max Reitz --- block/nbd.c | 1 + blockdev-nbd.c | 1 + chardev/char-socket.c | 5 ++--- ui/vnc.c | 1 + util/qemu-sockets.c | 4 ++++ 5 files changed, 9 insertions(+), 3 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index 1b832c2132..36ea617989 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -421,6 +421,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, goto error; } + /* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 */ if (s->saddr->type != SOCKET_ADDRESS_KIND_INET) { error_setg(errp, "TLS only supported over IP sockets"); goto error; diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 7ea836b46e..8a11807df3 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -124,6 +124,7 @@ void qmp_nbd_server_start(SocketAddress *addr, goto error; } + /* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 */ if (addr->type != SOCKET_ADDRESS_KIND_INET) { error_setg(errp, "TLS is only supported with IPv4/IPv6"); goto error; diff --git a/chardev/char-socket.c b/chardev/char-socket.c index d7e92e1bd3..6344b07822 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -47,7 +47,6 @@ typedef struct { int max_size; int do_telnetopt; int do_nodelay; - int is_unix; int *read_msgfds; size_t read_msgfds_num; int *write_msgfds; @@ -825,7 +824,6 @@ static void qmp_chardev_open_socket(Chardev *chr, int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0; QIOChannelSocket *sioc = NULL; - s->is_unix = addr->type == SOCKET_ADDRESS_KIND_UNIX; s->is_listen = is_listen; s->is_telnet = is_telnet; s->do_nodelay = do_nodelay; @@ -865,7 +863,8 @@ static void qmp_chardev_open_socket(Chardev *chr, s->addr = QAPI_CLONE(SocketAddress, sock->addr); qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE); - if (s->is_unix) { + /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */ + if (addr->type == SOCKET_ADDRESS_KIND_UNIX) { qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS); } diff --git a/ui/vnc.c b/ui/vnc.c index 243e99bb95..1095f9de68 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3642,6 +3642,7 @@ static int vnc_display_connect(VncDisplay *vd, error_setg(errp, "Expected a single address in reverse mode"); return -1; } + /* TODO SOCKET_ADDRESS_KIND_FD when fd has AF_UNIX */ vd->is_unix = saddr[0]->type == SOCKET_ADDRESS_KIND_UNIX; sioc = qio_channel_socket_new(); qio_channel_set_name(QIO_CHANNEL(sioc), "vnc-reverse"); diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 40164bf681..9b7368113c 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1154,6 +1154,10 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp) { int fd; + /* + * TODO SOCKET_ADDRESS_KIND_FD when fd is AF_INET or AF_INET6 + * (although other address families can do SOCK_DGRAM, too) + */ switch (remote->type) { case SOCKET_ADDRESS_KIND_INET: fd = inet_dgram_saddr(remote->u.inet.data, From d2e49aad7259af943b72be761ee5c18e14acd71a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:10 +0200 Subject: [PATCH 03/15] char: Fix socket with "type": "vsock" address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Watch this: $ qemu-system-x86_64 -nodefaults -S -display none -qmp stdio {"QMP": {"version": {"qemu": {"micro": 91, "minor": 8, "major": 2}, "package": " (v2.8.0-1195-gf84141e-dirty)"}, "capabilities": []}} { "execute": "qmp_capabilities" } {"return": {}} { "execute": "chardev-add", "arguments": { "id": "chr0", "backend": { "type": "socket", "data": { "addr": { "type": "vsock", "data": { "cid": "CID", "port": "P" }}}}}} Aborted (core dumped) Crashes because SocketAddress_to_str() is blissfully unaware of SOCKET_ADDRESS_KIND_VSOCK. Fix that. Pick the output format to match socket_parse(), just like the existing formats. Cc: Stefan Hajnoczi Cc: Paolo Bonzini Cc: Marc-André Lureau Signed-off-by: Markus Armbruster Reviewed-by: Marc-André Lureau Reviewed-by: Max Reitz Message-id: 1490895797-29094-3-git-send-email-armbru@redhat.com Reviewed-by: Stefan Hajnoczi Signed-off-by: Max Reitz --- chardev/char-socket.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 6344b07822..36ab0d633a 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -357,6 +357,10 @@ static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr, return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.data->str, is_listen ? ",server" : ""); break; + case SOCKET_ADDRESS_KIND_VSOCK: + return g_strdup_printf("%svsock:%s:%s", prefix, + addr->u.vsock.data->cid, + addr->u.vsock.data->port); default: abort(); } From a6c76285f2e41535527a46edf4d158a2779545e1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:11 +0200 Subject: [PATCH 04/15] io vnc sockets: Clean up SocketAddressKind switches We have quite a few switches over SocketAddressKind. Some have case labels for all enumeration values, others rely on a default label. Some abort when the value isn't a valid SocketAddressKind, others report an error then. Unify as follows. Always provide case labels for all enumeration values, to clarify intent. Abort when the value isn't a valid SocketAddressKind, because the program state is messed up then. Improve a few error messages while there. Signed-off-by: Markus Armbruster Reviewed-by: Max Reitz Message-id: 1490895797-29094-4-git-send-email-armbru@redhat.com Signed-off-by: Max Reitz --- io/dns-resolver.c | 7 +++++-- ui/vnc.c | 18 ++++++++++++------ util/qemu-sockets.c | 4 +--- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/io/dns-resolver.c b/io/dns-resolver.c index 0ac6b23c02..a407075934 100644 --- a/io/dns-resolver.c +++ b/io/dns-resolver.c @@ -164,9 +164,12 @@ int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver, addrs, errp); - default: - error_setg(errp, "Unknown socket address kind"); + case SOCKET_ADDRESS_KIND_FD: + error_setg(errp, "Unsupported socket address type 'fd'"); return -1; + + default: + abort(); } } diff --git a/ui/vnc.c b/ui/vnc.c index 1095f9de68..349cfc9d86 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -129,10 +129,13 @@ static void vnc_init_basic_info(SocketAddress *addr, info->family = NETWORK_ADDRESS_FAMILY_UNIX; break; - default: - error_setg(errp, "Unsupported socket kind %d", - addr->type); + case SOCKET_ADDRESS_KIND_VSOCK: + case SOCKET_ADDRESS_KIND_FD: + error_setg(errp, "Unsupported socket address type %s", + SocketAddressKind_lookup[addr->type]); break; + default: + abort(); } return; @@ -411,10 +414,13 @@ VncInfo *qmp_query_vnc(Error **errp) info->family = NETWORK_ADDRESS_FAMILY_UNIX; break; - default: - error_setg(errp, "Unsupported socket kind %d", - addr->type); + case SOCKET_ADDRESS_KIND_VSOCK: + case SOCKET_ADDRESS_KIND_FD: + error_setg(errp, "Unsupported socket address type %s", + SocketAddressKind_lookup[addr->type]); goto out_error; + default: + abort(); } info->has_host = true; diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 9b7368113c..4ae37bd2a2 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1337,9 +1337,7 @@ char *socket_address_to_string(struct SocketAddress *addr, Error **errp) break; default: - error_setg(errp, "socket family %d unsupported", - addr->type); - return NULL; + abort(); } return buf; } From 129c7d1c536d0c67a8781cb09fb5bdb3d0f6a2d0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:12 +0200 Subject: [PATCH 05/15] block: Document -drive problematic code and bugs -blockdev and blockdev_add convert their arguments via QObject to BlockdevOptions for qmp_blockdev_add(), which converts them back to QObject, then to a flattened QDict. The QDict's members are typed according to the QAPI schema. -drive converts its argument via QemuOpts to a (flat) QDict. This QDict's members are all QString. Thus, the QType of a flat QDict member depends on whether it comes from -drive or -blockdev/blockdev_add, except when the QAPI type maps to QString, which is the case for 'str' and enumeration types. The block layer core extracts generic configuration from the flat QDict, and the block driver extracts driver-specific configuration. Both commonly do so by converting (parts of) the flat QDict to QemuOpts, which turns all values into strings. Not exactly elegant, but correct. However, A few places access the flat QDict directly: * Most of them access members that are always QString. Correct. * bdrv_open_inherit() accesses a boolean, carefully. Correct. * nfs_config() uses a QObject input visitor. Correct only because the visited type contains nothing but QStrings. * nbd_config() and ssh_config() use a QObject input visitor, and the visited types contain non-QStrings: InetSocketAddress members @numeric, @to, @ipv4, @ipv6. -drive works as long as you don't try to use them (they're all optional). @to is ignored anyway. Reproducer: -drive driver=ssh,server.host=h,server.port=22,server.ipv4,path=p -drive driver=nbd,server.type=inet,server.data.host=h,server.data.port=22,server.data.ipv4 both fail with "Invalid parameter type for 'data.ipv4', expected: boolean" Add suitable comments to all these places. Mark the buggy ones FIXME. "Fortunately", -drive's driver-specific options are entirely undocumented. Signed-off-by: Markus Armbruster Message-id: 1490895797-29094-5-git-send-email-armbru@redhat.com [mreitz: Fixed two typos] Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- block.c | 48 +++++++++++++++++++++++++++++++++++++++++++--- block/file-posix.c | 6 ++++++ block/nbd.c | 8 ++++++++ block/nfs.c | 7 +++++++ block/rbd.c | 6 ++++++ block/ssh.c | 8 ++++++++ 6 files changed, 80 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 6e906ec53c..927ba89eb7 100644 --- a/block.c +++ b/block.c @@ -1157,6 +1157,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file, if (file != NULL) { filename = blk_bs(file)->filename; } else { + /* + * Caution: while qdict_get_try_str() is fine, getting + * non-string types would require more care. When @options + * come from -blockdev or blockdev_add, its members are typed + * according to the QAPI schema, but when they come from + * -drive, they're all QString. + */ filename = qdict_get_try_str(options, "filename"); } @@ -1324,6 +1331,13 @@ static int bdrv_fill_options(QDict **options, const char *filename, BlockDriver *drv = NULL; Error *local_err = NULL; + /* + * Caution: while qdict_get_try_str() is fine, getting non-string + * types would require more care. When @options come from + * -blockdev or blockdev_add, its members are typed according to + * the QAPI schema, but when they come from -drive, they're all + * QString. + */ drvname = qdict_get_try_str(*options, "driver"); if (drvname) { drv = bdrv_find_format(drvname); @@ -1358,6 +1372,7 @@ static int bdrv_fill_options(QDict **options, const char *filename, } /* Find the right block driver */ + /* See cautionary note on accessing @options above */ filename = qdict_get_try_str(*options, "filename"); if (!drvname && protocol) { @@ -1987,6 +2002,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, qdict_extract_subqdict(parent_options, &options, bdref_key_dot); g_free(bdref_key_dot); + /* + * Caution: while qdict_get_try_str() is fine, getting non-string + * types would require more care. When @parent_options come from + * -blockdev or blockdev_add, its members are typed according to + * the QAPI schema, but when they come from -drive, they're all + * QString. + */ reference = qdict_get_try_str(parent_options, bdref_key); if (reference || qdict_haskey(options, "file.filename")) { backing_filename[0] = '\0'; @@ -2059,6 +2081,13 @@ bdrv_open_child_bs(const char *filename, QDict *options, const char *bdref_key, qdict_extract_subqdict(options, &image_options, bdref_key_dot); g_free(bdref_key_dot); + /* + * Caution: while qdict_get_try_str() is fine, getting non-string + * types would require more care. When @options come from + * -blockdev or blockdev_add, its members are typed according to + * the QAPI schema, but when they come from -drive, they're all + * QString. + */ reference = qdict_get_try_str(options, bdref_key); if (!filename && !reference && !qdict_size(image_options)) { if (!allow_none) { @@ -2274,9 +2303,13 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, goto fail; } - /* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags. - * FIXME: we're parsing the QDict to avoid having to create a - * QemuOpts just for this, but neither option is optimal. */ + /* + * Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags. + * Caution: getting a boolean member of @options requires care. + * When @options come from -blockdev or blockdev_add, members are + * typed according to the QAPI schema, but when they come from + * -drive, they're all QString. + */ if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_READ_ONLY), "on") && !qdict_get_try_bool(options, BDRV_OPT_READ_ONLY, false)) { flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR); @@ -2298,6 +2331,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, options = qdict_clone_shallow(options); /* Find the right image format driver */ + /* See cautionary note on accessing @options above */ drvname = qdict_get_try_str(options, "driver"); if (drvname) { drv = bdrv_find_format(drvname); @@ -2309,6 +2343,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, assert(drvname || !(flags & BDRV_O_PROTOCOL)); + /* See cautionary note on accessing @options above */ backing = qdict_get_try_str(options, "backing"); if (backing && *backing == '\0') { flags |= BDRV_O_NO_BACKING; @@ -2787,6 +2822,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, do { QString *new_obj = qobject_to_qstring(entry->value); const char *new = qstring_get_str(new_obj); + /* + * Caution: while qdict_get_try_str() is fine, getting + * non-string types would require more care. When + * bs->options come from -blockdev or blockdev_add, its + * members are typed according to the QAPI schema, but + * when they come from -drive, they're all QString. + */ const char *old = qdict_get_try_str(reopen_state->bs->options, entry->key); diff --git a/block/file-posix.c b/block/file-posix.c index 0841a08785..0c4896876e 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2193,6 +2193,12 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags, int ret; #if defined(__APPLE__) && defined(__MACH__) + /* + * Caution: while qdict_get_str() is fine, getting non-string types + * would require more care. When @options come from -blockdev or + * blockdev_add, its members are typed according to the QAPI + * schema, but when they come from -drive, they're all QString. + */ const char *filename = qdict_get_str(options, "filename"); char bsd_path[MAXPATHLEN] = ""; bool error_occurred = false; diff --git a/block/nbd.c b/block/nbd.c index 36ea617989..11e3ba75fa 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -278,6 +278,14 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp) goto done; } + /* + * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive + * server.type=inet. .to doesn't matter, it's ignored anyway. + * That's because when @options come from -blockdev or + * blockdev_add, members are typed according to the QAPI schema, + * but when they come from -drive, they're all QString. The + * visitor expects the former. + */ iv = qobject_input_visitor_new(crumpled_addr); visit_type_SocketAddress(iv, NULL, &saddr, &local_err); if (local_err) { diff --git a/block/nfs.c b/block/nfs.c index 3f43f6e26a..0816678307 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -474,6 +474,13 @@ static NFSServer *nfs_config(QDict *options, Error **errp) goto out; } + /* + * Caution: this works only because all scalar members of + * NFSServer are QString in @crumpled_addr. The visitor expects + * @crumpled_addr to be typed according to the QAPI schema. It + * is when @options come from -blockdev or blockdev_add. But when + * they come from -drive, they're all QString. + */ iv = qobject_input_visitor_new(crumpled_addr); visit_type_NFSServer(iv, NULL, &server, &local_error); if (local_error) { diff --git a/block/rbd.c b/block/rbd.c index fbdb131a68..1ceeeb5a60 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -385,6 +385,12 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp) goto exit; } + /* + * Caution: while qdict_get_try_str() is fine, getting non-string + * types would require more care. When @options come from -blockdev + * or blockdev_add, its members are typed according to the QAPI + * schema, but when they come from -drive, they're all QString. + */ pool = qdict_get_try_str(options, "pool"); conf = qdict_get_try_str(options, "conf"); clientname = qdict_get_try_str(options, "user"); diff --git a/block/ssh.c b/block/ssh.c index 278e66faa6..471ba8a260 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -601,6 +601,14 @@ static InetSocketAddress *ssh_config(QDict *options, Error **errp) goto out; } + /* + * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive. + * .to doesn't matter, it's ignored anyway. + * That's because when @options come from -blockdev or + * blockdev_add, members are typed according to the QAPI schema, + * but when they come from -drive, they're all QString. The + * visitor expects the former. + */ iv = qobject_input_visitor_new(crumpled_addr); visit_type_InetSocketAddress(iv, NULL, &inet, &local_error); if (local_error) { From fce5d5386d8613e9659f1edc4be9c3f8b7cff073 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:13 +0200 Subject: [PATCH 06/15] gluster: Prepare for SocketAddressFlat extension qemu_gluster_glfs_init() and qemu_gluster_parse_json() rely on the fact that SocketAddressFlatType has only two members SOCKET_ADDRESS_FLAT_TYPE_INET and SOCKET_ADDRESS_FLAT_TYPE_UNIX. Correct, but won't stay correct. Make them more robust. Signed-off-by: Markus Armbruster Reviewed-by: Max Reitz Reviewed-by: Jeff Cody Message-id: 1490895797-29094-6-git-send-email-armbru@redhat.com Signed-off-by: Max Reitz --- block/gluster.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index a577daef10..fb0aafeaa3 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -412,10 +412,12 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, glfs_set_preopened(gconf->volume, glfs); for (server = gconf->server; server; server = server->next) { - if (server->value->type == SOCKET_ADDRESS_FLAT_TYPE_UNIX) { + switch (server->value->type) { + case SOCKET_ADDRESS_FLAT_TYPE_UNIX: ret = glfs_set_volfile_server(glfs, "unix", server->value->u.q_unix.path, 0); - } else { + break; + case SOCKET_ADDRESS_FLAT_TYPE_INET: if (parse_uint_full(server->value->u.inet.port, &port, 10) < 0 || port > 65535) { error_setg(errp, "'%s' is not a valid port number", @@ -426,6 +428,9 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, ret = glfs_set_volfile_server(glfs, "tcp", server->value->u.inet.host, (int)port); + break; + default: + abort(); } if (ret < 0) { @@ -487,7 +492,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, char *str = NULL; const char *ptr; size_t num_servers; - int i; + int i, type; /* create opts info from runtime_json_opts list */ opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); @@ -539,16 +544,17 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, if (!strcmp(ptr, "tcp")) { ptr = "inet"; /* accept legacy "tcp" */ } - gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr, - SOCKET_ADDRESS_FLAT_TYPE__MAX, -1, - &local_err); - if (local_err) { - error_append_hint(&local_err, - "Parameter '%s' may be 'inet' or 'unix'\n", - GLUSTER_OPT_TYPE); + type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr, + SOCKET_ADDRESS_FLAT_TYPE__MAX, -1, NULL); + if (type != SOCKET_ADDRESS_FLAT_TYPE_INET + && type != SOCKET_ADDRESS_FLAT_TYPE_UNIX) { + error_setg(&local_err, + "Parameter '%s' may be 'inet' or 'unix'", + GLUSTER_OPT_TYPE); error_append_hint(&local_err, GERR_INDEX_HINT, i); goto out; } + gsconf->type = type; qemu_opts_del(opts); if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_INET) { From 8bc0673f6dc2485b46da6de8204aaca2a711acea Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:14 +0200 Subject: [PATCH 07/15] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd' Note that the new variants are impossible in qemu_gluster_glfs_init(), because the gconf->server can only come from qemu_gluster_parse_uri() or qemu_gluster_parse_json(), and neither can create anything but 'inet' or 'unix'. Signed-off-by: Markus Armbruster Reviewed-by: Max Reitz Reviewed-by: Eric Blake Message-id: 1490895797-29094-7-git-send-email-armbru@redhat.com Signed-off-by: Max Reitz --- block/gluster.c | 2 ++ qapi-schema.json | 19 ++++++++----------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index fb0aafeaa3..cf29b5f9a4 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -429,6 +429,8 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, server->value->u.inet.host, (int)port); break; + case SOCKET_ADDRESS_FLAT_TYPE_VSOCK: + case SOCKET_ADDRESS_FLAT_TYPE_FD: default: abort(); } diff --git a/qapi-schema.json b/qapi-schema.json index b921994ae3..250e4dc49b 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4144,7 +4144,7 @@ # Since: 2.9 ## { 'enum': 'SocketAddressFlatType', - 'data': [ 'unix', 'inet' ] } + 'data': [ 'inet', 'unix', 'vsock', 'fd' ] } ## # @SocketAddressFlat: @@ -4153,22 +4153,19 @@ # # @type: Transport type # -# This is similar to SocketAddress, only distinction: -# -# 1. SocketAddressFlat is a flat union, SocketAddress is a simple union. -# A flat union is nicer than simple because it avoids nesting -# (i.e. more {}) on the wire. -# -# 2. SocketAddressFlat supports only types 'unix' and 'inet', because -# that's what its current users need. +# This is just like SocketAddress, except it's a flat union rather +# than a simple union. Nicer because it avoids nesting on the wire, +# i.e. this form has fewer {}. # # Since: 2.9 ## { 'union': 'SocketAddressFlat', 'base': { 'type': 'SocketAddressFlatType' }, 'discriminator': 'type', - 'data': { 'unix': 'UnixSocketAddress', - 'inet': 'InetSocketAddress' } } + 'data': { 'inet': 'InetSocketAddress', + 'unix': 'UnixSocketAddress', + 'vsock': 'VsockSocketAddress', + 'fd': 'String' } } ## # @getfd: From 216411b83967ae19e3c9c520e13ceb8bb383890c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:15 +0200 Subject: [PATCH 08/15] sockets: New helper socket_address_crumple() SocketAddress is a simple union, and simple unions are awkward: they have their variant members wrapped in a "data" object on the wire, and require additional indirections in C. I intend to limit its use to existing external interfaces. New ones should use SocketAddressFlat. I further intend to convert all internal interfaces to SocketAddressFlat. This helper should go away then. Signed-off-by: Markus Armbruster Message-id: 1490895797-29094-8-git-send-email-armbru@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- include/qemu/sockets.h | 11 +++++++++++ util/qemu-sockets.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h index 5f1bab9b3e..7842f6d150 100644 --- a/include/qemu/sockets.h +++ b/include/qemu/sockets.h @@ -119,4 +119,15 @@ SocketAddress *socket_remote_address(int fd, Error **errp); */ char *socket_address_to_string(struct SocketAddress *addr, Error **errp); +/** + * socket_address_crumple: + * @addr_flat: the socket address to crumple + * + * Convert SocketAddressFlat to SocketAddress. Caller is responsible + * for freeing with qapi_free_SocketAddress(). + * + * Returns: the argument converted to SocketAddress. + */ +SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat); + #endif /* QEMU_SOCKETS_H */ diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 4ae37bd2a2..21442c30dc 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -25,6 +25,7 @@ #include "qapi/error.h" #include "qemu/sockets.h" #include "qemu/main-loop.h" +#include "qapi/clone-visitor.h" #include "qapi/qobject-input-visitor.h" #include "qapi/qobject-output-visitor.h" #include "qapi-visit.h" @@ -1341,3 +1342,34 @@ char *socket_address_to_string(struct SocketAddress *addr, Error **errp) } return buf; } + +SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat) +{ + SocketAddress *addr = g_new(SocketAddress, 1); + + switch (addr_flat->type) { + case SOCKET_ADDRESS_FLAT_TYPE_INET: + addr->type = SOCKET_ADDRESS_KIND_INET; + addr->u.inet.data = QAPI_CLONE(InetSocketAddress, + &addr_flat->u.inet); + break; + case SOCKET_ADDRESS_FLAT_TYPE_UNIX: + addr->type = SOCKET_ADDRESS_KIND_UNIX; + addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, + &addr_flat->u.q_unix); + break; + case SOCKET_ADDRESS_FLAT_TYPE_VSOCK: + addr->type = SOCKET_ADDRESS_KIND_VSOCK; + addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress, + &addr_flat->u.vsock); + break; + case SOCKET_ADDRESS_FLAT_TYPE_FD: + addr->type = SOCKET_ADDRESS_KIND_FD; + addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd); + break; + default: + abort(); + } + + return addr; +} From 9445673ea67c272616b9f718396e267caa6446b7 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:16 +0200 Subject: [PATCH 09/15] nbd: Tidy up blockdev-add interface SocketAddress is a simple union, and simple unions are awkward: they have their variant members wrapped in a "data" object on the wire, and require additional indirections in C. I intend to limit its use to existing external interfaces, and convert all internal interfaces to SocketAddressFlat. BlockdevOptionsNbd is an external interface using SocketAddress. We already use SocketAddressFlat elsewhere in blockdev-add. Replace it by SocketAddressFlat while we can (it's new in 2.9) for simplicity and consistency. For example, { "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": "nbd", "server": { "type": "inet", "data": { "host": "localhost", "port": "12345" } } } } becomes { "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": "nbd", "server": { "type": "inet", "host": "localhost", "port": "12345" } } } Since the internal interfaces still take SocketAddress, this requires conversion function socket_address_crumple(). It'll go away when I update the interfaces. Unfortunately, SocketAddress is also visible in -drive since 2.8: -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345 Nobody should be using it, as it's fairly new and has never been documented, so adding still more compatibility gunk to keep it working isn't worth the trouble. You now have to use -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345 Signed-off-by: Markus Armbruster Message-id: 1490895797-29094-9-git-send-email-armbru@redhat.com [mreitz: Change iotest 147 accordingly] Because of this interface change, iotest 147 has to be adapted. Unfortunately, we cannot just flatten all of the addresses because nbd-server-start still takes a plain SocketAddress. Therefore, we need both and this is most easily achieved by writing the SocketAddress into the code and flattening it where necessary. Signed-off-by: Max Reitz Message-id: 20170330221243.17333-1-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- block/nbd.c | 53 ++++++++++++++++++++++-------------------- qapi/block-core.json | 2 +- tests/qemu-iotests/147 | 25 +++++++++++++------- 3 files changed, 46 insertions(+), 34 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index 11e3ba75fa..8bb29a90bb 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -47,7 +47,7 @@ typedef struct BDRVNBDState { NBDClientSession client; /* For nbd_refresh_filename() */ - SocketAddress *saddr; + SocketAddressFlat *saddr; char *export, *tlscredsid; } BDRVNBDState; @@ -95,7 +95,7 @@ static int nbd_parse_uri(const char *filename, QDict *options) goto out; } qdict_put(options, "server.type", qstring_from_str("unix")); - qdict_put(options, "server.data.path", + qdict_put(options, "server.path", qstring_from_str(qp->p[0].value)); } else { QString *host; @@ -116,10 +116,10 @@ static int nbd_parse_uri(const char *filename, QDict *options) } qdict_put(options, "server.type", qstring_from_str("inet")); - qdict_put(options, "server.data.host", host); + qdict_put(options, "server.host", host); port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT); - qdict_put(options, "server.data.port", qstring_from_str(port_str)); + qdict_put(options, "server.port", qstring_from_str(port_str)); g_free(port_str); } @@ -197,7 +197,7 @@ static void nbd_parse_filename(const char *filename, QDict *options, /* are we a UNIX or TCP socket? */ if (strstart(host_spec, "unix:", &unixpath)) { qdict_put(options, "server.type", qstring_from_str("unix")); - qdict_put(options, "server.data.path", qstring_from_str(unixpath)); + qdict_put(options, "server.path", qstring_from_str(unixpath)); } else { InetSocketAddress *addr = NULL; @@ -207,8 +207,8 @@ static void nbd_parse_filename(const char *filename, QDict *options, } qdict_put(options, "server.type", qstring_from_str("inet")); - qdict_put(options, "server.data.host", qstring_from_str(addr->host)); - qdict_put(options, "server.data.port", qstring_from_str(addr->port)); + qdict_put(options, "server.host", qstring_from_str(addr->host)); + qdict_put(options, "server.port", qstring_from_str(addr->port)); qapi_free_InetSocketAddress(addr); } @@ -248,20 +248,21 @@ static bool nbd_process_legacy_socket_options(QDict *output_options, } qdict_put(output_options, "server.type", qstring_from_str("unix")); - qdict_put(output_options, "server.data.path", qstring_from_str(path)); + qdict_put(output_options, "server.path", qstring_from_str(path)); } else if (host) { qdict_put(output_options, "server.type", qstring_from_str("inet")); - qdict_put(output_options, "server.data.host", qstring_from_str(host)); - qdict_put(output_options, "server.data.port", + qdict_put(output_options, "server.host", qstring_from_str(host)); + qdict_put(output_options, "server.port", qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT))); } return true; } -static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp) +static SocketAddressFlat *nbd_config(BDRVNBDState *s, QDict *options, + Error **errp) { - SocketAddress *saddr = NULL; + SocketAddressFlat *saddr = NULL; QDict *addr = NULL; QObject *crumpled_addr = NULL; Visitor *iv = NULL; @@ -287,7 +288,7 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp) * visitor expects the former. */ iv = qobject_input_visitor_new(crumpled_addr); - visit_type_SocketAddress(iv, NULL, &saddr, &local_err); + visit_type_SocketAddressFlat(iv, NULL, &saddr, &local_err); if (local_err) { error_propagate(errp, local_err); goto done; @@ -306,9 +307,10 @@ NBDClientSession *nbd_get_client_session(BlockDriverState *bs) return &s->client; } -static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr, +static QIOChannelSocket *nbd_establish_connection(SocketAddressFlat *saddr_flat, Error **errp) { + SocketAddress *saddr = socket_address_crumple(saddr_flat); QIOChannelSocket *sioc; Error *local_err = NULL; @@ -318,6 +320,7 @@ static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr, qio_channel_socket_connect_sync(sioc, saddr, &local_err); + qapi_free_SocketAddress(saddr); if (local_err) { error_propagate(errp, local_err); return NULL; @@ -409,7 +412,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, goto error; } - /* Translate @host, @port, and @path to a SocketAddress */ + /* Translate @host, @port, and @path to a SocketAddressFlat */ if (!nbd_process_legacy_socket_options(options, opts, errp)) { goto error; } @@ -430,11 +433,11 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, } /* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 */ - if (s->saddr->type != SOCKET_ADDRESS_KIND_INET) { + if (s->saddr->type != SOCKET_ADDRESS_FLAT_TYPE_INET) { error_setg(errp, "TLS only supported over IP sockets"); goto error; } - hostname = s->saddr->u.inet.data->host; + hostname = s->saddr->u.inet.host; } /* establish TCP connection, return error if it fails @@ -457,7 +460,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, object_unref(OBJECT(tlscreds)); } if (ret < 0) { - qapi_free_SocketAddress(s->saddr); + qapi_free_SocketAddressFlat(s->saddr); g_free(s->export); g_free(s->tlscredsid); } @@ -483,7 +486,7 @@ static void nbd_close(BlockDriverState *bs) nbd_client_close(bs); - qapi_free_SocketAddress(s->saddr); + qapi_free_SocketAddressFlat(s->saddr); g_free(s->export); g_free(s->tlscredsid); } @@ -514,15 +517,15 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) Visitor *ov; const char *host = NULL, *port = NULL, *path = NULL; - if (s->saddr->type == SOCKET_ADDRESS_KIND_INET) { - const InetSocketAddress *inet = s->saddr->u.inet.data; + if (s->saddr->type == SOCKET_ADDRESS_FLAT_TYPE_INET) { + const InetSocketAddress *inet = &s->saddr->u.inet; if (!inet->has_ipv4 && !inet->has_ipv6 && !inet->has_to) { host = inet->host; port = inet->port; } - } else if (s->saddr->type == SOCKET_ADDRESS_KIND_UNIX) { - path = s->saddr->u.q_unix.data->path; - } + } else if (s->saddr->type == SOCKET_ADDRESS_FLAT_TYPE_UNIX) { + path = s->saddr->u.q_unix.path; + } /* else can't represent as pseudo-filename */ qdict_put(opts, "driver", qstring_from_str("nbd")); @@ -541,7 +544,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) } ov = qobject_output_visitor_new(&saddr_qdict); - visit_type_SocketAddress(ov, NULL, &s->saddr, &error_abort); + visit_type_SocketAddressFlat(ov, NULL, &s->saddr, &error_abort); visit_complete(ov, &saddr_qdict); visit_free(ov); qdict_put_obj(opts, "server", saddr_qdict); diff --git a/qapi/block-core.json b/qapi/block-core.json index 8de39d143a..e4d8a63575 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2847,7 +2847,7 @@ # Since: 2.9 ## { 'struct': 'BlockdevOptionsNbd', - 'data': { 'server': 'SocketAddress', + 'data': { 'server': 'SocketAddressFlat', '*export': 'str', '*tls-creds': 'str' } } diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147 index cca75c562c..32afea63d4 100755 --- a/tests/qemu-iotests/147 +++ b/tests/qemu-iotests/147 @@ -30,6 +30,13 @@ NBD_PORT = 10811 test_img = os.path.join(iotests.test_dir, 'test.img') unix_socket = os.path.join(iotests.test_dir, 'nbd.socket') + +def flatten_sock_addr(crumpled_address): + result = { 'type': crumpled_address['type'] } + result.update(crumpled_address['data']) + return result + + class NBDBlockdevAddBase(iotests.QMPTestCase): def blockdev_add_options(self, address, export=None): options = { 'node-name': 'nbd-blockdev', @@ -85,13 +92,15 @@ class QemuNBD(NBDBlockdevAddBase): 'host': 'localhost', 'port': str(NBD_PORT) } } - self.client_test('nbd://localhost:%i' % NBD_PORT, address) + self.client_test('nbd://localhost:%i' % NBD_PORT, + flatten_sock_addr(address)) def test_unix(self): self._server_up('-k', unix_socket) address = { 'type': 'unix', 'data': { 'path': unix_socket } } - self.client_test('nbd+unix://?socket=' + unix_socket, address) + self.client_test('nbd+unix://?socket=' + unix_socket, + flatten_sock_addr(address)) class BuiltinNBD(NBDBlockdevAddBase): @@ -134,7 +143,7 @@ class BuiltinNBD(NBDBlockdevAddBase): } } self._server_up(address) self.client_test('nbd://localhost:%i/nbd-export' % NBD_PORT, - address, 'nbd-export') + flatten_sock_addr(address), 'nbd-export') self._server_down() def test_inet6(self): @@ -149,10 +158,10 @@ class BuiltinNBD(NBDBlockdevAddBase): 'file': { 'driver': 'nbd', 'export': 'nbd-export', - 'server': address + 'server': flatten_sock_addr(address) } } self._server_up(address) - self.client_test(filename, address, 'nbd-export') + self.client_test(filename, flatten_sock_addr(address), 'nbd-export') self._server_down() def test_unix(self): @@ -160,7 +169,7 @@ class BuiltinNBD(NBDBlockdevAddBase): 'data': { 'path': unix_socket } } self._server_up(address) self.client_test('nbd+unix:///nbd-export?socket=' + unix_socket, - address, 'nbd-export') + flatten_sock_addr(address), 'nbd-export') self._server_down() def test_fd(self): @@ -182,9 +191,9 @@ class BuiltinNBD(NBDBlockdevAddBase): 'file': { 'driver': 'nbd', 'export': 'nbd-export', - 'server': address + 'server': flatten_sock_addr(address) } } - self.client_test(filename, address, 'nbd-export') + self.client_test(filename, flatten_sock_addr(address), 'nbd-export') self._server_down() From d1c136885ba5b302f9781b8927a8ea2ee38cccd2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 30 Mar 2017 19:43:17 +0200 Subject: [PATCH 10/15] sheepdog: Fix blockdev-add Commit 831acdc "sheepdog: Implement bdrv_parse_filename()" and commit d282f34 "sheepdog: Support blockdev-add" have different ideas on how the QemuOpts parameters for the server address are named. Fix that. While there, rename BlockdevOptionsSheepdog member addr to server, for consistency with BlockdevOptionsSsh, BlockdevOptionsGluster, BlockdevOptionsNbd. Commit 831acdc's example becomes --drive driver=sheepdog,server.type=inet,server.host=fido,server.port=7000,vdi=dolly instead of --drive driver=sheepdog,host=fido,vdi=dolly Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Tested-by: Kashyap Chamarthy Message-id: 1490895797-29094-10-git-send-email-armbru@redhat.com Signed-off-by: Max Reitz --- block/sheepdog.c | 90 ++++++++++++++++++++++++++++---------------- qapi/block-core.json | 4 +- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 89e98edab6..1b71fc81ec 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -13,9 +13,11 @@ */ #include "qemu/osdep.h" +#include "qapi-visit.h" #include "qapi/error.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qint.h" +#include "qapi/qobject-input-visitor.h" #include "qemu/uri.h" #include "qemu/error-report.h" #include "qemu/sockets.h" @@ -547,6 +549,47 @@ static SocketAddress *sd_socket_address(const char *path, return addr; } +static SocketAddress *sd_server_config(QDict *options, Error **errp) +{ + QDict *server = NULL; + QObject *crumpled_server = NULL; + Visitor *iv = NULL; + SocketAddressFlat *saddr_flat = NULL; + SocketAddress *saddr = NULL; + Error *local_err = NULL; + + qdict_extract_subqdict(options, &server, "server."); + + crumpled_server = qdict_crumple(server, errp); + if (!crumpled_server) { + goto done; + } + + /* + * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive + * server.type=inet. .to doesn't matter, it's ignored anyway. + * That's because when @options come from -blockdev or + * blockdev_add, members are typed according to the QAPI schema, + * but when they come from -drive, they're all QString. The + * visitor expects the former. + */ + iv = qobject_input_visitor_new(crumpled_server); + visit_type_SocketAddressFlat(iv, NULL, &saddr_flat, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto done; + } + + saddr = socket_address_crumple(saddr_flat); + +done: + qapi_free_SocketAddressFlat(saddr_flat); + visit_free(iv); + qobject_decref(crumpled_server); + QDECREF(server); + return saddr; +} + /* Return -EIO in case of error, file descriptor on success */ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp) { @@ -1174,15 +1217,15 @@ static void sd_parse_filename(const char *filename, QDict *options, return; } - if (cfg.host) { - qdict_set_default_str(options, "host", cfg.host); - } - if (cfg.port) { - snprintf(buf, sizeof(buf), "%d", cfg.port); - qdict_set_default_str(options, "port", buf); - } if (cfg.path) { - qdict_set_default_str(options, "path", cfg.path); + qdict_set_default_str(options, "server.path", cfg.path); + qdict_set_default_str(options, "server.type", "unix"); + } else { + qdict_set_default_str(options, "server.type", "inet"); + qdict_set_default_str(options, "server.host", + cfg.host ?: SD_DEFAULT_ADDR); + snprintf(buf, sizeof(buf), "%d", cfg.port ?: SD_DEFAULT_PORT); + qdict_set_default_str(options, "server.port", buf); } qdict_set_default_str(options, "vdi", cfg.vdi); qdict_set_default_str(options, "tag", cfg.tag); @@ -1509,18 +1552,6 @@ static QemuOptsList runtime_opts = { .name = "sheepdog", .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), .desc = { - { - .name = "host", - .type = QEMU_OPT_STRING, - }, - { - .name = "port", - .type = QEMU_OPT_STRING, - }, - { - .name = "path", - .type = QEMU_OPT_STRING, - }, { .name = "vdi", .type = QEMU_OPT_STRING, @@ -1543,7 +1574,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, int ret, fd; uint32_t vid = 0; BDRVSheepdogState *s = bs->opaque; - const char *host, *port, *path, *vdi, *snap_id_str, *tag; + const char *vdi, *snap_id_str, *tag; uint64_t snap_id; char *buf = NULL; QemuOpts *opts; @@ -1560,20 +1591,17 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, goto err_no_fd; } - host = qemu_opt_get(opts, "host"); - port = qemu_opt_get(opts, "port"); - path = qemu_opt_get(opts, "path"); + s->addr = sd_server_config(options, errp); + if (!s->addr) { + ret = -EINVAL; + goto err_no_fd; + } + vdi = qemu_opt_get(opts, "vdi"); snap_id_str = qemu_opt_get(opts, "snap-id"); snap_id = qemu_opt_get_number(opts, "snap-id", CURRENT_VDI_ID); tag = qemu_opt_get(opts, "tag"); - if ((host || port) && path) { - error_setg(errp, "can't use 'path' together with 'host' or 'port'"); - ret = -EINVAL; - goto err_no_fd; - } - if (!vdi) { error_setg(errp, "parameter 'vdi' is missing"); ret = -EINVAL; @@ -1604,8 +1632,6 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, goto err_no_fd; } - s->addr = sd_socket_address(path, host, port); - QLIST_INIT(&s->inflight_aio_head); QLIST_INIT(&s->failed_aio_head); QLIST_INIT(&s->inflight_aiocb_head); diff --git a/qapi/block-core.json b/qapi/block-core.json index e4d8a63575..033457ce86 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2623,7 +2623,7 @@ # Driver specific block device options for sheepdog # # @vdi: Virtual disk image name -# @addr: The Sheepdog server to connect to +# @server: The Sheepdog server to connect to # @snap-id: Snapshot ID # @tag: Snapshot tag name # @@ -2632,7 +2632,7 @@ # Since: 2.9 ## { 'struct': 'BlockdevOptionsSheepdog', - 'data': { 'addr': 'SocketAddressFlat', + 'data': { 'server': 'SocketAddressFlat', 'vdi': 'str', '*snap-id': 'uint32', '*tag': 'str' } } From 6aabeb58391a33cda7e2b406adb431a5ef7e8eea Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 31 Mar 2017 14:38:49 +0100 Subject: [PATCH 11/15] qemu-io-cmds: Assert that global and nofile commands don't use ct->perms It would be a bug for a command with the CMD_NOFILE_OK or CMD_FLAG_GLOBAL flags set to also set the ct->perms field, because the former says "OK for a file not to be open" but the latter is a check on a file. Add an assertion in qemuio_add_command() so we can catch that sort of buggy command definition immediately rather than it being a bug that only manifests when a particular set of command line options is used. (Coverity gets confused about this (CID 1371723) and reports that we might dereference a NULL blk pointer in this case, because it can't tell that that code path never happens with the cmdinfo_t that we have. This commit won't help unconfuse it, but it does fix the underlying issue.) Signed-off-by: Peter Maydell Message-id: 1490967529-4767-1-git-send-email-peter.maydell@linaro.org Signed-off-by: Max Reitz --- qemu-io-cmds.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 2c48f9ce1a..883f53b64d 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -35,6 +35,13 @@ static int compare_cmdname(const void *a, const void *b) void qemuio_add_command(const cmdinfo_t *ci) { + /* ci->perm assumes a file is open, but the GLOBAL and NOFILE_OK + * flags allow it not to be, so that combination is invalid. + * Catch it now rather than letting it manifest as a crash if a + * particular set of command line options are used. + */ + assert(ci->perm == 0 || + (ci->flags & (CMD_FLAG_GLOBAL | CMD_NOFILE_OK)) == 0); cmdtab = g_renew(cmdinfo_t, cmdtab, ++ncmds); cmdtab[ncmds - 1] = *ci; qsort(cmdtab, ncmds, sizeof(*cmdtab), compare_cmdname); From 07ff948bd135373c516842ae5ce856f41f7c553a Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Fri, 31 Mar 2017 13:53:54 -0500 Subject: [PATCH 12/15] iotests: fix 097 when run with qcow The previous commit: commit a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd Author: Eric Blake Date: Mon Dec 5 09:49:34 2016 -0600 qcow2: Don't strand clusters near 2G intervals during commit extended the 097 test case so that it did two passes, once with an internal snapshot, once without. qcow (v1) does not support internal snapshots, so this change broke test 097 when run against qcow. This splits 097 in two, creating a new 176 that tests the internal snapshot codepath, effectively putting 097 back to its content before the above commit. Reviewed-by: Max Reitz Signed-off-by: Daniel P. Berrange Message-Id: <20170221115512.21918-8-berrange@redhat.com> [eblake: test collisions: s/173/176/g] Signed-off-by: Eric Blake Message-id: 20170331185356.2479-2-eblake@redhat.com Signed-off-by: Max Reitz --- tests/qemu-iotests/097 | 10 +-- tests/qemu-iotests/097.out | 125 ++---------------------------------- tests/qemu-iotests/176 | 126 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/176.out | 119 +++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 5 files changed, 251 insertions(+), 130 deletions(-) create mode 100755 tests/qemu-iotests/176 create mode 100644 tests/qemu-iotests/176.out diff --git a/tests/qemu-iotests/097 b/tests/qemu-iotests/097 index 4c33e8038a..1d28aff99d 100755 --- a/tests/qemu-iotests/097 +++ b/tests/qemu-iotests/097 @@ -56,26 +56,19 @@ _supported_os Linux # 3: Two-layer backing chain, commit to lower backing file # (in this case, the top image will implicitly stay unchanged) # -# Each pass is run twice, since qcow2 has different code paths for cleaning -# an image depending on whether it has a snapshot. -# # 020 already tests committing, so this only tests whether image chains are # working properly and that all images above the base are emptied; therefore, # no complicated patterns are necessary. Check near the 2G mark, as qcow2 # has been buggy at that boundary in the past. for i in 0 1 2 3; do -for j in 0 1; do echo -echo "=== Test pass $i.$j ===" +echo "=== Test pass $i ===" echo TEST_IMG="$TEST_IMG.base" _make_test_img 2100M TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" 2100M _make_test_img -b "$TEST_IMG.itmd" 2100M -if [ $j -eq 0 ]; then - $QEMU_IMG snapshot -c snap "$TEST_IMG" -fi $QEMU_IO -c 'write -P 1 0x7ffd0000 192k' "$TEST_IMG.base" | _filter_qemu_io $QEMU_IO -c 'write -P 2 0x7ffe0000 128k' "$TEST_IMG.itmd" | _filter_qemu_io @@ -121,7 +114,6 @@ $QEMU_IMG map "$TEST_IMG.itmd" | _filter_qemu_img_map $QEMU_IMG map "$TEST_IMG" | _filter_qemu_img_map done -done # success, all done diff --git a/tests/qemu-iotests/097.out b/tests/qemu-iotests/097.out index 8106cc9275..81fc2252a0 100644 --- a/tests/qemu-iotests/097.out +++ b/tests/qemu-iotests/097.out @@ -1,6 +1,6 @@ QA output created by 097 -=== Test pass 0.0 === +=== Test pass 0 === Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base @@ -29,36 +29,7 @@ Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd -=== Test pass 0.1 === - -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd -wrote 196608/196608 bytes at offset 2147287040 -192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -wrote 131072/131072 bytes at offset 2147352576 -128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -wrote 65536/65536 bytes at offset 2147418112 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Image committed. -read 196608/196608 bytes at offset 2147287040 -192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147287040 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147352576 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147418112 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Offset Length File -0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base -Offset Length File -0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base -0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd -Offset Length File -0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base -0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd - -=== Test pass 1.0 === +=== Test pass 1 === Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base @@ -88,7 +59,7 @@ Offset Length File 0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT -=== Test pass 1.1 === +=== Test pass 2 === Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base @@ -118,95 +89,7 @@ Offset Length File 0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT -=== Test pass 2.0 === - -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd -wrote 196608/196608 bytes at offset 2147287040 -192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -wrote 131072/131072 bytes at offset 2147352576 -128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -wrote 65536/65536 bytes at offset 2147418112 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Image committed. -read 196608/196608 bytes at offset 2147287040 -192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147287040 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147352576 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147418112 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Offset Length File -0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base -Offset Length File -0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base -0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd -Offset Length File -0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base -0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd -0x7fff0000 0x10000 TEST_DIR/t.IMGFMT - -=== Test pass 2.1 === - -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd -wrote 196608/196608 bytes at offset 2147287040 -192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -wrote 131072/131072 bytes at offset 2147352576 -128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -wrote 65536/65536 bytes at offset 2147418112 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Image committed. -read 196608/196608 bytes at offset 2147287040 -192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147287040 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147352576 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147418112 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Offset Length File -0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base -Offset Length File -0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base -0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd -Offset Length File -0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base -0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd -0x7fff0000 0x10000 TEST_DIR/t.IMGFMT - -=== Test pass 3.0 === - -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd -wrote 196608/196608 bytes at offset 2147287040 -192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -wrote 131072/131072 bytes at offset 2147352576 -128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -wrote 65536/65536 bytes at offset 2147418112 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Image committed. -read 65536/65536 bytes at offset 2147287040 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147352576 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 65536/65536 bytes at offset 2147418112 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Offset Length File -0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base -Offset Length File -0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base -0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd -Offset Length File -0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base -0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd -0x7fff0000 0x10000 TEST_DIR/t.IMGFMT - -=== Test pass 3.1 === +=== Test pass 3 === Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base diff --git a/tests/qemu-iotests/176 b/tests/qemu-iotests/176 new file mode 100755 index 0000000000..9a70a8707c --- /dev/null +++ b/tests/qemu-iotests/176 @@ -0,0 +1,126 @@ +#!/bin/bash +# +# Commit changes into backing chains and empty the top image if the +# backing image is not explicitly specified. +# +# Variant of 097, which includes snapshots to test different codepath +# in qcow2 +# +# Copyright (C) 2014 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=mreitz@redhat.com + +seq="$(basename $0)" +echo "QA output created by $seq" + +here="$PWD" +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img + _rm_test_img "$TEST_IMG.itmd" +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.pattern + +# Any format supporting backing files and bdrv_make_empty +_supported_fmt qcow2 +_supported_proto file +_supported_os Linux + + +# Four passes: +# 0: Two-layer backing chain, commit to upper backing file (implicitly) +# (in this case, the top image will be emptied) +# 1: Two-layer backing chain, commit to upper backing file (explicitly) +# (in this case, the top image will implicitly stay unchanged) +# 2: Two-layer backing chain, commit to upper backing file (implicitly with -d) +# (in this case, the top image will explicitly stay unchanged) +# 3: Two-layer backing chain, commit to lower backing file +# (in this case, the top image will implicitly stay unchanged) +# +# 020 already tests committing, so this only tests whether image chains are +# working properly and that all images above the base are emptied; therefore, +# no complicated patterns are necessary. Check near the 2G mark, as qcow2 +# has been buggy at that boundary in the past. +for i in 0 1 2 3; do + +echo +echo "=== Test pass $i ===" +echo + +TEST_IMG="$TEST_IMG.base" _make_test_img 2100M +TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" 2100M +_make_test_img -b "$TEST_IMG.itmd" 2100M +$QEMU_IMG snapshot -c snap "$TEST_IMG" + +$QEMU_IO -c 'write -P 1 0x7ffd0000 192k' "$TEST_IMG.base" | _filter_qemu_io +$QEMU_IO -c 'write -P 2 0x7ffe0000 128k' "$TEST_IMG.itmd" | _filter_qemu_io +$QEMU_IO -c 'write -P 3 0x7fff0000 64k' "$TEST_IMG" | _filter_qemu_io + +if [ $i -lt 3 ]; then + if [ $i == 0 ]; then + # -b "$TEST_IMG.itmd" should be the default (that is, committing to the + # first backing file in the chain) + $QEMU_IMG commit "$TEST_IMG" + elif [ $i == 1 ]; then + # explicitly specify the commit target (this should imply -d) + $QEMU_IMG commit -b "$TEST_IMG.itmd" "$TEST_IMG" + else + # do not explicitly specify the commit target, but use -d to leave the + # top image unchanged + $QEMU_IMG commit -d "$TEST_IMG" + fi + + # Bottom should be unchanged + $QEMU_IO -c 'read -P 1 0x7ffd0000 192k' "$TEST_IMG.base" | _filter_qemu_io + + # Intermediate should contain changes from top + $QEMU_IO -c 'read -P 1 0x7ffd0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io + $QEMU_IO -c 'read -P 2 0x7ffe0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io + $QEMU_IO -c 'read -P 3 0x7fff0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io + + # And in pass 0, the top image should be empty, whereas in both other passes + # it should be unchanged (which is both checked by qemu-img map) +else + $QEMU_IMG commit -b "$TEST_IMG.base" "$TEST_IMG" + + # Bottom should contain all changes + $QEMU_IO -c 'read -P 1 0x7ffd0000 64k' "$TEST_IMG.base" | _filter_qemu_io + $QEMU_IO -c 'read -P 2 0x7ffe0000 64k' "$TEST_IMG.base" | _filter_qemu_io + $QEMU_IO -c 'read -P 3 0x7fff0000 64k' "$TEST_IMG.base" | _filter_qemu_io + + # Both top and intermediate should be unchanged +fi + +$QEMU_IMG map "$TEST_IMG.base" | _filter_qemu_img_map +$QEMU_IMG map "$TEST_IMG.itmd" | _filter_qemu_img_map +$QEMU_IMG map "$TEST_IMG" | _filter_qemu_img_map + +done + + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/176.out b/tests/qemu-iotests/176.out new file mode 100644 index 0000000000..fc6365309e --- /dev/null +++ b/tests/qemu-iotests/176.out @@ -0,0 +1,119 @@ +QA output created by 176 + +=== Test pass 0 === + +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +wrote 196608/196608 bytes at offset 2147287040 +192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 131072/131072 bytes at offset 2147352576 +128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 65536/65536 bytes at offset 2147418112 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Image committed. +read 196608/196608 bytes at offset 2147287040 +192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147287040 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147352576 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147418112 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base +Offset Length File +0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base +0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +Offset Length File +0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base +0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd + +=== Test pass 1 === + +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +wrote 196608/196608 bytes at offset 2147287040 +192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 131072/131072 bytes at offset 2147352576 +128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 65536/65536 bytes at offset 2147418112 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Image committed. +read 196608/196608 bytes at offset 2147287040 +192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147287040 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147352576 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147418112 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base +Offset Length File +0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base +0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +Offset Length File +0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base +0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd +0x7fff0000 0x10000 TEST_DIR/t.IMGFMT + +=== Test pass 2 === + +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +wrote 196608/196608 bytes at offset 2147287040 +192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 131072/131072 bytes at offset 2147352576 +128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 65536/65536 bytes at offset 2147418112 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Image committed. +read 196608/196608 bytes at offset 2147287040 +192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147287040 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147352576 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147418112 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base +Offset Length File +0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base +0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +Offset Length File +0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base +0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd +0x7fff0000 0x10000 TEST_DIR/t.IMGFMT + +=== Test pass 3 === + +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +wrote 196608/196608 bytes at offset 2147287040 +192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 131072/131072 bytes at offset 2147352576 +128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 65536/65536 bytes at offset 2147418112 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Image committed. +read 65536/65536 bytes at offset 2147287040 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147352576 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 65536/65536 bytes at offset 2147418112 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base +Offset Length File +0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base +0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +Offset Length File +0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base +0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd +0x7fff0000 0x10000 TEST_DIR/t.IMGFMT +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 1f4bf03185..43142ddfcf 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -168,3 +168,4 @@ 173 rw auto 174 auto 175 auto quick +176 rw auto backing From 0c1bd4692f9a19fb4d4bb3afe45439a09c37ab4c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 31 Mar 2017 13:53:55 -0500 Subject: [PATCH 13/15] qcow2: Discard unaligned tail when wiping image There is a subtle difference between the fast (qcow2v3 with no extra data) and slow path (qcow2v2 format [aka 0.10], or when a snapshot is present) of qcow2_make_empty(). The slow path fails to discard the final (partial) cluster of an unaligned image. The problem stems from the fact that qcow2_discard_clusters() was silently ignoring sub-cluster head and tail on unaligned requests. A quick audit of all callers shows that qcow2_snapshot_create() has always passed a cluster-aligned request since the call was added in commit 1ebf561; qcow2_co_pdiscard() has passed a cluster-aligned request since commit ecdbead taught the block layer about preferred discard alignment; and qcow2_make_empty() was fixed to pass an aligned start (but not necessarily end) in commit a3e1505. Asserting that the start is always aligned also points out that we now have a dead check: rounding the end offset down can never result in a value less than the aligned start offset (the check was rendered dead with commit ecdbead). Meanwhile, we do not want to round the end cluster down in the one case of the end offset matching the (unaligned) file size - that final partial cluster should still be discarded. With those fixes in place, the fast and slow paths are back in sync at discarding an entire image; the next patch will update qemu-iotests to ensure we don't regress. Note that bdrv_co_pdiscard ignores ALL partial cluster requests, including the partial cluster at the end of an image; it can be argued that the partial cluster at the end should be special-cased so that a guest issuing discard requests at proper alignments everywhere else can likewise empty the entire image. But that optimization is left for another day. Signed-off-by: Eric Blake Message-id: 20170331185356.2479-3-eblake@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- block/qcow2-cluster.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 78c11d4948..100398c565 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1519,12 +1519,10 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS); - /* Round start up and end down */ - offset = align_offset(offset, s->cluster_size); - end_offset = start_of_cluster(s, end_offset); - - if (offset > end_offset) { - return 0; + /* The caller must cluster-align start; round end down except at EOF */ + assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); + if (end_offset != bs->total_sectors * BDRV_SECTOR_SIZE) { + end_offset = start_of_cluster(s, end_offset); } nb_clusters = size_to_clusters(s, end_offset - offset); From f82c5b17ea85d8f432069a4afd6cbdb65d9d8a66 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 31 Mar 2017 13:53:56 -0500 Subject: [PATCH 14/15] iotests: Improve image-clear tests on non-aligned image Tweak 097 and 176 to operate on an image that is not cluster-aligned, to give further coverage of clearing out an entire image, including the recent fix to eliminate the difference between fast path (97) and slow (176) for qcow2. Also tested on qcow (97 only, since qcow lacks snapshots). Signed-off-by: Eric Blake Message-id: 20170331185356.2479-4-eblake@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- tests/qemu-iotests/097 | 17 +++++++----- tests/qemu-iotests/097.out | 55 +++++++++++++++++++++++++++++--------- tests/qemu-iotests/176 | 17 +++++++----- tests/qemu-iotests/176.out | 55 +++++++++++++++++++++++++++++--------- 4 files changed, 108 insertions(+), 36 deletions(-) diff --git a/tests/qemu-iotests/097 b/tests/qemu-iotests/097 index 1d28aff99d..e22670c8d0 100755 --- a/tests/qemu-iotests/097 +++ b/tests/qemu-iotests/097 @@ -66,13 +66,15 @@ echo echo "=== Test pass $i ===" echo -TEST_IMG="$TEST_IMG.base" _make_test_img 2100M -TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" 2100M -_make_test_img -b "$TEST_IMG.itmd" 2100M +len=$((2100 * 1024 * 1024 + 512)) # larger than 2G, and not cluster aligned +TEST_IMG="$TEST_IMG.base" _make_test_img $len +TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" $len +_make_test_img -b "$TEST_IMG.itmd" $len -$QEMU_IO -c 'write -P 1 0x7ffd0000 192k' "$TEST_IMG.base" | _filter_qemu_io -$QEMU_IO -c 'write -P 2 0x7ffe0000 128k' "$TEST_IMG.itmd" | _filter_qemu_io -$QEMU_IO -c 'write -P 3 0x7fff0000 64k' "$TEST_IMG" | _filter_qemu_io +$QEMU_IO -c "write -P 1 0x7ffd0000 192k" "$TEST_IMG.base" | _filter_qemu_io +$QEMU_IO -c "write -P 2 0x7ffe0000 128k" "$TEST_IMG.itmd" | _filter_qemu_io +$QEMU_IO -c "write -P 3 0x7fff0000 64k" "$TEST_IMG" | _filter_qemu_io +$QEMU_IO -c "write -P 4 $(($len - 512)) 512" "$TEST_IMG" | _filter_qemu_io if [ $i -lt 3 ]; then if [ $i == 0 ]; then @@ -90,11 +92,13 @@ if [ $i -lt 3 ]; then # Bottom should be unchanged $QEMU_IO -c 'read -P 1 0x7ffd0000 192k' "$TEST_IMG.base" | _filter_qemu_io + $QEMU_IO -c "read -P 0 $((len - 512)) 512" "$TEST_IMG.base" | _filter_qemu_io # Intermediate should contain changes from top $QEMU_IO -c 'read -P 1 0x7ffd0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io $QEMU_IO -c 'read -P 2 0x7ffe0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io $QEMU_IO -c 'read -P 3 0x7fff0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io + $QEMU_IO -c "read -P 4 $((len - 512)) 512" "$TEST_IMG.itmd" | _filter_qemu_io # And in pass 0, the top image should be empty, whereas in both other passes # it should be unchanged (which is both checked by qemu-img map) @@ -105,6 +109,7 @@ else $QEMU_IO -c 'read -P 1 0x7ffd0000 64k' "$TEST_IMG.base" | _filter_qemu_io $QEMU_IO -c 'read -P 2 0x7ffe0000 64k' "$TEST_IMG.base" | _filter_qemu_io $QEMU_IO -c 'read -P 3 0x7fff0000 64k' "$TEST_IMG.base" | _filter_qemu_io + $QEMU_IO -c "read -P 4 $((len - 512)) 512" "$TEST_IMG.base" | _filter_qemu_io # Both top and intermediate should be unchanged fi diff --git a/tests/qemu-iotests/097.out b/tests/qemu-iotests/097.out index 81fc2252a0..f6705a1cc7 100644 --- a/tests/qemu-iotests/097.out +++ b/tests/qemu-iotests/097.out @@ -2,104 +2,130 @@ QA output created by 097 === Test pass 0 === -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202010112 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.itmd wrote 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 131072/131072 bytes at offset 2147352576 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Image committed. read 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147287040 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147352576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File 0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd === Test pass 1 === -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202010112 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.itmd wrote 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 131072/131072 bytes at offset 2147352576 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Image committed. read 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147287040 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147352576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File 0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT +0x83400000 0x200 TEST_DIR/t.IMGFMT === Test pass 2 === -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202010112 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.itmd wrote 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 131072/131072 bytes at offset 2147352576 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Image committed. read 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147287040 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147352576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File 0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT +0x83400000 0x200 TEST_DIR/t.IMGFMT === Test pass 3 === -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202010112 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.itmd wrote 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 131072/131072 bytes at offset 2147352576 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Image committed. read 65536/65536 bytes at offset 2147287040 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) @@ -107,13 +133,18 @@ read 65536/65536 bytes at offset 2147352576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File 0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base +0x83400000 0x200 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT +0x83400000 0x200 TEST_DIR/t.IMGFMT *** done diff --git a/tests/qemu-iotests/176 b/tests/qemu-iotests/176 index 9a70a8707c..950b28720e 100755 --- a/tests/qemu-iotests/176 +++ b/tests/qemu-iotests/176 @@ -69,14 +69,16 @@ echo echo "=== Test pass $i ===" echo -TEST_IMG="$TEST_IMG.base" _make_test_img 2100M -TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" 2100M -_make_test_img -b "$TEST_IMG.itmd" 2100M +len=$((2100 * 1024 * 1024 + 512)) # larger than 2G, and not cluster aligned +TEST_IMG="$TEST_IMG.base" _make_test_img $len +TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" $len +_make_test_img -b "$TEST_IMG.itmd" $len $QEMU_IMG snapshot -c snap "$TEST_IMG" -$QEMU_IO -c 'write -P 1 0x7ffd0000 192k' "$TEST_IMG.base" | _filter_qemu_io -$QEMU_IO -c 'write -P 2 0x7ffe0000 128k' "$TEST_IMG.itmd" | _filter_qemu_io -$QEMU_IO -c 'write -P 3 0x7fff0000 64k' "$TEST_IMG" | _filter_qemu_io +$QEMU_IO -c "write -P 1 0x7ffd0000 192k" "$TEST_IMG.base" | _filter_qemu_io +$QEMU_IO -c "write -P 2 0x7ffe0000 128k" "$TEST_IMG.itmd" | _filter_qemu_io +$QEMU_IO -c "write -P 3 0x7fff0000 64k" "$TEST_IMG" | _filter_qemu_io +$QEMU_IO -c "write -P 4 $(($len - 512)) 512" "$TEST_IMG" | _filter_qemu_io if [ $i -lt 3 ]; then if [ $i == 0 ]; then @@ -94,11 +96,13 @@ if [ $i -lt 3 ]; then # Bottom should be unchanged $QEMU_IO -c 'read -P 1 0x7ffd0000 192k' "$TEST_IMG.base" | _filter_qemu_io + $QEMU_IO -c "read -P 0 $((len - 512)) 512" "$TEST_IMG.base" | _filter_qemu_io # Intermediate should contain changes from top $QEMU_IO -c 'read -P 1 0x7ffd0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io $QEMU_IO -c 'read -P 2 0x7ffe0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io $QEMU_IO -c 'read -P 3 0x7fff0000 64k' "$TEST_IMG.itmd" | _filter_qemu_io + $QEMU_IO -c "read -P 4 $((len - 512)) 512" "$TEST_IMG.itmd" | _filter_qemu_io # And in pass 0, the top image should be empty, whereas in both other passes # it should be unchanged (which is both checked by qemu-img map) @@ -109,6 +113,7 @@ else $QEMU_IO -c 'read -P 1 0x7ffd0000 64k' "$TEST_IMG.base" | _filter_qemu_io $QEMU_IO -c 'read -P 2 0x7ffe0000 64k' "$TEST_IMG.base" | _filter_qemu_io $QEMU_IO -c 'read -P 3 0x7fff0000 64k' "$TEST_IMG.base" | _filter_qemu_io + $QEMU_IO -c "read -P 4 $((len - 512)) 512" "$TEST_IMG.base" | _filter_qemu_io # Both top and intermediate should be unchanged fi diff --git a/tests/qemu-iotests/176.out b/tests/qemu-iotests/176.out index fc6365309e..6271fa7d6f 100644 --- a/tests/qemu-iotests/176.out +++ b/tests/qemu-iotests/176.out @@ -2,104 +2,130 @@ QA output created by 176 === Test pass 0 === -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202010112 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.itmd wrote 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 131072/131072 bytes at offset 2147352576 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Image committed. read 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147287040 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147352576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File 0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd === Test pass 1 === -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202010112 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.itmd wrote 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 131072/131072 bytes at offset 2147352576 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Image committed. read 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147287040 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147352576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File 0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT +0x83400000 0x200 TEST_DIR/t.IMGFMT === Test pass 2 === -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202010112 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.itmd wrote 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 131072/131072 bytes at offset 2147352576 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Image committed. read 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147287040 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147352576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File 0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.itmd Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT +0x83400000 0x200 TEST_DIR/t.IMGFMT === Test pass 3 === -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202009600 -Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.base -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202009600 backing_file=TEST_DIR/t.IMGFMT.itmd +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=2202010112 +Formatting 'TEST_DIR/t.IMGFMT.itmd', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2202010112 backing_file=TEST_DIR/t.IMGFMT.itmd wrote 196608/196608 bytes at offset 2147287040 192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 131072/131072 bytes at offset 2147352576 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Image committed. read 65536/65536 bytes at offset 2147287040 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) @@ -107,13 +133,18 @@ read 65536/65536 bytes at offset 2147352576 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 65536/65536 bytes at offset 2147418112 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 2202009600 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File 0x7ffd0000 0x30000 TEST_DIR/t.IMGFMT.base +0x83400000 0x200 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x20000 TEST_DIR/t.IMGFMT.itmd +0x83400000 0x200 TEST_DIR/t.IMGFMT.base Offset Length File 0x7ffd0000 0x10000 TEST_DIR/t.IMGFMT.base 0x7ffe0000 0x10000 TEST_DIR/t.IMGFMT.itmd 0x7fff0000 0x10000 TEST_DIR/t.IMGFMT +0x83400000 0x200 TEST_DIR/t.IMGFMT *** done From 86d1bd70987cd11d85d01f52aa5824c63d910471 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 31 Mar 2017 19:05:12 +0200 Subject: [PATCH 15/15] block/parallels: Avoid overflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the types of variables in allocate_clusters() to int64_t so we do not have to worry about potential overflows. Add an assertion that our accesses to s->bat[] do not result in a buffer overflow and that the implicit conversion performed when invoking bat_entry_off() does not result in an integer overflow. Coverity-id: 1307776 Signed-off-by: Max Reitz Message-id: 20170331170512.10381-1-mreitz@redhat.com Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Max Reitz --- block/parallels.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 4173b3fb9d..90acf79687 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -192,8 +192,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) { BDRVParallelsState *s = bs->opaque; - uint32_t idx, to_allocate, i; - int64_t pos, space; + int64_t pos, space, idx, to_allocate, i; pos = block_status(s, sector_num, nb_sectors, pnum); if (pos > 0) { @@ -201,11 +200,19 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, } idx = sector_num / s->tracks; - if (idx >= s->bat_size) { - return -EINVAL; - } - to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx; + + /* This function is called only by parallels_co_writev(), which will never + * pass a sector_num at or beyond the end of the image (because the block + * layer never passes such a sector_num to that function). Therefore, idx + * is always below s->bat_size. + * block_status() will limit *pnum so that sector_num + *pnum will not + * exceed the image end. Therefore, idx + to_allocate cannot exceed + * s->bat_size. + * Note that s->bat_size is an unsigned int, therefore idx + to_allocate + * will always fit into a uint32_t. */ + assert(idx < s->bat_size && idx + to_allocate <= s->bat_size); + space = to_allocate * s->tracks; if (s->data_end + space > bdrv_getlength(bs->file->bs) >> BDRV_SECTOR_BITS) { int ret;