Re: [Qemu-block] [PATCH] iotests: Add more "skip_if_unsupported" statements to the python tests
On 8/19/19 9:13 PM, Max Reitz wrote: > On 19.08.19 11:21, Thomas Huth wrote: >> The python code already contains a possibility to skip tests if the >> corresponding driver is not available in the qemu binary - use it >> in more spots to avoid that the tests are failing if the driver has >> been disabled. >> >> Signed-off-by: Thomas Huth >> --- >> tests/qemu-iotests/030 | 3 +++ >> tests/qemu-iotests/040 | 2 ++ >> tests/qemu-iotests/041 | 14 +- >> tests/qemu-iotests/245 | 2 ++ >> 4 files changed, 20 insertions(+), 1 deletion(-) > > [...] > >> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 >> index 26bf1701eb..f45d20fbe0 100755 >> --- a/tests/qemu-iotests/041 >> +++ b/tests/qemu-iotests/041 >> @@ -817,6 +817,7 @@ class TestRepairQuorum(iotests.QMPTestCase): >> image_len = 1 * 1024 * 1024 # MB >> IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ] >> >> +@iotests.skip_if_unsupported(['quorum']) >> def setUp(self): >> self.vm = iotests.VM() > > It’s clear that none of these tests can run if there is no quorum > support, because setUp() creates a quorum node. I think it would be > nice if it would suffice to just skip everything automatically if > setUp() is skipped and not have to bother about each of the test cases. > > Coincidentally (:-)), I have a patch to do that, namely “iotests: Allow > skipping test cases” in my “iotests: Selfish patches” series: > > https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg01106.html > > Yes, that means you cannot use an annotation because it needs @self to > be able to skip the test. Hm... But I think I can make that work by > simply s/case_notrun/args[0].case_skip/ in skip_if_unsupported()? Sure, feel free to ignore my patch here or to modify it according to your reworks. As long as we finally get the iotests into a shape where they are a little bit more flexible wrt the enabled/disabled drivers, I'm happy. Thomas signature.asc Description: OpenPGP digital signature
[Qemu-block] [PATCH v2 1/5] socket: Add backlog parameter to socket_listen
Current parameter was always one. We continue with that value for now
in all callers.
Signed-off-by: Juan Quintela
---
include/qemu/sockets.h| 2 +-
io/channel-socket.c | 2 +-
qga/channel-posix.c | 2 +-
tests/test-util-sockets.c | 12 ++--
util/qemu-sockets.c | 33 ++---
util/trace-events | 2 ++
6 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 8140fea685..57cd049d6e 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -41,7 +41,7 @@ int unix_connect(const char *path, Error **errp);
SocketAddress *socket_parse(const char *str, Error **errp);
int socket_connect(SocketAddress *addr, Error **errp);
-int socket_listen(SocketAddress *addr, Error **errp);
+int socket_listen(SocketAddress *addr, int num, Error **errp);
void socket_listen_cleanup(int fd, Error **errp);
int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
diff --git a/io/channel-socket.c b/io/channel-socket.c
index bec3d931d1..a533c8bc11 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -202,7 +202,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
int fd;
trace_qio_channel_socket_listen_sync(ioc, addr);
-fd = socket_listen(addr, errp);
+fd = socket_listen(addr, 1, errp);
if (fd < 0) {
trace_qio_channel_socket_listen_fail(ioc);
return -1;
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 5a925a9818..8fc205ad21 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -215,7 +215,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar
*path,
return false;
}
-fd = socket_listen(addr, &local_err);
+fd = socket_listen(addr, 1, &local_err);
qapi_free_SocketAddress(addr);
if (local_err != NULL) {
g_critical("%s", error_get_pretty(local_err));
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index f1ebffee5a..c8e1893c11 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -93,7 +93,7 @@ static void test_socket_fd_pass_name_good(void)
g_assert_cmpint(fd, !=, mon_fd);
close(fd);
-fd = socket_listen(&addr, &error_abort);
+fd = socket_listen(&addr, 1, &error_abort);
g_assert_cmpint(fd, !=, -1);
g_assert_cmpint(fd, !=, mon_fd);
close(fd);
@@ -124,7 +124,7 @@ static void test_socket_fd_pass_name_bad(void)
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
-fd = socket_listen(&addr, &err);
+fd = socket_listen(&addr, 1, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -151,7 +151,7 @@ static void test_socket_fd_pass_name_nomon(void)
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
-fd = socket_listen(&addr, &err);
+fd = socket_listen(&addr, 1, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -174,7 +174,7 @@ static void test_socket_fd_pass_num_good(void)
fd = socket_connect(&addr, &error_abort);
g_assert_cmpint(fd, ==, sfd);
-fd = socket_listen(&addr, &error_abort);
+fd = socket_listen(&addr, 1, &error_abort);
g_assert_cmpint(fd, ==, sfd);
g_free(addr.u.fd.str);
@@ -197,7 +197,7 @@ static void test_socket_fd_pass_num_bad(void)
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
-fd = socket_listen(&addr, &err);
+fd = socket_listen(&addr, 1, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -220,7 +220,7 @@ static void test_socket_fd_pass_num_nocli(void)
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
-fd = socket_listen(&addr, &err);
+fd = socket_listen(&addr, 1, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index e3a1666578..3f0a80404f 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -31,6 +31,7 @@
#include "qapi/qobject-input-visitor.h"
#include "qapi/qobject-output-visitor.h"
#include "qemu/cutils.h"
+#include "trace.h"
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -207,6 +208,7 @@ static int try_bind(int socket, InetSocketAddress *saddr,
struct addrinfo *e)
static int inet_listen_saddr(InetSocketAddress *saddr,
int port_offset,
+ int num,
Error **errp)
{
struct addrinfo ai,*res,*e;
@@ -309,7 +311,8 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
goto listen_failed;
}
} else {
-if (!listen(slisten, 1)) {
+trace_inet_listen_saddr(num);
+if (!listen(slisten, num)) {
goto listen_ok;
}
if (errno != EADDRINUSE) {
@@ -774,6 +777,7 @@ stat
[Qemu-block] [PATCH v2 0/5] Fix multifd with big number of channels
Hi In this v2: - dropped the already included patches upstream - dropped the semaphore changes - add backlog listen parameter through all the call chain (danp suggestion) - also add the change for qio_channel_socket_async(), for consistency (it has zero users on tree anyways) - for "fd" case, I just give an error if expected number of connections is bigger than 1. - All patches except the multifd one should be noops (i.e. I pass everywhere the number of expected channels as one). With this changes, I can migrate with 100 channels consistently. It always work. Please, review. Juan Quintela (5): socket: Add backlog parameter to socket_listen socket: Add num connections to qio_channel_socket_sync() socket: Add num connections to qio_channel_socket_async() socket: Add num connections to qio_net_listener_open_sync() multifd: Use number of channels as listen backlog blockdev-nbd.c | 2 +- chardev/char-socket.c | 2 +- include/io/channel-socket.h| 4 include/io/net-listener.h | 2 ++ include/qemu/sockets.h | 2 +- io/channel-socket.c| 35 +- io/net-listener.c | 3 ++- io/trace-events| 4 ++-- migration/socket.c | 7 ++- qemu-nbd.c | 2 +- qga/channel-posix.c| 2 +- scsi/qemu-pr-helper.c | 3 ++- tests/test-char.c | 4 ++-- tests/test-io-channel-socket.c | 4 ++-- tests/test-util-sockets.c | 12 ++-- tests/tpm-emu.c| 2 +- ui/vnc.c | 4 ++-- util/qemu-sockets.c| 33 +--- util/trace-events | 2 ++ 19 files changed, 86 insertions(+), 43 deletions(-) -- 2.21.0
[Qemu-block] [PATCH v2 2/5] socket: Add num connections to qio_channel_socket_sync()
Signed-off-by: Juan Quintela
---
include/io/channel-socket.h| 2 ++
io/channel-socket.c| 7 ---
io/net-listener.c | 2 +-
io/trace-events| 2 +-
scsi/qemu-pr-helper.c | 2 +-
tests/test-char.c | 4 ++--
tests/test-io-channel-socket.c | 2 +-
tests/tpm-emu.c| 2 +-
8 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index d7134d2cd6..ed88e5b8c1 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -123,6 +123,7 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
* qio_channel_socket_listen_sync:
* @ioc: the socket channel object
* @addr: the address to listen to
+ * @num: the expected ammount of connections
* @errp: pointer to a NULL-initialized error object
*
* Attempt to listen to the address @addr. This method
@@ -132,6 +133,7 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
*/
int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
SocketAddress *addr,
+ int num,
Error **errp);
/**
diff --git a/io/channel-socket.c b/io/channel-socket.c
index a533c8bc11..6258c25983 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -197,12 +197,13 @@ void qio_channel_socket_connect_async(QIOChannelSocket
*ioc,
int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
SocketAddress *addr,
+ int num,
Error **errp)
{
int fd;
-trace_qio_channel_socket_listen_sync(ioc, addr);
-fd = socket_listen(addr, 1, errp);
+trace_qio_channel_socket_listen_sync(ioc, addr, num);
+fd = socket_listen(addr, num, errp);
if (fd < 0) {
trace_qio_channel_socket_listen_fail(ioc);
return -1;
@@ -226,7 +227,7 @@ static void qio_channel_socket_listen_worker(QIOTask *task,
SocketAddress *addr = opaque;
Error *err = NULL;
-qio_channel_socket_listen_sync(ioc, addr, &err);
+qio_channel_socket_listen_sync(ioc, addr, 1, &err);
qio_task_set_error(task, err);
}
diff --git a/io/net-listener.c b/io/net-listener.c
index d8cfe52673..dc81150318 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -82,7 +82,7 @@ int qio_net_listener_open_sync(QIONetListener *listener,
for (i = 0; i < nresaddrs; i++) {
QIOChannelSocket *sioc = qio_channel_socket_new();
-if (qio_channel_socket_listen_sync(sioc, resaddrs[i],
+if (qio_channel_socket_listen_sync(sioc, resaddrs[i], 1,
err ? NULL : &err) == 0) {
success = true;
diff --git a/io/trace-events b/io/trace-events
index 378390521e..2e6aa1d749 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -17,7 +17,7 @@ qio_channel_socket_connect_sync(void *ioc, void *addr)
"Socket connect sync ioc=
qio_channel_socket_connect_async(void *ioc, void *addr) "Socket connect async
ioc=%p addr=%p"
qio_channel_socket_connect_fail(void *ioc) "Socket connect fail ioc=%p"
qio_channel_socket_connect_complete(void *ioc, int fd) "Socket connect
complete ioc=%p fd=%d"
-qio_channel_socket_listen_sync(void *ioc, void *addr) "Socket listen sync
ioc=%p addr=%p"
+qio_channel_socket_listen_sync(void *ioc, void *addr, int num) "Socket listen
sync ioc=%p addr=%p num=%d"
qio_channel_socket_listen_async(void *ioc, void *addr) "Socket listen async
ioc=%p addr=%p"
qio_channel_socket_listen_fail(void *ioc) "Socket listen fail ioc=%p"
qio_channel_socket_listen_complete(void *ioc, int fd) "Socket listen complete
ioc=%p fd=%d"
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index a256ce490b..f960d8303b 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -1005,7 +1005,7 @@ int main(int argc, char **argv)
.u.q_unix.path = socket_path,
};
server_ioc = qio_channel_socket_new();
-if (qio_channel_socket_listen_sync(server_ioc, &saddr, &local_err) <
0) {
+if (qio_channel_socket_listen_sync(server_ioc, &saddr, 1, &local_err)
< 0) {
object_unref(OBJECT(server_ioc));
error_report_err(local_err);
return 1;
diff --git a/tests/test-char.c b/tests/test-char.c
index f9440cdcfd..af131fc850 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -666,7 +666,7 @@ char_socket_addr_to_opt_str(SocketAddress *addr, bool
fd_pass,
char *optstr;
g_assert(!reconnect);
if (is_listen) {
-qio_channel_socket_listen_sync(ioc, addr, &error_abort);
+qio_channel_socket_listen_sync(ioc, addr, 1, &error_abort);
} else {
qio_channel_socket_connect_sync(ioc, addr, &error_abort);
}
@@ -891,7 +891,7 @@ static void char_socket_client_test(gconstpointer opaque)
*/
ioc = qio
[Qemu-block] [PATCH v2 4/5] socket: Add num connections to qio_net_listener_open_sync()
Signed-off-by: Juan Quintela
---
blockdev-nbd.c| 2 +-
chardev/char-socket.c | 2 +-
include/io/net-listener.h | 2 ++
io/net-listener.c | 3 ++-
migration/socket.c| 2 +-
qemu-nbd.c| 2 +-
scsi/qemu-pr-helper.c | 3 ++-
ui/vnc.c | 4 ++--
8 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 7a71da447f..c621686131 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -101,7 +101,7 @@ void nbd_server_start(SocketAddress *addr, const char
*tls_creds,
qio_net_listener_set_name(nbd_server->listener,
"nbd-listener");
-if (qio_net_listener_open_sync(nbd_server->listener, addr, errp) < 0) {
+if (qio_net_listener_open_sync(nbd_server->listener, addr, 1, errp) < 0) {
goto error;
}
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 7ca5d97af3..8c7c9da567 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1160,7 +1160,7 @@ static int qmp_chardev_open_socket_server(Chardev *chr,
qio_net_listener_set_name(s->listener, name);
g_free(name);
-if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
+if (qio_net_listener_open_sync(s->listener, s->addr, 1, errp) < 0) {
object_unref(OBJECT(s->listener));
s->listener = NULL;
return -1;
diff --git a/include/io/net-listener.h b/include/io/net-listener.h
index 8081ac58a2..fb101703e3 100644
--- a/include/io/net-listener.h
+++ b/include/io/net-listener.h
@@ -95,6 +95,7 @@ void qio_net_listener_set_name(QIONetListener *listener,
* qio_net_listener_open_sync:
* @listener: the network listener object
* @addr: the address to listen on
+ * @num: the amount of expected connections
* @errp: pointer to a NULL initialized error object
*
* Synchronously open a listening connection on all
@@ -104,6 +105,7 @@ void qio_net_listener_set_name(QIONetListener *listener,
*/
int qio_net_listener_open_sync(QIONetListener *listener,
SocketAddress *addr,
+ int num,
Error **errp);
/**
diff --git a/io/net-listener.c b/io/net-listener.c
index dc81150318..5d8a226872 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -62,6 +62,7 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
int qio_net_listener_open_sync(QIONetListener *listener,
SocketAddress *addr,
+ int num,
Error **errp)
{
QIODNSResolver *resolver = qio_dns_resolver_get_instance();
@@ -82,7 +83,7 @@ int qio_net_listener_open_sync(QIONetListener *listener,
for (i = 0; i < nresaddrs; i++) {
QIOChannelSocket *sioc = qio_channel_socket_new();
-if (qio_channel_socket_listen_sync(sioc, resaddrs[i], 1,
+if (qio_channel_socket_listen_sync(sioc, resaddrs[i], num,
err ? NULL : &err) == 0) {
success = true;
diff --git a/migration/socket.c b/migration/socket.c
index 98efdc0286..e63f5e1612 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -181,7 +181,7 @@ static void socket_start_incoming_migration(SocketAddress
*saddr,
qio_net_listener_set_name(listener, "migration-socket-listener");
-if (qio_net_listener_open_sync(listener, saddr, errp) < 0) {
+if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
object_unref(OBJECT(listener));
return;
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 049645491d..83b6c32d73 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1054,7 +1054,7 @@ int main(int argc, char **argv)
server = qio_net_listener_new();
if (socket_activation == 0) {
saddr = nbd_build_socket_address(sockpath, bindto, port);
-if (qio_net_listener_open_sync(server, saddr, &local_err) < 0) {
+if (qio_net_listener_open_sync(server, saddr, 1, &local_err) < 0) {
object_unref(OBJECT(server));
error_report_err(local_err);
exit(EXIT_FAILURE);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index f960d8303b..2d2edded69 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -1005,7 +1005,8 @@ int main(int argc, char **argv)
.u.q_unix.path = socket_path,
};
server_ioc = qio_channel_socket_new();
-if (qio_channel_socket_listen_sync(server_ioc, &saddr, 1, &local_err)
< 0) {
+if (qio_channel_socket_listen_sync(server_ioc, &saddr, 1,
+ &local_err) < 0) {
object_unref(OBJECT(server_ioc));
error_report_err(local_err);
return 1;
diff --git a/ui/vnc.c b/ui/vnc.c
index 4812ed29d0..258461f814 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3762,7 +3762,7 @@ static int vnc_display_listen(VncDisplay *vd,
qio_net_listener_set_
[Qemu-block] [PATCH v2 3/5] socket: Add num connections to qio_channel_socket_async()
Signed-off-by: Juan Quintela
---
include/io/channel-socket.h| 2 ++
io/channel-socket.c| 30 +++---
io/trace-events| 2 +-
tests/test-io-channel-socket.c | 2 +-
4 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index ed88e5b8c1..777ff5954e 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -140,6 +140,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
* qio_channel_socket_listen_async:
* @ioc: the socket channel object
* @addr: the address to listen to
+ * @num: the expected ammount of connections
* @callback: the function to invoke on completion
* @opaque: user data to pass to @callback
* @destroy: the function to free @opaque
@@ -155,6 +156,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
*/
void qio_channel_socket_listen_async(QIOChannelSocket *ioc,
SocketAddress *addr,
+ int num,
QIOTaskFunc callback,
gpointer opaque,
GDestroyNotify destroy,
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 6258c25983..b74f5b92a0 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -220,14 +220,27 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
}
+struct QIOChannelListenWorkerData {
+SocketAddress *addr;
+int num; /* amount of expected connections */
+};
+
+static void qio_channel_listen_worker_free(gpointer opaque)
+{
+struct QIOChannelListenWorkerData *data = opaque;
+
+qapi_free_SocketAddress(data->addr);
+g_free(data);
+}
+
static void qio_channel_socket_listen_worker(QIOTask *task,
gpointer opaque)
{
QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
-SocketAddress *addr = opaque;
+struct QIOChannelListenWorkerData *data = opaque;
Error *err = NULL;
-qio_channel_socket_listen_sync(ioc, addr, 1, &err);
+qio_channel_socket_listen_sync(ioc, data->addr, data->num, &err);
qio_task_set_error(task, err);
}
@@ -235,6 +248,7 @@ static void qio_channel_socket_listen_worker(QIOTask *task,
void qio_channel_socket_listen_async(QIOChannelSocket *ioc,
SocketAddress *addr,
+ int num,
QIOTaskFunc callback,
gpointer opaque,
GDestroyNotify destroy,
@@ -242,16 +256,18 @@ void qio_channel_socket_listen_async(QIOChannelSocket
*ioc,
{
QIOTask *task = qio_task_new(
OBJECT(ioc), callback, opaque, destroy);
-SocketAddress *addrCopy;
+struct QIOChannelListenWorkerData *data;
-addrCopy = QAPI_CLONE(SocketAddress, addr);
+data = g_new0(struct QIOChannelListenWorkerData, 1);
+data->addr = QAPI_CLONE(SocketAddress, addr);
+data->num = num;
/* socket_listen() blocks in DNS lookups, so we must use a thread */
-trace_qio_channel_socket_listen_async(ioc, addr);
+trace_qio_channel_socket_listen_async(ioc, addr, num);
qio_task_run_in_thread(task,
qio_channel_socket_listen_worker,
- addrCopy,
- (GDestroyNotify)qapi_free_SocketAddress,
+ data,
+ qio_channel_listen_worker_free,
context);
}
diff --git a/io/trace-events b/io/trace-events
index 2e6aa1d749..d7bc70b966 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -18,7 +18,7 @@ qio_channel_socket_connect_async(void *ioc, void *addr)
"Socket connect async io
qio_channel_socket_connect_fail(void *ioc) "Socket connect fail ioc=%p"
qio_channel_socket_connect_complete(void *ioc, int fd) "Socket connect
complete ioc=%p fd=%d"
qio_channel_socket_listen_sync(void *ioc, void *addr, int num) "Socket listen
sync ioc=%p addr=%p num=%d"
-qio_channel_socket_listen_async(void *ioc, void *addr) "Socket listen async
ioc=%p addr=%p"
+qio_channel_socket_listen_async(void *ioc, void *addr, int num) "Socket listen
async ioc=%p addr=%p num=%d"
qio_channel_socket_listen_fail(void *ioc) "Socket listen fail ioc=%p"
qio_channel_socket_listen_complete(void *ioc, int fd) "Socket listen complete
ioc=%p fd=%d"
qio_channel_socket_dgram_sync(void *ioc, void *localAddr, void *remoteAddr)
"Socket dgram sync ioc=%p localAddr=%p remoteAddr=%p"
diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index 6eebcee115..50235c1547 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -113,7 +113,7 @@ static void test_io_channel_setup_async(SocketAddress
*listen_addr,
lioc = qio_channel_socket_new();
qio_channel_so
[Qemu-block] [PATCH v2 5/5] multifd: Use number of channels as listen backlog
Signed-off-by: Juan Quintela
---
migration/socket.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/migration/socket.c b/migration/socket.c
index e63f5e1612..97c9efde59 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -178,10 +178,15 @@ static void socket_start_incoming_migration(SocketAddress
*saddr,
{
QIONetListener *listener = qio_net_listener_new();
size_t i;
+int num = 1;
qio_net_listener_set_name(listener, "migration-socket-listener");
-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
+if (migrate_use_multifd()) {
+num = migrate_multifd_channels();
+}
+
+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
object_unref(OBJECT(listener));
return;
}
--
2.21.0
Re: [Qemu-block] [PATCH 0/4] backup: fix skipping unallocated clusters
16.08.2019 22:11, John Snow wrote: > > > On 8/14/19 12:54 PM, Vladimir Sementsov-Ogievskiy wrote: >> >> >> 14 авг. 2019 г. 17:43 пользователь Vladimir Sementsov-Ogievskiy >> написал: >> >> Hi all! >> >> There is a bug in not yet merged patch >> "block/backup: teach TOP to never copy unallocated regions" >> in https://github.com/jnsnow/qemu bitmaps. 04 fixes it. So, I propose >> to put 01-03 somewhere before >> "block/backup: teach TOP to never copy unallocated regions" >> and squash 04 into "block/backup: teach TOP to never copy >> unallocated regions" >> >> >> Hmm, don't bother with it. Simpler is fix the bug in your commit by just >> use skip_bytes variable when initializing dirty_end. >> > > OK, just use Max's fix instead of this entire 4 patch series? > > --js > Yes, I think it's OK -- Best regards, Vladimir
Re: [Qemu-block] [PATCH v2 1/5] socket: Add backlog parameter to socket_listen
On Tue, Aug 20, 2019 at 10:24:55AM +0200, Juan Quintela wrote:
> Current parameter was always one. We continue with that value for now
> in all callers.
>
> Signed-off-by: Juan Quintela
> ---
> include/qemu/sockets.h| 2 +-
> io/channel-socket.c | 2 +-
> qga/channel-posix.c | 2 +-
> tests/test-util-sockets.c | 12 ++--
> util/qemu-sockets.c | 33 ++---
> util/trace-events | 2 ++
> 6 files changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index e3a1666578..3f0a80404f 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -31,6 +31,7 @@
> #include "qapi/qobject-input-visitor.h"
> #include "qapi/qobject-output-visitor.h"
> #include "qemu/cutils.h"
> +#include "trace.h"
>
> #ifndef AI_ADDRCONFIG
> # define AI_ADDRCONFIG 0
> @@ -207,6 +208,7 @@ static int try_bind(int socket, InetSocketAddress *saddr,
> struct addrinfo *e)
>
> static int inet_listen_saddr(InetSocketAddress *saddr,
> int port_offset,
> + int num,
> Error **errp)
> {
> struct addrinfo ai,*res,*e;
> @@ -309,7 +311,8 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> goto listen_failed;
> }
> } else {
> -if (!listen(slisten, 1)) {
> +trace_inet_listen_saddr(num);
It is a bit odd to only have the trace event for inet sockets. I'd
prefer it in the caller for all sockets, with just "socket_listen"
name.
> +if (!listen(slisten, num)) {
> goto listen_ok;
> }
> if (errno != EADDRINUSE) {
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH v2 2/5] socket: Add num connections to qio_channel_socket_sync()
On Tue, Aug 20, 2019 at 10:24:56AM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > include/io/channel-socket.h| 2 ++ > io/channel-socket.c| 7 --- > io/net-listener.c | 2 +- > io/trace-events| 2 +- > scsi/qemu-pr-helper.c | 2 +- > tests/test-char.c | 4 ++-- > tests/test-io-channel-socket.c | 2 +- > tests/tpm-emu.c| 2 +- > 8 files changed, 13 insertions(+), 10 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH v2 3/5] socket: Add num connections to qio_channel_socket_async()
On Tue, Aug 20, 2019 at 10:24:57AM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > include/io/channel-socket.h| 2 ++ > io/channel-socket.c| 30 +++--- > io/trace-events| 2 +- > tests/test-io-channel-socket.c | 2 +- > 4 files changed, 27 insertions(+), 9 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH v2 5/5] multifd: Use number of channels as listen backlog
On Tue, Aug 20, 2019 at 10:24:59AM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > migration/socket.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH v2 4/5] socket: Add num connections to qio_net_listener_open_sync()
On Tue, Aug 20, 2019 at 10:24:58AM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > blockdev-nbd.c| 2 +- > chardev/char-socket.c | 2 +- > include/io/net-listener.h | 2 ++ > io/net-listener.c | 3 ++- > migration/socket.c| 2 +- > qemu-nbd.c| 2 +- > scsi/qemu-pr-helper.c | 3 ++- > ui/vnc.c | 4 ++-- > 8 files changed, 12 insertions(+), 8 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH] nbd: Tolerate more errors to structured reply request
19.08.2019 20:57, Eric Blake wrote:
> A server may have a reason to reject a request for structured replies,
> beyond just not recognizing them as a valid request. It doesn't hurt
> us to continue talking to such a server; otherwise 'qemu-nbd --list'
> of such a server fails to display all possible details about the
> export.
>
> Encountered when temporarily tweaking nbdkit to reply with
> NBD_REP_ERR_POLICY. Present since structured reply support was first
> added (commit d795299b reused starttls handling, but starttls has to
> reject all errors).
>
> Signed-off-by: Eric Blake
> ---
> nbd/client.c | 39 +++
> 1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 8f524c3e3502..204f6e928d14 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2016-2018 Red Hat, Inc.
> + * Copyright (C) 2016-2019 Red Hat, Inc.
>* Copyright (C) 2005 Anthony Liguori
>*
>* Network Block Device Client Side
> @@ -141,17 +141,19 @@ static int nbd_receive_option_reply(QIOChannel *ioc,
> uint32_t opt,
> return 0;
> }
>
> -/* If reply represents success, return 1 without further action.
> - * If reply represents an error, consume the optional payload of
> - * the packet on ioc. Then return 0 for unsupported (so the client
> - * can fall back to other approaches), or -1 with errp set for other
> - * errors.
> +/*
> + * If reply represents success, return 1 without further action. If
> + * reply represents an error, consume the optional payload of the
> + * packet on ioc. Then return 0 for unsupported (so the client can
> + * fall back to other approaches), where @strict determines if only
> + * ERR_UNSUP or all errors fit that category, or -1 with errp set for
hmm not all but "errors returned by server accordingly to protocol", as "all"
a bit in contrast with following "result = -1", but it's OK as is anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy
> + * other errors.
>*/
> static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> -Error **errp)
> +bool strict, Error **errp)
> {
> char *msg = NULL;
> -int result = -1;
> +int result = strict ? -1 : 0;
>
> if (!(reply->type & (1 << 31))) {
> return 1;
> @@ -162,6 +164,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc,
> NBDOptionReply *reply,
> error_setg(errp, "server error %" PRIu32
> " (%s) message is too long",
> reply->type, nbd_rep_lookup(reply->type));
> +result = -1;
> goto cleanup;
> }
> msg = g_malloc(reply->length + 1);
> @@ -169,6 +172,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc,
> NBDOptionReply *reply,
> error_prepend(errp, "Failed to read option error %" PRIu32
> " (%s) message: ",
> reply->type, nbd_rep_lookup(reply->type));
> +result = -1;
> goto cleanup;
> }
> msg[reply->length] = '\0';
> @@ -257,7 +261,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name,
> char **description,
> if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
> return -1;
> }
> -error = nbd_handle_reply_err(ioc, &reply, errp);
> +error = nbd_handle_reply_err(ioc, &reply, true, errp);
> if (error <= 0) {
> return error;
> }
> @@ -370,7 +374,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t
> opt,
> if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
> return -1;
> }
> -error = nbd_handle_reply_err(ioc, &reply, errp);
> +error = nbd_handle_reply_err(ioc, &reply, true, errp);
> if (error <= 0) {
> return error;
> }
> @@ -545,12 +549,15 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
> }
> }
>
> -/* nbd_request_simple_option: Send an option request, and parse the reply
> +/*
> + * nbd_request_simple_option: Send an option request, and parse the reply.
> + * @strict controls whether ERR_UNSUP or all errors produce 0 status.
>* return 1 for successful negotiation,
>*0 if operation is unsupported,
>*-1 with errp set for any other error
>*/
> -static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
> +static int nbd_request_simple_option(QIOChannel *ioc, int opt, bool strict,
> + Error **errp)
> {
> NBDOptionReply reply;
> int error;
> @@ -562,7 +569,7 @@ static int nbd_request_simple_option(QIOChannel *ioc, int
> opt, Error **errp)
> if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
> return -1;
> }
> -error = nbd_handle_reply_err(ioc, &reply, errp)
Re: [Qemu-block] [PATCH] nbd: Advertise multi-conn for shared read-only connections
17.08.2019 17:30, Eric Blake wrote:
> On 8/16/19 5:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>
+++ b/blockdev-nbd.c
@@ -189,7 +189,7 @@ void qmp_nbd_server_add(const char *device, bool
has_name, const char *name,
}
exp = nbd_export_new(bs, 0, len, name, NULL, bitmap,
- writable ? 0 : NBD_FLAG_READ_ONLY,
+ writable ? 0 : NBD_FLAG_READ_ONLY, true,
>>>
>>> s/true/!writable ?
>>
>> Oh, I see, John already noticed this, it's checked in nbd_export_new anyway..
>
> Still, since two reviewers have caught it, I'm fixing it :)
With it or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy
>
>
@@ -1486,6 +1486,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
uint64_t dev_offset,
perm = BLK_PERM_CONSISTENT_READ;
if ((nbdflags & NBD_FLAG_READ_ONLY) == 0) {
perm |= BLK_PERM_WRITE;
+ } else if (shared) {
+ nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
>>
>> For me it looks a bit strange: we already have nbdflags parameter for
>> nbd_export_new(), why
>> to add a separate boolean to pass one of nbdflags flags?
>
> Because I want to get rid of the nbdflags in my next patch.
>
>>
>> Also, for qemu-nbd, shouldn't we allow -e only together with -r ?
>
> I'm reluctant to; it might break whatever existing user is okay exposing
> it (although such users are questionable, so maybe we can argue they
> were already broken). Maybe it's time to start a deprecation cycle?
>
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v2 1/5] socket: Add backlog parameter to socket_listen
Daniel P. Berrangé wrote:
> On Tue, Aug 20, 2019 at 10:24:55AM +0200, Juan Quintela wrote:
>> Current parameter was always one. We continue with that value for now
>> in all callers.
>>
>> Signed-off-by: Juan Quintela
>> @@ -309,7 +311,8 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>> goto listen_failed;
>> }
>> } else {
>> -if (!listen(slisten, 1)) {
>> +trace_inet_listen_saddr(num);
>
> It is a bit odd to only have the trace event for inet sockets. I'd
> prefer it in the caller for all sockets, with just "socket_listen"
> name.
Ok. will change.
This is the one that I needed, I just changed an error_report() to a
trace O:-)
Re: [Qemu-block] [Qemu-devel] Difference between commit and rebase
Cc'ing qemu-block@ On 8/20/19 11:27 AM, lampahome wrote: > I want to remove snapshots and I found two ways: > qemu-img commit > qemu-img rebase > > I found they both can choose where to rebase(merge) the images. > commit can truncate or not on specific image. > rebase won't truncate rebased image. > > I found they have something similarity and I don't know what situation is > suitable for commit or rebase?
[Qemu-block] [PATCH v3 0/5] Fix multifd with big number of channels
Hi In this version: - updated to latest upstream - moved trace to suggested position (danp) Please review. Later, Juan. Juan Quintela (5): socket: Add backlog parameter to socket_listen socket: Add num connections to qio_channel_socket_sync() socket: Add num connections to qio_channel_socket_async() socket: Add num connections to qio_net_listener_open_sync() multifd: Use number of channels as listen backlog blockdev-nbd.c | 2 +- chardev/char-socket.c | 2 +- include/io/channel-socket.h| 4 include/io/net-listener.h | 2 ++ include/qemu/sockets.h | 2 +- io/channel-socket.c| 35 +- io/net-listener.c | 3 ++- io/trace-events| 4 ++-- migration/socket.c | 7 ++- qemu-nbd.c | 2 +- qga/channel-posix.c| 2 +- scsi/qemu-pr-helper.c | 3 ++- tests/test-char.c | 4 ++-- tests/test-io-channel-socket.c | 4 ++-- tests/test-util-sockets.c | 12 ++-- tests/tpm-emu.c| 2 +- ui/vnc.c | 4 ++-- util/qemu-sockets.c| 33 +--- util/trace-events | 3 +++ 19 files changed, 87 insertions(+), 43 deletions(-) -- 2.21.0
[Qemu-block] [PATCH v3 1/5] socket: Add backlog parameter to socket_listen
Current parameter was always one. We continue with that value for now
in all callers.
Signed-off-by: Juan Quintela
---
Moved trace to socket_listen
---
include/qemu/sockets.h| 2 +-
io/channel-socket.c | 2 +-
qga/channel-posix.c | 2 +-
tests/test-util-sockets.c | 12 ++--
util/qemu-sockets.c | 33 ++---
util/trace-events | 3 +++
6 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 8140fea685..57cd049d6e 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -41,7 +41,7 @@ int unix_connect(const char *path, Error **errp);
SocketAddress *socket_parse(const char *str, Error **errp);
int socket_connect(SocketAddress *addr, Error **errp);
-int socket_listen(SocketAddress *addr, Error **errp);
+int socket_listen(SocketAddress *addr, int num, Error **errp);
void socket_listen_cleanup(int fd, Error **errp);
int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
diff --git a/io/channel-socket.c b/io/channel-socket.c
index bec3d931d1..a533c8bc11 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -202,7 +202,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
int fd;
trace_qio_channel_socket_listen_sync(ioc, addr);
-fd = socket_listen(addr, errp);
+fd = socket_listen(addr, 1, errp);
if (fd < 0) {
trace_qio_channel_socket_listen_fail(ioc);
return -1;
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 5a925a9818..8fc205ad21 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -215,7 +215,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar
*path,
return false;
}
-fd = socket_listen(addr, &local_err);
+fd = socket_listen(addr, 1, &local_err);
qapi_free_SocketAddress(addr);
if (local_err != NULL) {
g_critical("%s", error_get_pretty(local_err));
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index f1ebffee5a..c8e1893c11 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -93,7 +93,7 @@ static void test_socket_fd_pass_name_good(void)
g_assert_cmpint(fd, !=, mon_fd);
close(fd);
-fd = socket_listen(&addr, &error_abort);
+fd = socket_listen(&addr, 1, &error_abort);
g_assert_cmpint(fd, !=, -1);
g_assert_cmpint(fd, !=, mon_fd);
close(fd);
@@ -124,7 +124,7 @@ static void test_socket_fd_pass_name_bad(void)
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
-fd = socket_listen(&addr, &err);
+fd = socket_listen(&addr, 1, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -151,7 +151,7 @@ static void test_socket_fd_pass_name_nomon(void)
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
-fd = socket_listen(&addr, &err);
+fd = socket_listen(&addr, 1, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -174,7 +174,7 @@ static void test_socket_fd_pass_num_good(void)
fd = socket_connect(&addr, &error_abort);
g_assert_cmpint(fd, ==, sfd);
-fd = socket_listen(&addr, &error_abort);
+fd = socket_listen(&addr, 1, &error_abort);
g_assert_cmpint(fd, ==, sfd);
g_free(addr.u.fd.str);
@@ -197,7 +197,7 @@ static void test_socket_fd_pass_num_bad(void)
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
-fd = socket_listen(&addr, &err);
+fd = socket_listen(&addr, 1, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
@@ -220,7 +220,7 @@ static void test_socket_fd_pass_num_nocli(void)
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
-fd = socket_listen(&addr, &err);
+fd = socket_listen(&addr, 1, &err);
g_assert_cmpint(fd, ==, -1);
error_free_or_abort(&err);
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index e3a1666578..98ff3a1cce 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -31,6 +31,7 @@
#include "qapi/qobject-input-visitor.h"
#include "qapi/qobject-output-visitor.h"
#include "qemu/cutils.h"
+#include "trace.h"
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -207,6 +208,7 @@ static int try_bind(int socket, InetSocketAddress *saddr,
struct addrinfo *e)
static int inet_listen_saddr(InetSocketAddress *saddr,
int port_offset,
+ int num,
Error **errp)
{
struct addrinfo ai,*res,*e;
@@ -309,7 +311,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
goto listen_failed;
}
} else {
-if (!listen(slisten, 1)) {
+if (!listen(slisten, num)) {
goto listen_ok;
}
if (errno != EADDRINUSE) {
@@ -774,6 +776,7 @@ static int vsock
[Qemu-block] [PATCH v3 4/5] socket: Add num connections to qio_net_listener_open_sync()
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Juan Quintela
---
blockdev-nbd.c| 2 +-
chardev/char-socket.c | 2 +-
include/io/net-listener.h | 2 ++
io/net-listener.c | 3 ++-
migration/socket.c| 2 +-
qemu-nbd.c| 2 +-
ui/vnc.c | 4 ++--
7 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 7a71da447f..c621686131 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -101,7 +101,7 @@ void nbd_server_start(SocketAddress *addr, const char
*tls_creds,
qio_net_listener_set_name(nbd_server->listener,
"nbd-listener");
-if (qio_net_listener_open_sync(nbd_server->listener, addr, errp) < 0) {
+if (qio_net_listener_open_sync(nbd_server->listener, addr, 1, errp) < 0) {
goto error;
}
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 7ca5d97af3..8c7c9da567 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1160,7 +1160,7 @@ static int qmp_chardev_open_socket_server(Chardev *chr,
qio_net_listener_set_name(s->listener, name);
g_free(name);
-if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
+if (qio_net_listener_open_sync(s->listener, s->addr, 1, errp) < 0) {
object_unref(OBJECT(s->listener));
s->listener = NULL;
return -1;
diff --git a/include/io/net-listener.h b/include/io/net-listener.h
index 8081ac58a2..fb101703e3 100644
--- a/include/io/net-listener.h
+++ b/include/io/net-listener.h
@@ -95,6 +95,7 @@ void qio_net_listener_set_name(QIONetListener *listener,
* qio_net_listener_open_sync:
* @listener: the network listener object
* @addr: the address to listen on
+ * @num: the amount of expected connections
* @errp: pointer to a NULL initialized error object
*
* Synchronously open a listening connection on all
@@ -104,6 +105,7 @@ void qio_net_listener_set_name(QIONetListener *listener,
*/
int qio_net_listener_open_sync(QIONetListener *listener,
SocketAddress *addr,
+ int num,
Error **errp);
/**
diff --git a/io/net-listener.c b/io/net-listener.c
index dc81150318..5d8a226872 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -62,6 +62,7 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
int qio_net_listener_open_sync(QIONetListener *listener,
SocketAddress *addr,
+ int num,
Error **errp)
{
QIODNSResolver *resolver = qio_dns_resolver_get_instance();
@@ -82,7 +83,7 @@ int qio_net_listener_open_sync(QIONetListener *listener,
for (i = 0; i < nresaddrs; i++) {
QIOChannelSocket *sioc = qio_channel_socket_new();
-if (qio_channel_socket_listen_sync(sioc, resaddrs[i], 1,
+if (qio_channel_socket_listen_sync(sioc, resaddrs[i], num,
err ? NULL : &err) == 0) {
success = true;
diff --git a/migration/socket.c b/migration/socket.c
index 98efdc0286..e63f5e1612 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -181,7 +181,7 @@ static void socket_start_incoming_migration(SocketAddress
*saddr,
qio_net_listener_set_name(listener, "migration-socket-listener");
-if (qio_net_listener_open_sync(listener, saddr, errp) < 0) {
+if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
object_unref(OBJECT(listener));
return;
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 049645491d..83b6c32d73 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1054,7 +1054,7 @@ int main(int argc, char **argv)
server = qio_net_listener_new();
if (socket_activation == 0) {
saddr = nbd_build_socket_address(sockpath, bindto, port);
-if (qio_net_listener_open_sync(server, saddr, &local_err) < 0) {
+if (qio_net_listener_open_sync(server, saddr, 1, &local_err) < 0) {
object_unref(OBJECT(server));
error_report_err(local_err);
exit(EXIT_FAILURE);
diff --git a/ui/vnc.c b/ui/vnc.c
index 4812ed29d0..258461f814 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3762,7 +3762,7 @@ static int vnc_display_listen(VncDisplay *vd,
qio_net_listener_set_name(vd->listener, "vnc-listen");
for (i = 0; i < nsaddr; i++) {
if (qio_net_listener_open_sync(vd->listener,
- saddr[i],
+ saddr[i], 1,
errp) < 0) {
return -1;
}
@@ -3777,7 +3777,7 @@ static int vnc_display_listen(VncDisplay *vd,
qio_net_listener_set_name(vd->wslistener, "vnc-ws-listen");
for (i = 0; i < nwsaddr; i++) {
if (qio_net_listener_open_sync(vd->wslistener,
- wsaddr[i],
+
[Qemu-block] [PATCH v3 2/5] socket: Add num connections to qio_channel_socket_sync()
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Juan Quintela
---
include/io/channel-socket.h| 2 ++
io/channel-socket.c| 7 ---
io/net-listener.c | 2 +-
io/trace-events| 2 +-
scsi/qemu-pr-helper.c | 3 ++-
tests/test-char.c | 4 ++--
tests/test-io-channel-socket.c | 2 +-
tests/tpm-emu.c| 2 +-
8 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index d7134d2cd6..ed88e5b8c1 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -123,6 +123,7 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
* qio_channel_socket_listen_sync:
* @ioc: the socket channel object
* @addr: the address to listen to
+ * @num: the expected ammount of connections
* @errp: pointer to a NULL-initialized error object
*
* Attempt to listen to the address @addr. This method
@@ -132,6 +133,7 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
*/
int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
SocketAddress *addr,
+ int num,
Error **errp);
/**
diff --git a/io/channel-socket.c b/io/channel-socket.c
index a533c8bc11..6258c25983 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -197,12 +197,13 @@ void qio_channel_socket_connect_async(QIOChannelSocket
*ioc,
int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
SocketAddress *addr,
+ int num,
Error **errp)
{
int fd;
-trace_qio_channel_socket_listen_sync(ioc, addr);
-fd = socket_listen(addr, 1, errp);
+trace_qio_channel_socket_listen_sync(ioc, addr, num);
+fd = socket_listen(addr, num, errp);
if (fd < 0) {
trace_qio_channel_socket_listen_fail(ioc);
return -1;
@@ -226,7 +227,7 @@ static void qio_channel_socket_listen_worker(QIOTask *task,
SocketAddress *addr = opaque;
Error *err = NULL;
-qio_channel_socket_listen_sync(ioc, addr, &err);
+qio_channel_socket_listen_sync(ioc, addr, 1, &err);
qio_task_set_error(task, err);
}
diff --git a/io/net-listener.c b/io/net-listener.c
index d8cfe52673..dc81150318 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -82,7 +82,7 @@ int qio_net_listener_open_sync(QIONetListener *listener,
for (i = 0; i < nresaddrs; i++) {
QIOChannelSocket *sioc = qio_channel_socket_new();
-if (qio_channel_socket_listen_sync(sioc, resaddrs[i],
+if (qio_channel_socket_listen_sync(sioc, resaddrs[i], 1,
err ? NULL : &err) == 0) {
success = true;
diff --git a/io/trace-events b/io/trace-events
index 378390521e..2e6aa1d749 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -17,7 +17,7 @@ qio_channel_socket_connect_sync(void *ioc, void *addr)
"Socket connect sync ioc=
qio_channel_socket_connect_async(void *ioc, void *addr) "Socket connect async
ioc=%p addr=%p"
qio_channel_socket_connect_fail(void *ioc) "Socket connect fail ioc=%p"
qio_channel_socket_connect_complete(void *ioc, int fd) "Socket connect
complete ioc=%p fd=%d"
-qio_channel_socket_listen_sync(void *ioc, void *addr) "Socket listen sync
ioc=%p addr=%p"
+qio_channel_socket_listen_sync(void *ioc, void *addr, int num) "Socket listen
sync ioc=%p addr=%p num=%d"
qio_channel_socket_listen_async(void *ioc, void *addr) "Socket listen async
ioc=%p addr=%p"
qio_channel_socket_listen_fail(void *ioc) "Socket listen fail ioc=%p"
qio_channel_socket_listen_complete(void *ioc, int fd) "Socket listen complete
ioc=%p fd=%d"
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index a256ce490b..a8a74d1dba 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -1005,7 +1005,8 @@ int main(int argc, char **argv)
.u.q_unix.path = socket_path,
};
server_ioc = qio_channel_socket_new();
-if (qio_channel_socket_listen_sync(server_ioc, &saddr, &local_err) <
0) {
+if (qio_channel_socket_listen_sync(server_ioc, &saddr,
+ 1, &local_err) < 0) {
object_unref(OBJECT(server_ioc));
error_report_err(local_err);
return 1;
diff --git a/tests/test-char.c b/tests/test-char.c
index f9440cdcfd..af131fc850 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -666,7 +666,7 @@ char_socket_addr_to_opt_str(SocketAddress *addr, bool
fd_pass,
char *optstr;
g_assert(!reconnect);
if (is_listen) {
-qio_channel_socket_listen_sync(ioc, addr, &error_abort);
+qio_channel_socket_listen_sync(ioc, addr, 1, &error_abort);
} else {
qio_channel_socket_connect_sync(ioc, addr, &error_abort);
}
@@ -891,7 +891,7 @@ stat
Re: [Qemu-block] [PATCH v3 1/5] socket: Add backlog parameter to socket_listen
On Tue, Aug 20, 2019 at 12:48:32PM +0200, Juan Quintela wrote: > Current parameter was always one. We continue with that value for now > in all callers. > > Signed-off-by: Juan Quintela > > --- > Moved trace to socket_listen > --- > include/qemu/sockets.h| 2 +- > io/channel-socket.c | 2 +- > qga/channel-posix.c | 2 +- > tests/test-util-sockets.c | 12 ++-- > util/qemu-sockets.c | 33 ++--- > util/trace-events | 3 +++ > 6 files changed, 34 insertions(+), 20 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-block] [PATCH v3 3/5] socket: Add num connections to qio_channel_socket_async()
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Juan Quintela
---
include/io/channel-socket.h| 2 ++
io/channel-socket.c| 30 +++---
io/trace-events| 2 +-
tests/test-io-channel-socket.c | 2 +-
4 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index ed88e5b8c1..777ff5954e 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -140,6 +140,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
* qio_channel_socket_listen_async:
* @ioc: the socket channel object
* @addr: the address to listen to
+ * @num: the expected ammount of connections
* @callback: the function to invoke on completion
* @opaque: user data to pass to @callback
* @destroy: the function to free @opaque
@@ -155,6 +156,7 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
*/
void qio_channel_socket_listen_async(QIOChannelSocket *ioc,
SocketAddress *addr,
+ int num,
QIOTaskFunc callback,
gpointer opaque,
GDestroyNotify destroy,
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 6258c25983..b74f5b92a0 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -220,14 +220,27 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
}
+struct QIOChannelListenWorkerData {
+SocketAddress *addr;
+int num; /* amount of expected connections */
+};
+
+static void qio_channel_listen_worker_free(gpointer opaque)
+{
+struct QIOChannelListenWorkerData *data = opaque;
+
+qapi_free_SocketAddress(data->addr);
+g_free(data);
+}
+
static void qio_channel_socket_listen_worker(QIOTask *task,
gpointer opaque)
{
QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
-SocketAddress *addr = opaque;
+struct QIOChannelListenWorkerData *data = opaque;
Error *err = NULL;
-qio_channel_socket_listen_sync(ioc, addr, 1, &err);
+qio_channel_socket_listen_sync(ioc, data->addr, data->num, &err);
qio_task_set_error(task, err);
}
@@ -235,6 +248,7 @@ static void qio_channel_socket_listen_worker(QIOTask *task,
void qio_channel_socket_listen_async(QIOChannelSocket *ioc,
SocketAddress *addr,
+ int num,
QIOTaskFunc callback,
gpointer opaque,
GDestroyNotify destroy,
@@ -242,16 +256,18 @@ void qio_channel_socket_listen_async(QIOChannelSocket
*ioc,
{
QIOTask *task = qio_task_new(
OBJECT(ioc), callback, opaque, destroy);
-SocketAddress *addrCopy;
+struct QIOChannelListenWorkerData *data;
-addrCopy = QAPI_CLONE(SocketAddress, addr);
+data = g_new0(struct QIOChannelListenWorkerData, 1);
+data->addr = QAPI_CLONE(SocketAddress, addr);
+data->num = num;
/* socket_listen() blocks in DNS lookups, so we must use a thread */
-trace_qio_channel_socket_listen_async(ioc, addr);
+trace_qio_channel_socket_listen_async(ioc, addr, num);
qio_task_run_in_thread(task,
qio_channel_socket_listen_worker,
- addrCopy,
- (GDestroyNotify)qapi_free_SocketAddress,
+ data,
+ qio_channel_listen_worker_free,
context);
}
diff --git a/io/trace-events b/io/trace-events
index 2e6aa1d749..d7bc70b966 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -18,7 +18,7 @@ qio_channel_socket_connect_async(void *ioc, void *addr)
"Socket connect async io
qio_channel_socket_connect_fail(void *ioc) "Socket connect fail ioc=%p"
qio_channel_socket_connect_complete(void *ioc, int fd) "Socket connect
complete ioc=%p fd=%d"
qio_channel_socket_listen_sync(void *ioc, void *addr, int num) "Socket listen
sync ioc=%p addr=%p num=%d"
-qio_channel_socket_listen_async(void *ioc, void *addr) "Socket listen async
ioc=%p addr=%p"
+qio_channel_socket_listen_async(void *ioc, void *addr, int num) "Socket listen
async ioc=%p addr=%p num=%d"
qio_channel_socket_listen_fail(void *ioc) "Socket listen fail ioc=%p"
qio_channel_socket_listen_complete(void *ioc, int fd) "Socket listen complete
ioc=%p fd=%d"
qio_channel_socket_dgram_sync(void *ioc, void *localAddr, void *remoteAddr)
"Socket dgram sync ioc=%p localAddr=%p remoteAddr=%p"
diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index 6eebcee115..50235c1547 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -113,7 +113,7 @@ static void test_io_channel_setup_async(SocketAddress
*listen_addr,
lioc = qio_channel_
[Qemu-block] [PATCH v3 5/5] multifd: Use number of channels as listen backlog
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Juan Quintela
---
migration/socket.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/migration/socket.c b/migration/socket.c
index e63f5e1612..97c9efde59 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -178,10 +178,15 @@ static void socket_start_incoming_migration(SocketAddress
*saddr,
{
QIONetListener *listener = qio_net_listener_new();
size_t i;
+int num = 1;
qio_net_listener_set_name(listener, "migration-socket-listener");
-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
+if (migrate_use_multifd()) {
+num = migrate_multifd_channels();
+}
+
+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
object_unref(OBJECT(listener));
return;
}
--
2.21.0
Re: [Qemu-block] [PATCH v3 1/5] socket: Add backlog parameter to socket_listen
Daniel P. Berrangé wrote: > On Tue, Aug 20, 2019 at 12:48:32PM +0200, Juan Quintela wrote: >> Current parameter was always one. We continue with that value for now >> in all callers. >> >> Signed-off-by: Juan Quintela >> >> --- >> Moved trace to socket_listen >> --- >> include/qemu/sockets.h| 2 +- >> io/channel-socket.c | 2 +- >> qga/channel-posix.c | 2 +- >> tests/test-util-sockets.c | 12 ++-- >> util/qemu-sockets.c | 33 ++--- >> util/trace-events | 3 +++ >> 6 files changed, 34 insertions(+), 20 deletions(-) > > Reviewed-by: Daniel P. Berrangé Hi Everything is reviewed by you, and it is mostly socket code. Should I do the pull request, or are you doing it? Thanks, Juan.
Re: [Qemu-block] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data
On 19.08.19 21:23, Eric Blake wrote: > On 8/19/19 1:55 PM, Max Reitz wrote: >> The qcow2 specification says to ignore unknown extra data fields in >> snapshot table entries. Currently, we discard it whenever we update the >> image, which is a bit different from "ignore". >> >> This patch makes the qcow2 driver keep all unknown extra data fields >> when updating an image's snapshot table. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2.h | 5 >> block/qcow2-snapshot.c | 61 +++--- >> 2 files changed, 56 insertions(+), 10 deletions(-) >> > >> @@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> sn = s->snapshots + i; >> offset = ROUND_UP(offset, 8); >> offset += sizeof(h); >> -offset += sizeof(extra); >> +offset += MAX(sizeof(extra), sn->extra_data_size); > > Why would we ever have less than sizeof(extra) bytes to write on output, > since we always produce the fields on creation and synthesize the > missing fields of extra on read? Can't you rewrite this as: > > assert(sn->extra_data_size >= sizeof(extra)); > offset += sn->extra_data_size; Hm, but I don’t prop up extra_data_size to be at least sizeof(extra). I can do that, but it would add a few extra lines here and there. > Otherwise, > > Reviewed-by: Eric Blake In any case, thanks for reviewing again :-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data
On 19.08.19 21:34, Eric Blake wrote:
> On 8/19/19 1:55 PM, Max Reitz wrote:
>> The qcow2 specification says to ignore unknown extra data fields in
>> snapshot table entries. Currently, we discard it whenever we update the
>> image, which is a bit different from "ignore".
>>
>> This patch makes the qcow2 driver keep all unknown extra data fields
>> when updating an image's snapshot table.
>>
>
>> @@ -80,31 +80,53 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error
>> **errp)
>> sn->date_sec = be32_to_cpu(h.date_sec);
>> sn->date_nsec = be32_to_cpu(h.date_nsec);
>> sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec);
>> -extra_data_size = be32_to_cpu(h.extra_data_size);
>> +sn->extra_data_size = be32_to_cpu(h.extra_data_size);
>>
>> id_str_size = be16_to_cpu(h.id_str_size);
>> name_size = be16_to_cpu(h.name_size);
>>
>> -/* Read extra data */
>> +if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
>> +ret = -EFBIG;
>> +error_setg(errp, "Too much extra metadata in snapshot table "
>> + "entry %i", i);
>> +goto fail;
>
> We fail if extra_data_size is > 1024...
>
>
>> +if (sn->extra_data_size > sizeof(extra)) {
>> +/* Store unknown extra data */
>> +size_t unknown_extra_data_size =
>> +sn->extra_data_size - sizeof(extra);
>> +
>
> But read at most 1008 bytes into sn->unknown_extra_data.
>
>> @@ -234,6 +257,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>> }
>> offset += sizeof(extra);
>>
>> +if (sn->extra_data_size > sizeof(extra)) {
>> +size_t unknown_extra_data_size =
>> +sn->extra_data_size - sizeof(extra);
>> +
>> +/* qcow2_read_snapshots() ensures no unbounded allocation */
>> +assert(unknown_extra_data_size <= BDRV_REQUEST_MAX_BYTES);
>
> So this assertion is quite loose in what it permits; tighter would be
>
> assert(unknown_extra_data_size <= QCOW_MAX_SNAPSHOT_EXTRA_DATA -
> sizeof(extra))
As I said in the last version, I have this assertion here just because
of the following bdrv_pwrite(); so all we need to assert is that it fits
BDRV_REQUEST_MAX_BYTES (which it clearly does, as you say).
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries
On 19.08.19 21:37, Eric Blake wrote:
> On 8/19/19 1:55 PM, Max Reitz wrote:
>> The only case where we currently reject snapshot table entries is when
>> they have too much extra data. Fix them with qemu-img check -r all by
>> counting it as a corruption, reducing their extra_data_size, and then
>> letting qcow2_check_fix_snapshot_table() do the rest.
>>
>> Signed-off-by: Max Reitz
>> ---
>> block/qcow2-snapshot.c | 67 +++---
>> 1 file changed, 56 insertions(+), 11 deletions(-)
>>
>
>> @@ -64,6 +80,8 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error
>> **errp)
>> s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
>>
>> for(i = 0; i < s->nb_snapshots; i++) {
>> +bool truncate_unknown_extra_data = false;
>
> Worth adding space after 'for' while in the vicinity?
Hm, it doesn’t bother me enough, at least. It’d probably be better to
just fix all occurrences of that in block/qcow2* in one patch (and there
are a few indeed). That is, if someone is sufficiently bothered by it. ;-)
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling
On 19.08.19 22:25, Eric Blake wrote: > On 8/19/19 1:56 PM, Max Reitz wrote: >> Add a test how our qcow2 driver handles extra data in snapshot table >> entries, and how it repairs overly long snapshot tables. >> >> Signed-off-by: Max Reitz >> --- > >> +++ b/tests/qemu-iotests/261.out >> @@ -0,0 +1,346 @@ >> +QA output created by 261 >> + >> +=== Create v2 template === >> + >> +Formatting 'TEST_DIR/t.IMGFMT.v2.orig', fmt=IMGFMT size=67108864 >> +No errors were found on the image. >> +Snapshots in TEST_DIR/t.IMGFMT.v2.orig: >> + [0] >> +ID: 1 >> +Name: sn0 >> +Extra data size: 0 >> + [1] >> +ID: 2 >> +Name: sn1 >> +Extra data size: 42 >> +VM state size: 0 >> +Disk size: 67108864 >> +Unknown extra data: very important data > > Hmm - possibly one more patch to write - but when checking snapshots for > accuracy, do we want to insist that the 32-bit truncated VM state size > is either 0 or matches the low 32-bits of the 64-bit VM state size > field? Any mismatch between those fields (other than the 32-bit field > being left 0 because we knew to use the 64-bit field) might be a hint of > a possible corruption. But there's no good way to correct it other than > wiping the 32-bit field to 0; and for a v2 image, any change we make to > the 32-bit field might actually make the snapshot unusable for an older > client that doesn't know how to use the 64-bit field. So maybe we just > overlook that. The spec clearly says that when the 64-bit field is present, the 32-bit field is to be ignored. So there’s at least no standard-compliant way of checking it. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables
On 19.08.19 21:43, Eric Blake wrote: > On 8/19/19 1:55 PM, Max Reitz wrote: >> We currently refuse to open qcow2 images with overly long snapshot >> tables. This patch makes qemu-img check -r all drop all offending >> entries past what we deem acceptable. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-snapshot.c | 88 +- >> 1 file changed, 78 insertions(+), 10 deletions(-) > > I know I was reluctant in v1, but you also managed to convince me that > it really takes a LOT of effort to get a table with that many entries. > And a user has to opt-in to 'qemu-img -r' (it may discard a snapshot > they value, but that beats not being able to use the image under qemu at > all, and we don't erase it for plain 'qemu-img check'). So I'm okay > with this going in. Maybe the commit message can state this sort of > reasoning. So maybe: The user cannot choose which snapshots are removed. This is fine because we have chosen the maximum snapshot table size to be so large (64 MB) that it cannot be reasonably reached. If the snapshot table exceeds this size, the image has probably been corrupted in some way; in this case, it is most important to just make the image usable such that the user can copy off at least the active layer. (Also note that the snapshots will be removed only with "-r all", so a plain "check" or "check -r leaks" will not delete any data.) ? Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries
On 19.08.19 21:45, Eric Blake wrote: > On 8/19/19 1:55 PM, Max Reitz wrote: >> Signed-off-by: Max Reitz >> --- > > Short on the reasoning why this isn't a problem in practice. (Again, > because we only do it via opt-in qemu-img -r; you can already learn if > qemu-img will have problem with your file created externally without > destroying the image, and elect to not have qemu-img clean it if you > don't like the algorithm qemu-img will use). OK. I’ll add the same message as for patch 12, just with s/64 MB/65536 snapshots/, if that seems good to you. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3 6/8] iotests: Test driver whitelisting in 093
On 20.08.19 08:40, Thomas Huth wrote:
> On 8/19/19 10:18 PM, Max Reitz wrote:
>> null-aio may not be whitelisted. Skip all test cases that require it.
>>
>> Signed-off-by: Max Reitz
>> ---
>> tests/qemu-iotests/093 | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>> index 50c1e7f2ec..f03fa24a07 100755
>> --- a/tests/qemu-iotests/093
>> +++ b/tests/qemu-iotests/093
>> @@ -24,7 +24,7 @@ import iotests
>> nsec_per_sec = 10
>>
>> class ThrottleTestCase(iotests.QMPTestCase):
>> -test_img = "null-aio://"
>> +test_driver = "null-aio"
>> max_drives = 3
>>
>> def blockstats(self, device):
>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>> return stat['rd_bytes'], stat['rd_operations'],
>> stat['wr_bytes'], stat['wr_operations']
>> raise Exception("Device not found for blockstats: %s" % device)
>>
>> +def required_drivers(self):
>> +return [self.test_driver]
>> +
>> +@iotests.skip_if_unsupported(required_drivers)
>> def setUp(self):
>> self.vm = iotests.VM()
>> for i in range(0, self.max_drives):
>> -self.vm.add_drive(self.test_img, "file.read-zeroes=on")
>> +self.vm.add_drive(self.test_driver + "://",
>> "file.read-zeroes=on")
>> self.vm.launch()
>>
>> def tearDown(self):
>> @@ -264,7 +268,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
>> self.assertEqual(self.blockstats('drive1')[0], 4096)
>>
>> class ThrottleTestCoroutine(ThrottleTestCase):
>> -test_img = "null-co://"
>> +test_driver = "null-co"
>>
>> class ThrottleTestGroupNames(iotests.QMPTestCase):
>> max_drives = 3
>> @@ -425,4 +429,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
>>
>>
>> if __name__ == '__main__':
>> +if 'null-co' not in iotests.supported_formats():
>> +iotests.notrun('null-co driver support missing')
>> iotests.main(supported_fmts=["raw"])
>
> Maybe also mention null-co in the patch description?
I probably didn’t because I felt bad that maybe I should add a null-co
check to all tests that require it... But two wrongs don’t make a
right, so I’ll leave it at one wrong and put “Skip the whole test if
null-co is not whitelisted.” into the commit message, yes.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] iotests: Add more "skip_if_unsupported" statements to the python tests
On 20.08.19 09:14, Thomas Huth wrote: > On 8/19/19 9:13 PM, Max Reitz wrote: >> On 19.08.19 11:21, Thomas Huth wrote: >>> The python code already contains a possibility to skip tests if the >>> corresponding driver is not available in the qemu binary - use it >>> in more spots to avoid that the tests are failing if the driver has >>> been disabled. >>> >>> Signed-off-by: Thomas Huth >>> --- >>> tests/qemu-iotests/030 | 3 +++ >>> tests/qemu-iotests/040 | 2 ++ >>> tests/qemu-iotests/041 | 14 +- >>> tests/qemu-iotests/245 | 2 ++ >>> 4 files changed, 20 insertions(+), 1 deletion(-) >> >> [...] >> >>> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 >>> index 26bf1701eb..f45d20fbe0 100755 >>> --- a/tests/qemu-iotests/041 >>> +++ b/tests/qemu-iotests/041 >>> @@ -817,6 +817,7 @@ class TestRepairQuorum(iotests.QMPTestCase): >>> image_len = 1 * 1024 * 1024 # MB >>> IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ] >>> >>> +@iotests.skip_if_unsupported(['quorum']) >>> def setUp(self): >>> self.vm = iotests.VM() >> >> It’s clear that none of these tests can run if there is no quorum >> support, because setUp() creates a quorum node. I think it would be >> nice if it would suffice to just skip everything automatically if >> setUp() is skipped and not have to bother about each of the test cases. >> >> Coincidentally (:-)), I have a patch to do that, namely “iotests: Allow >> skipping test cases” in my “iotests: Selfish patches” series: >> >> https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg01106.html >> >> Yes, that means you cannot use an annotation because it needs @self to >> be able to skip the test. Hm... But I think I can make that work by >> simply s/case_notrun/args[0].case_skip/ in skip_if_unsupported()? > > Sure, feel free to ignore my patch here or to modify it according to > your reworks. As long as we finally get the iotests into a shape where > they are a little bit more flexible wrt the enabled/disabled drivers, > I'm happy. Oh, no, I don’t mean to ignore the patch. I just think that a skip_if_unsupported() annotation above setUp() should be enough to skip all tests. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PULL 00/17] Block patches
On Mon, 19 Aug 2019 at 17:17, Max Reitz wrote: > > The following changes since commit 3fbd3405d2b0604ea530fc7a1828f19da1e95ff9: > > Merge remote-tracking branch > 'remotes/huth-gitlab/tags/pull-request-2019-08-17' into staging (2019-08-19 > 14:14:09 +0100) > > are available in the Git repository at: > > https://github.com/XanClic/qemu.git tags/pull-block-2019-08-19 > > for you to fetch changes up to fa27c478102a6b5d1c6b02c005607ad9404b915f: > > doc: Preallocation does not require writing zeroes (2019-08-19 17:13:26 > +0200) > > > Block patches: > - preallocation=falloc/full support for LUKS > - Various minor fixes > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2 for any user-visible changes. -- PMM
Re: [Qemu-block] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables
On 8/20/19 7:09 AM, Max Reitz wrote: > On 19.08.19 21:43, Eric Blake wrote: >> On 8/19/19 1:55 PM, Max Reitz wrote: >>> We currently refuse to open qcow2 images with overly long snapshot >>> tables. This patch makes qemu-img check -r all drop all offending >>> entries past what we deem acceptable. >>> >>> Signed-off-by: Max Reitz >>> --- >>> block/qcow2-snapshot.c | 88 +- >>> 1 file changed, 78 insertions(+), 10 deletions(-) >> >> I know I was reluctant in v1, but you also managed to convince me that >> it really takes a LOT of effort to get a table with that many entries. >> And a user has to opt-in to 'qemu-img -r' (it may discard a snapshot >> they value, but that beats not being able to use the image under qemu at >> all, and we don't erase it for plain 'qemu-img check'). So I'm okay >> with this going in. Maybe the commit message can state this sort of >> reasoning. > > So maybe: > > The user cannot choose which snapshots are removed. This is fine > because we have chosen the maximum snapshot table size to be so large > (64 MB) that it cannot be reasonably reached. If the snapshot table > exceeds this size, the image has probably been corrupted in some way; in > this case, it is most important to just make the image usable such that > the user can copy off at least the active layer. > (Also note that the snapshots will be removed only with "-r all", so a > plain "check" or "check -r leaks" will not delete any data.) I like it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v9] qemu-io: add pattern file for write command
On 19.08.19 18:18, Denis Plotnikov wrote:
> The patch allows to provide a pattern file for write
> command. There was no similar ability before.
>
> Signed-off-by: Denis Plotnikov
> ---
> v9:
> * replace flag cast to int with bool [Eric]
> * fix the error message [Eric]
> * use qemu_io_free instead of qemu_vfree [Eric]
> * add function description [Eric]
>
> v8: fix according to Max's comments
> * get rid of unnecessary buffer for the pattern
> * buffer allocation just in bytes
> * take into account the missalign offset
> * don't copy file name
> * changed char* to const char* in input params
>
> v7:
> * fix variable naming
> * make code more readable
> * extend help for write command
>
> v6:
> * the pattern file is read once to reduce io
>
> v5:
> * file name initiated with null to make compilers happy
>
> v4:
> * missing signed-off clause added
>
> v3:
> * missing file closing added
> * exclusive flags processing changed
> * buffer void* converted to char* to fix pointer arithmetics
> * file reading error processing added
> ---
> qemu-io-cmds.c | 97 ++
> 1 file changed, 91 insertions(+), 6 deletions(-)
>
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 09750a23ce..f7bdfe673b 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -351,6 +351,77 @@ static void qemu_io_free(void *p)
> qemu_vfree(p);
> }
>
> +/*
> + * qemu_io_alloc_from_file()
> + *
> + * Allocates the buffer and populates it with the content of the given file
> + * up to @len bytes. If the file length is less then @len, then the buffer
s/then/than/ (the first one)
> + * is populated with then file content cyclically.
s/then/the/
> + *
> + * @blk - the block backend where the buffer content is going to be written
> to
> + * @len - the buffer length
> + * @file_name - the file to copy the content from
Probably better s/copy/read/
> + *
> + * Returns: the buffer pointer on success
> + * NULL on error
> + */
> +static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> + const char *file_name)
> +{
> +char *buf, *buf_origin;
> +FILE *f = fopen(file_name, "r");
> +int pattern_len;
> +
> +if (!f) {
> +perror(file_name);
> +return NULL;
> +}
> +
> +if (qemuio_misalign) {
> +len += MISALIGN_OFFSET;
> +}
> +
> +buf_origin = buf = blk_blockalign(blk, len);
> +
> +if (qemuio_misalign) {
> +buf_origin += MISALIGN_OFFSET;
> +}
> +
> +pattern_len = fread(buf_origin, 1, len, f);
Pulling the misalignment up here has more effects than just requiring
qemu_io_free() rather than qemu_vfree().
First, it breaks this fread(), because @len is the length of the buffer
in total, so this here is a buffer overflow.
> +
> +if (ferror(f)) {
> +perror(file_name);
> +goto error;
> +}
> +
> +if (pattern_len == 0) {
> +fprintf(stderr, "%s: file is empty\n", file_name);
> +goto error;
> +}
> +
> +fclose(f);
> +
> +if (len > pattern_len) {
> +len -= pattern_len;
> +buf += pattern_len;
> +
> +while (len > 0) {
> +size_t len_to_copy = MIN(pattern_len, len);
> +
> +memcpy(buf, buf_origin, len_to_copy);
Second, it breaks this memcpy(), because now [buf, buf + len_to_copy)
and [buf_origin, buf_origin + len_to_copy) may overlap.
I think the solution would be (1) to add MISALIGN_OFFSET not only to
@buf_origin, but to @buf also, and (2) to reduce len by MISALIGN_OFFSET.
So all in all, I think both issues should be fixed if you add
“buf += MISALIGN_OFFSET” and “len -= MISALIGN_OFFSET” to the second
“if (qemuio_misalign)” block. I think.
> +
> +len -= len_to_copy;
> +buf += len_to_copy;
> +}
> +}
> +
> +return buf_origin;
> +
> +error:
> +qemu_io_free(buf_origin);
> +return NULL;
> +}
> +
> static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
> {
> uint64_t i;
[...]
> @@ -1051,8 +1128,9 @@ static int write_f(BlockBackend *blk, int argc, char
> **argv)
> return -EINVAL;
> }
>
> -if (zflag && Pflag) {
> -printf("-z and -P cannot be specified at the same time\n");
> +if ((bool)zflag + (bool)Pflag + (bool)sflag > 1) {
Eric’s point was that you don’t need to cast at all.
Max
> +printf("Only one of -z, -P, and -s "
> + "can be specified at the same time\n");
> return -EINVAL;
> }
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] iotests: Check for enabled drivers before testing them
On 19.08.19 09:53, Thomas Huth wrote:
> It is possible to enable only a subset of the block drivers with the
> "--block-drv-rw-whitelist" option of the "configure" script. All other
> drivers are marked as unusable (or only included as read-only with the
> "--block-drv-ro-whitelist" option). If an iotest is now using such a
> disabled block driver, it is failing - which is bad, since at least the
> tests in the "auto" group should be able to deal with this situation.
> Thus let's introduce a "_require_drivers" function that can be used by
> the shell tests to check for the availability of certain drivers first,
> and marks the test as "not run" if one of the drivers is missing.
Well, the reasoning for generally not making blkdebug/blkverify explicit
requirements was that you should just have both enabled when running
iotests.
Of course, that no longer works as an argument now that we
unconditionally run some iotests in make check.
But still, the question is how strict you want to be. If blkdebug
cannot be assumed to be present, what about null-co? What about raw?
> Signed-off-by: Thomas Huth
> ---
> tests/qemu-iotests/071 | 1 +
> tests/qemu-iotests/081 | 1 +
> tests/qemu-iotests/099 | 1 +
> tests/qemu-iotests/184 | 1 +
> tests/qemu-iotests/common.rc | 13 +
> 5 files changed, 17 insertions(+)
>
> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
> index 1cca9233d0..fab52b 100755
> --- a/tests/qemu-iotests/071
> +++ b/tests/qemu-iotests/071
> @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>
> _supported_fmt qcow2
> _supported_proto file
> +_require_drivers blkdebug blkverify
Because this test also requires the raw driver.
>
> do_run_qemu()
> {
> diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
> index c418bab093..1695781bc0 100755
> --- a/tests/qemu-iotests/081
> +++ b/tests/qemu-iotests/081
> @@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> _supported_fmt raw
> _supported_proto file
> _supported_os Linux
> +_require_drivers quorum
This test has already a check whether quorum is supported, that should
be removed now.
(Also, this test requires the raw driver.)
> do_run_qemu()
> {
[...]
> diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
> index cb0c181228..33dd8d2a4f 100755
> --- a/tests/qemu-iotests/184
> +++ b/tests/qemu-iotests/184
> @@ -33,6 +33,7 @@ trap "exit \$status" 0 1 2 3 15
> . ./common.filter
>
> _supported_os Linux
> +_require_drivers throttle
This test also requires null-co.
> do_run_qemu()
> {
I found two more check-block tests that may or may not require use of
_require_drivers (depending on which drivers we deem absolutely essential):
- 120: Needs raw
- 186: Needs null-co
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 5502c3da2f..7d4e68846f 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -520,5 +520,18 @@ _require_command()
> [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
> }
>
> +# Check that a set of drivers has been whitelisted in the QEMU binary
> +#
> +_require_drivers()
> +{
> +available=$($QEMU -drive format=help | grep 'Supported formats:')
Seems a bit shortcut-y to not remove the “Supported formats:” prefix,
but I don’t suppose we’ll ever have block drivers with either name...
> +for driver
> +do
> +if ! echo "$available" | grep -q "$driver"; then
162 greps like this:
> grep '^Supported formats:.* ssh\( \|$\)'
Maybe the same should be done here, i.e. grep -q " $driver\( \|\$\)"? I
can well imagine that something like “ssh” might appear as a substring
in some other driver.
(Speaking of which, why not change 162 to using this new function? Yes,
it isn’t in auto, but still...)
Max
> +_notrun "$driver not available"
> +fi
> +done
> +}
> +
> # make sure this script returns success
> true
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3 3/7] block/io: handle alignment and max_transfer for copy_range
12.08.2019 17:48, Max Reitz wrote: > On 10.08.19 21:31, Vladimir Sementsov-Ogievskiy wrote: >> copy_range ignores these limitations, let's improve it. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/io.c | 44 >> 1 file changed, 36 insertions(+), 8 deletions(-) > > Reviewed-by: Max Reitz > Hmm. Now I think that next patch is arguable, and I still don't see true way of organizing limitation of request length and memory allocation in conjunction with async requests in backup. So, I'll send next version of "improvements" without this (there are already enough simpler patches). And this patch becomes something separate. Do you think we need it anyway? If yes, please queue it in separate. It may be better to return ENOTSUP on too big requests too, to keep simpler code and make callers optimize their copying loops by themselves. -- Best regards, Vladimir
Re: [Qemu-block] [PATCH] iotests: Check for enabled drivers before testing them
On 8/20/19 5:01 PM, Max Reitz wrote:
> On 19.08.19 09:53, Thomas Huth wrote:
>> It is possible to enable only a subset of the block drivers with the
>> "--block-drv-rw-whitelist" option of the "configure" script. All other
>> drivers are marked as unusable (or only included as read-only with the
>> "--block-drv-ro-whitelist" option). If an iotest is now using such a
>> disabled block driver, it is failing - which is bad, since at least the
>> tests in the "auto" group should be able to deal with this situation.
>> Thus let's introduce a "_require_drivers" function that can be used by
>> the shell tests to check for the availability of certain drivers first,
>> and marks the test as "not run" if one of the drivers is missing.
>
> Well, the reasoning for generally not making blkdebug/blkverify explicit
> requirements was that you should just have both enabled when running
> iotests.
Well, we disable blkverify in our downstream RHEL version of QEMU - so
it would be great if the iotests could at least adapt to that missing
driver.
> Of course, that no longer works as an argument now that we
> unconditionally run some iotests in make check.
>
> But still, the question is how strict you want to be. If blkdebug
> cannot be assumed to be present, what about null-co? What about raw?
I tried to disable everything beside qcow2 - but that causes so many
things to fail that it hardly makes sense to try to get that working. I
think we can assume that at least null-co, qcow2 and raw are enabled.
(If anybody still wants to try to run "make check" with one of these
drivers disabled, I think we should rather add a superior check to
tests/check-block.sh or tests/qemu-iotests/check instead and skip the
iotests completely in that case).
>> Signed-off-by: Thomas Huth
>> ---
>> tests/qemu-iotests/071 | 1 +
>> tests/qemu-iotests/081 | 1 +
>> tests/qemu-iotests/099 | 1 +
>> tests/qemu-iotests/184 | 1 +
>> tests/qemu-iotests/common.rc | 13 +
>> 5 files changed, 17 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
>> index 1cca9233d0..fab52b 100755
>> --- a/tests/qemu-iotests/071
>> +++ b/tests/qemu-iotests/071
>> @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>
>> _supported_fmt qcow2
>> _supported_proto file
>> +_require_drivers blkdebug blkverify
>
> Because this test also requires the raw driver.
The test also works for me when I configured QEMU with:
--block-drv-rw-whitelist="qcow2 file null-co blkdebug blkverify"
i.e. the raw driver should be disabled in that case?
>>
>> do_run_qemu()
>> {
>> diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
>> index c418bab093..1695781bc0 100755
>> --- a/tests/qemu-iotests/081
>> +++ b/tests/qemu-iotests/081
>> @@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>> _supported_fmt raw
>> _supported_proto file
>> _supported_os Linux
>> +_require_drivers quorum
>
> This test has already a check whether quorum is supported, that should
> be removed now.
Hmm, true ... apparently that test was not working for my case ... could
it be that qemu-img ignores the whitelist and simply says that all
drivers are supported?
> (Also, this test requires the raw driver.)
Agreed, this test indeed does not work without 'raw'. But it is already
marked with "_supported_fmt raw", so you can indeed only run it with
"raw". And running a raw-only test with a qemu binary where raw is
disabled could be considered as user error, I guess ;-)
>> diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
>> index cb0c181228..33dd8d2a4f 100755
>> --- a/tests/qemu-iotests/184
>> +++ b/tests/qemu-iotests/184
>> @@ -33,6 +33,7 @@ trap "exit \$status" 0 1 2 3 15
>> . ./common.filter
>>
>> _supported_os Linux
>> +_require_drivers throttle
>
> This test also requires null-co.
>
>> do_run_qemu()
>> {
>
> I found two more check-block tests that may or may not require use of
> _require_drivers (depending on which drivers we deem absolutely essential):
> - 120: Needs raw
> - 186: Needs null-co
I think we really have to assume that null-co is available, otherwise
too many things break... (also some qtests are using null-co).
But I could for sure add a check for raw in 120 if desired.
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 5502c3da2f..7d4e68846f 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -520,5 +520,18 @@ _require_command()
>> [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
>> }
>>
>> +# Check that a set of drivers has been whitelisted in the QEMU binary
>> +#
>> +_require_drivers()
>> +{
>> +available=$($QEMU -drive format=help | grep 'Supported formats:')
> Seems a bit shortcut-y to not remove the “Supported formats:” prefix,
> but I don’t suppose we’ll ever have block drivers with either name...
I can remove it, just to be sure.
>> +for driver
>> +do
Re: [Qemu-block] [PATCH v9] qemu-io: add pattern file for write command
On Aug 20 2019, at 4:35 pm, Max Reitz wrote:
On 19.08.19 18:18, Denis Plotnikov wrote:
The patch allows to provide a pattern file for write
command. There was no similar ability before.
Signed-off-by: Denis Plotnikov
---
v9:
* replace flag cast to int with bool [Eric]
* fix the error message [Eric]
* use qemu_io_free instead of qemu_vfree [Eric]
* add function description [Eric]
v8: fix according to Max's comments
* get rid of unnecessary buffer for the pattern
* buffer allocation just in bytes
* take into account the missalign offset
* don't copy file name
* changed char* to const char* in input params
v7:
* fix variable naming
* make code more readable
* extend help for write command
v6:
* the pattern file is read once to reduce io
v5:
* file name initiated with null to make compilers happy
v4:
* missing signed-off clause added
v3:
* missing file closing added
* exclusive flags processing changed
* buffer void* converted to char* to fix pointer arithmetics
* file reading error processing added
---
qemu-io-cmds.c | 97 ++
1 file changed, 91 insertions(+), 6 deletions(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 09750a23ce..f7bdfe673b 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -351,6 +351,77 @@ static void qemu_io_free(void *p)
qemu_vfree(p);
}
+/*
+ * qemu_io_alloc_from_file()
+ *
+ * Allocates the buffer and populates it with the content of the given file
+ * up to @len bytes. If the file length is less then @len, then the buffer
s/then/than/ (the first one)
+ * is populated with then file content cyclically.
s/then/the/
+ *
+ * @blk - the block backend where the buffer content is going to be written to
+ * @len - the buffer length
+ * @file_name - the file to copy the content from
Probably better s/copy/read/
+ *
+ * Returns: the buffer pointer on success
+ * NULL on error
+ */
+static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
+ const char *file_name)
+{
+ char *buf, *buf_origin;
+ FILE *f = fopen(file_name, "r");
+ int pattern_len;
+
+ if (!f) {
+ perror(file_name);
+ return NULL;
+ }
+
+ if (qemuio_misalign) {
+ len += MISALIGN_OFFSET;
+ }
+
+ buf_origin = buf = blk_blockalign(blk, len);
+
+ if (qemuio_misalign) {
+ buf_origin += MISALIGN_OFFSET;
+ }
+
+ pattern_len = fread(buf_origin, 1, len, f);
Pulling the misalignment up here has more effects than just requiring
qemu_io_free() rather than qemu_vfree().
First, it breaks this fread(), because @len is the length of the buffer
in total, so this here is a buffer overflow.
+
+ if (ferror(f)) {
+ perror(file_name);
+ goto error;
+ }
+
+ if (pattern_len == 0) {
+ fprintf(stderr, "%s: file is empty\n", file_name);
+ goto error;
+ }
+
+ fclose(f);
+
+ if (len > pattern_len) {
+ len -= pattern_len;
+ buf += pattern_len;
+
+ while (len > 0) {
+ size_t len_to_copy = MIN(pattern_len, len);
+
+ memcpy(buf, buf_origin, len_to_copy);
Second, it breaks this memcpy(), because now [buf, buf + len_to_copy)
and [buf_origin, buf_origin + len_to_copy) may overlap.
I think the solution would be (1) to add MISALIGN_OFFSET not only to
@buf_origin, but to @buf also, and (2) to reduce len by MISALIGN_OFFSET.
So all in all, I think both issues should be fixed if you add
“buf += MISALIGN_OFFSET” and “len -= MISALIGN_OFFSET” to the second
“if (qemuio_misalign)” block. I think.
Yes, thanks for pointing out
Denis
+
+ len -= len_to_copy;
+ buf += len_to_copy;
+ }
+ }
+
+ return buf_origin;
+
+error:
+ qemu_io_free(buf_origin);
+ return NULL;
+}
+
static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
{
uint64_t i;
[...]
@@ -1051,8 +1128,9 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
return -EINVAL;
}
- if (zflag && Pflag) {
- printf("-z and -P cannot be specified at the same time\n");
+ if ((bool)zflag + (bool)Pflag + (bool)sflag > 1) {
Eric’s point was that you don’t need to cast at all.
Max
+ printf("Only one of -z, -P, and -s "
+ "can be specified at the same time\n");
return -EINVAL;
}
Re: [Qemu-block] [PATCH 01/13] block-crypto: misc refactoring
On 14.08.19 22:22, Maxim Levitsky wrote:
> * rename the write_func to create_write_func,
> and init_func to create_init_func
> this is preparation for other write_func that will
> be used to update the encryption keys.
>
> No functional changes
>
> Signed-off-by: Maxim Levitsky
> ---
> block/crypto.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
I’m not quite sure why you remove or add blank lines seemingly at random...
> diff --git a/block/crypto.c b/block/crypto.c
> index 8237424ae6..42a3f0898b 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
[...]
> @@ -77,7 +76,7 @@ struct BlockCryptoCreateData {
> };
>
>
> -static ssize_t block_crypto_write_func(QCryptoBlock *block,
> +static ssize_t block_crypto_create_write_func(QCryptoBlock *block,
> size_t offset,
> const uint8_t *buf,
> size_t buflen,
Alignment should be kept at the opening parentheses.
But other than those two things, why not.
Max
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [PATCH v10] qemu-io: add pattern file for write command
The patch allows to provide a pattern file for write
command. There was no similar ability before.
Signed-off-by: Denis Plotnikov
---
v10:
* fix overflow [Max]
* remove casting to bool [Max, Eric]
* fix wording [Max]
v9:
* replace flag cast to int with bool [Eric]
* fix the error message [Eric]
* use qemu_io_free instead of qemu_vfree [Eric]
* add function description [Eric]
v8: fix according to Max's comments
* get rid of unnecessary buffer for the pattern
* buffer allocation just in bytes
* take into account the missalign offset
* don't copy file name
* changed char* to const char* in input params
v7:
* fix variable naming
* make code more readable
* extend help for write command
v6:
* the pattern file is read once to reduce io
v5:
* file name initiated with null to make compilers happy
v4:
* missing signed-off clause added
v3:
* missing file closing added
* exclusive flags processing changed
* buffer void* converted to char* to fix pointer arithmetics
* file reading error processing added
---
qemu-io-cmds.c | 99 +++---
1 file changed, 93 insertions(+), 6 deletions(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 09750a23ce..f411811d95 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -351,6 +351,79 @@ static void qemu_io_free(void *p)
qemu_vfree(p);
}
+/*
+ * qemu_io_alloc_from_file()
+ *
+ * Allocates the buffer and populates it with the content of the given file
+ * up to @len bytes. If the file length is less than @len, then the buffer
+ * is populated with the file content cyclically.
+ *
+ * @blk - the block backend where the buffer content is going to be written to
+ * @len - the buffer length
+ * @file_name - the file to read the content from
+ *
+ * Returns: the buffer pointer on success
+ * NULL on error
+ */
+static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
+ const char *file_name)
+{
+char *buf, *buf_origin;
+FILE *f = fopen(file_name, "r");
+int pattern_len;
+
+if (!f) {
+perror(file_name);
+return NULL;
+}
+
+if (qemuio_misalign) {
+len += MISALIGN_OFFSET;
+}
+
+buf_origin = buf = blk_blockalign(blk, len);
+
+if (qemuio_misalign) {
+buf_origin += MISALIGN_OFFSET;
+buf += MISALIGN_OFFSET;
+len -= MISALIGN_OFFSET;
+}
+
+pattern_len = fread(buf_origin, 1, len, f);
+
+if (ferror(f)) {
+perror(file_name);
+goto error;
+}
+
+if (pattern_len == 0) {
+fprintf(stderr, "%s: file is empty\n", file_name);
+goto error;
+}
+
+fclose(f);
+
+if (len > pattern_len) {
+len -= pattern_len;
+buf += pattern_len;
+
+while (len > 0) {
+size_t len_to_copy = MIN(pattern_len, len);
+
+memcpy(buf, buf_origin, len_to_copy);
+
+len -= len_to_copy;
+buf += len_to_copy;
+}
+}
+
+return buf_origin;
+
+error:
+qemu_io_free(buf_origin);
+return NULL;
+}
+
static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
{
uint64_t i;
@@ -949,6 +1022,7 @@ static void write_help(void)
" -n, -- with -z, don't allow slow fallback\n"
" -p, -- ignored for backwards compatibility\n"
" -P, -- use different pattern to fill file\n"
+" -s, -- use a pattern file to fill the write buffer\n"
" -C, -- report statistics in a machine parsable format\n"
" -q, -- quiet mode, do not show I/O statistics\n"
" -u, -- with -z, allow unmapping\n"
@@ -965,7 +1039,7 @@ static const cmdinfo_t write_cmd = {
.perm = BLK_PERM_WRITE,
.argmin = 2,
.argmax = -1,
-.args = "[-bcCfnquz] [-P pattern] off len",
+.args = "[-bcCfnquz] [-P pattern | -s source_file] off len",
.oneline= "writes a number of bytes at a specified offset",
.help = write_help,
};
@@ -974,7 +1048,7 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
{
struct timeval t1, t2;
bool Cflag = false, qflag = false, bflag = false;
-bool Pflag = false, zflag = false, cflag = false;
+bool Pflag = false, zflag = false, cflag = false, sflag = false;
int flags = 0;
int c, cnt, ret;
char *buf = NULL;
@@ -983,8 +1057,9 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
/* Some compilers get confused and warn if this is not initialized. */
int64_t total = 0;
int pattern = 0xcd;
+const char *file_name = NULL;
-while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
+while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {
switch (c) {
case 'b':
bflag = true;
@@ -1020,6 +1095,10 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
case 'z':
zflag = true;
break;
+case 's':
+sflag = true;
+file_name = optarg
Re: [Qemu-block] [PATCH v10] qemu-io: add pattern file for write command
On 8/20/19 11:46 AM, Denis Plotnikov wrote:
> The patch allows to provide a pattern file for write
> command. There was no similar ability before.
>
> Signed-off-by: Denis Plotnikov
> ---
> @@ -983,8 +1057,9 @@ static int write_f(BlockBackend *blk, int argc, char
> **argv)
> /* Some compilers get confused and warn if this is not initialized. */
> int64_t total = 0;
> int pattern = 0xcd;
> +const char *file_name = NULL;
>
> -while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
> +while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {
This one looks odd (I would have preserved ordering by sticking s:
between q and u). But a maintainer could fix that.
> switch (c) {
> case 'b':
> bflag = true;
> @@ -1020,6 +1095,10 @@ static int write_f(BlockBackend *blk, int argc, char
> **argv)
> case 'z':
> zflag = true;
> break;
> +case 's':
> +sflag = true;
> +file_name = optarg;
> +break;
Likewise, sorting the cases in the same order as the getopt() listing
helps in finding code during later edits.
> @@ -1088,7 +1168,14 @@ static int write_f(BlockBackend *blk, int argc, char
> **argv)
> }
>
> if (!zflag) {
> -buf = qemu_io_alloc(blk, count, pattern);
> +if (sflag) {
> +buf = qemu_io_alloc_from_file(blk, count, file_name);
> +if (!buf) {
> +return -EINVAL;
> +}
> +} else {
> +buf = qemu_io_alloc(blk, count, pattern);
> +}
Pre-existing, but it is odd that qemu_io_alloc() exit()s rather than
returning NULL on huge allocation requests that can't be met. (Then
again, we have an early exit on any length > 2G, and 2G allocations tend
to succeed on modern development machines). Perhaps it would be nice to
teach qemu-io to use blk_try_blockalign for more graceful handling even
on 32-bit platforms, but that's not the problem of your patch.
Option ordering is minor enough that I'm fine giving:
Reviewed-by: Eric Blake
Now, to figure out which maintainer should take it. Perhaps you want to
add a patch 2/1 that adds an iotest using this new mode, to a) ensure it
doesn't regress, and b) makes it reasonable to take in through the
iotest tree.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring
On 14.08.19 22:22, Maxim Levitsky wrote:
> This is also a preparation for key read/write/erase functions
>
> * use master key len from the header
> * prefer to use crypto params in the QCryptoBlockLUKS
> over passing them as function arguments
> * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME
> * Add comments to various crypto parameters in the QCryptoBlockLUKS
>
> Signed-off-by: Maxim Levitsky
> ---
> crypto/block-luks.c | 213 ++--
> 1 file changed, 105 insertions(+), 108 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 409ab50f20..48213abde7 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
[...]
> @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader)
> != 592);
> struct QCryptoBlockLUKS {
> QCryptoBlockLUKSHeader header;
>
> -/* Cache parsed versions of what's in header fields,
> - * as we can't rely on QCryptoBlock.cipher being
> - * non-NULL */
Hm, why remove this comment?
> +/* Main encryption algorithm used for encryption*/
> QCryptoCipherAlgorithm cipher_alg;
> +
> +/* Mode of encryption for the selected encryption algorithm */
> QCryptoCipherMode cipher_mode;
> +
> +/* Initialization vector generation algorithm */
> QCryptoIVGenAlgorithm ivgen_alg;
> +
> +/* Hash algorithm used for IV generation*/
> QCryptoHashAlgorithm ivgen_hash_alg;
> +
> +/*
> + * Encryption algorithm used for IV generation.
> + * Usually the same as main encryption algorithm
> + */
> +QCryptoCipherAlgorithm ivgen_cipher_alg;
> +
> +/* Hash algorithm used in pbkdf2 function */
> QCryptoHashAlgorithm hash_alg;
> };
>
> @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm
> cipher,
> }
> }
>
> +static int masterkeylen(QCryptoBlockLUKS *luks)
This should be a const pointer.
> +{
> +return luks->header.key_bytes;
> +}
> +
> +
> /*
> * Given a key slot, and user password, this will attempt to unlock
> * the master encryption key from the key slot.
> @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm
> cipher,
> */
> static int
> qcrypto_block_luks_load_key(QCryptoBlock *block,
> -QCryptoBlockLUKSKeySlot *slot,
> +uint slot_idx,
Did you use uint on purpose or do you mean a plain “unsigned”?
> const char *password,
> -QCryptoCipherAlgorithm cipheralg,
> -QCryptoCipherMode ciphermode,
> -QCryptoHashAlgorithm hash,
> -QCryptoIVGenAlgorithm ivalg,
> -QCryptoCipherAlgorithm ivcipheralg,
> -QCryptoHashAlgorithm ivhash,
> uint8_t *masterkey,
> -size_t masterkeylen,
> QCryptoBlockReadFunc readfunc,
> void *opaque,
> Error **errp)
> {
> QCryptoBlockLUKS *luks = block->opaque;
> +QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
I think this is a great opportunity to make this a const pointer.
> uint8_t *splitkey;
> size_t splitkeylen;
> uint8_t *possiblekey;
[...]
> @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> goto fail;
> }
>
> +cipher_mode = g_strdup(luks->header.cipher_mode);
> +
This should be freed under the fail label.
(And maybe the fact that this no longer modifies
luks->header.cipher_mode should be mentioned in the commit message, I
don’t know.)
> /*
> * The cipher_mode header contains a string that we have
> * to further parse, of the format
[...]
> @@ -730,13 +718,13 @@ qcrypto_block_luks_open(QCryptoBlock *block,
>
> ivhash_name = strchr(ivgen_name, ':');
> if (!ivhash_name) {
> -ivhash = 0;
> +luks->ivgen_hash_alg = 0;
*luks is initialized to 0 anyway, but it doesn’t hurt, of course.
> } else {
> *ivhash_name = '\0';
> ivhash_name++;
>
> -ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name,
> - &local_err);
> +luks->ivgen_hash_alg =
> qcrypto_block_luks_hash_name_lookup(ivhash_name,
> +
> &local_err);
> if (local_err) {
> ret = -ENOTSUP;
> error_propagate(errp, local_err);
> @@ -744,25 +732,27 @@ qcrypto_block_luks_open(QCryptoBlock *block,
[...]
>
> -hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
> +luks->hash_alg =
> +qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
> &local_err);
Indentation is off now.
> if (local_e
Re: [Qemu-block] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
On 14.08.19 22:22, Maxim Levitsky wrote: [...] > Testing. This was lightly tested with manual testing and with few iotests > that I prepared. > I haven't yet tested fully the write sharing behavior, nor did I run the > whole iotests > suite to see if this code causes some regressions. Since I will need probably > to rewrite some chunks of it to change to 'amend' interface, I decided to > post it now, > to see if you have other ideas/comments to add. I can see that, because half of the qcow2 tests that contain the string “secret” break: Failures: 087 134 158 178 188 198 206 Failed 7 of 13 tests Also, 210 when run with -luks. Some are just due to different test outputs (because you change _filter_img_create to filter some encrypt.* parameters), but some of them are due to aborts. All of them look like different kinds of heap corruptions. I can fully understand not running all iotests (because only the maintainers do that before pull requests), but just running the iotests that immediately concern a series seems prudent to me (unless the series is trivial). (Just “(cd tests/qemu-iotests && grep -l secret ???)” tells you which tests to run that may concern themselves with qcow2 encryption, for example.) So I suppose I’ll stop reviewing the series in detail and just give a more cursory glance from now on. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions
On 14.08.19 22:22, Maxim Levitsky wrote: > With upcoming key management, the header will > need to be stored after the image is created. > > Extracting load header isn't strictly needed, but > do this anyway for the symmetry. > > Also I extracted a function that does basic sanity > checks on the just read header, and a function > which parses all the crypto format to make the > code a bit more readable, plus now the code > doesn't destruct the in-header cipher-mode string, > so that the header now can be stored many times, > which is needed for the key management. > > Also this allows to contain the endianess conversions > in these functions alone > > The header is no longer endian swapped in place, > to prevent (mostly theoretical races I think) > races where someone could see the header in the > process of beeing byteswapped. The formatting looks weird, it doesn’t look quite 72 characters wide... (what commit messages normally use) > Signed-off-by: Maxim Levitsky > --- > crypto/block-luks.c | 756 ++-- > 1 file changed, 440 insertions(+), 316 deletions(-) Also, this commit is just too big. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always
On 14.08.19 22:22, Maxim Levitsky wrote: > While there are other places where these are still stored in memory, > this is still one less key material area that can be sniffed with > various side channel attacks > > > (Many empty lines here) > Signed-off-by: Maxim Levitsky > --- > crypto/block-luks.c | 52 ++--- > 1 file changed, 44 insertions(+), 8 deletions(-) Wouldn’t it make sense to introduce a dedicated function for this? Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] Difference between commit and rebase
On 8/20/19 5:27 AM, lampahome wrote: > I want to remove snapshots and I found two ways: > qemu-img commit Commit takes a chain of images: [base] <-- [top] and commits the top image down into the base image: [base+top] > qemu-img rebase Rebase takes a chain of images: [base] <-- [top] And moves the top image onto a new base image, copying data as necessary to preserve the differential relationship: [different_base] <-- [top] > > I found they both can choose where to rebase(merge) the images. > commit can truncate or not on specific image. > rebase won't truncate rebased image. > > I found they have something similarity and I don't know what situation is > suitable for commit or rebase? > Commit is best when you have a single lineage of snapshots, like A<--B<--C and you decide you don't actually need all of those snapshots anymore, and would prefer to go back to a single image. Rebase is most useful when you have several different chains of images that are based on some common ancestor, and you would like to reparent an image on top of a cousin branch. (There are many more uses, but these ones are the most obvious to me personally.) --js
Re: [Qemu-block] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)
On 14.08.19 22:22, Maxim Levitsky wrote:
> This adds:
>
> * x-blockdev-update-encryption and x-blockdev-erase-encryption qmp commands
> Both commands take the QCryptoKeyManageOptions
> the x-blockdev-update-encryption is meant for non destructive addition
> of key slots / whatever the encryption driver supports in the future
>
> x-blockdev-erase-encryption is meant for destructive encryption key erase,
> in some cases even without way to recover the data.
>
>
> * bdrv_setup_encryption callback in the block driver
> This callback does both the above functions with 'action' parameter
>
> * QCryptoKeyManageOptions with set of options that drivers can use for
> encryption managment
> Currently it has all the options that LUKS needs, and later it can be
> extended
> (via union) to support more encryption drivers if needed
>
> * blk_setup_encryption / bdrv_setup_encryption - the usual block layer
> wrappers.
> Note that bdrv_setup_encryption takes BlockDriverState and not BdrvChild,
> for the ease of use from the qmp code. It is not expected that this function
> will be used by anything but qmp and qemu-img code
>
>
> Signed-off-by: Maxim Levitsky
> ---
> block/block-backend.c | 9
> block/io.c | 24
> blockdev.c | 40 ++
> include/block/block.h | 12 ++
> include/block/block_int.h | 11 ++
> include/sysemu/block-backend.h | 7 ++
> qapi/block-core.json | 36 ++
> qapi/crypto.json | 26 ++
> 8 files changed, 165 insertions(+)
Now I don’t know whether you want to keep this interface at all, because
the cover letter seemed to imply you’d prefer a QMP amend. But let it
be said that a QMP amend is no trivial task. I think the most difficult
bit is that the qcow2 implementation currently is inherently an offline
operation. It isn’t a good idea to use it on a live image. (Maybe it
works, but it’s definitely not what I had in mind when I wrote it.)
So I’ll still take a quick glance at the interface here.
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0d43d4f37c..53ed411eed 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5327,3 +5327,39 @@
>'data' : { 'node-name': 'str',
> 'iothread': 'StrOrNull',
> '*force': 'bool' } }
> +
> +
> +##
> +# @x-blockdev-update-encryption:
> +#
> +# Update the encryption keys for an encrypted block device
> +#
> +# @node-name: Name of the blockdev to operate on
> +# @force: Disable safety checks (use with care)
> +# @options: Driver specific options
> +#
> +
> +# Since: 4.2
> +##
> +{ 'command': 'x-blockdev-update-encryption',
> + 'data': { 'node-name' : 'str',
> +'*force' : 'bool',
> +'options': 'QCryptoEncryptionSetupOptions' } }
> +
> +##
> +# @x-blockdev-erase-encryption:
> +#
> +# Erase the encryption keys for an encrypted block device
> +#
> +# @node-name: Name of the blockdev to operate on
Why the tab?
> +# @force: Disable safety checks (use with care)
I think being a bit more verbose wouldn’t hurt.
(Same above.)
> +# @options: Driver specific options
> +#
> +# Returns: @QCryptoKeyManageResult
> +#
> +# Since: 4.2
> +##
> +{ 'command': 'x-blockdev-erase-encryption',
> + 'data': { 'node-name' : 'str',
> +'*force' : 'bool',
> +'options': 'QCryptoEncryptionSetupOptions' } }
> diff --git a/qapi/crypto.json b/qapi/crypto.json
> index b2a4cff683..69e8b086db 100644
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -309,3 +309,29 @@
>'base': 'QCryptoBlockInfoBase',
>'discriminator': 'format',
>'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> +
> +
> +##
> +# @QCryptoEncryptionSetupOptions:
> +#
> +# Driver specific options for encryption key management.
The options do seem LUKS-specific, but the name of this structure does not.
> +# @key-secret: the ID of a QCryptoSecret object providing the password
> +# to add or to erase (optional for erase)
> +#
> +# @old-key-secret: the ID of a QCryptoSecret object providing the password
> +# that can currently unlock the image
> +#
> +# @slot: Key slot to update/erase
> +#(optional, for update will select a free slot,
> +#for erase will erase all slots that match the password)
> +#
> +# @iter-time: number of milliseconds to spend in
> +# PBKDF passphrase processing. Currently defaults to 2000
> +# Since: 4.2
> +##
Does it really make sense to use the same structure for erasing and
updating? I think there are ways to represent @key-secret vs. @slot
being alternatives to each other for erase; @iter-time doesn’t seem to
make sense for erase; and @slot doesn’t seem to make sense for update.
Also, I don’t know whether to use @key-secret or @old-k
Re: [Qemu-block] [PATCH 12/13] qemu-img: implement key management
On 14.08.19 22:22, Maxim Levitsky wrote: > Signed-off-by: Maxim Levitsky > --- > block/crypto.c | 16 ++ > block/crypto.h | 3 + > qemu-img-cmds.hx | 13 + > qemu-img.c | 140 +++ > 4 files changed, 172 insertions(+) Yes, this seems a bit weird. Putting it under amend seems like the natural thing if that works; if not, I think it should be a single qemu-img subcommand instead of two. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] iotests: Check for enabled drivers before testing them
On 20.08.19 18:01, Thomas Huth wrote:
> On 8/20/19 5:01 PM, Max Reitz wrote:
>> On 19.08.19 09:53, Thomas Huth wrote:
>>> It is possible to enable only a subset of the block drivers with the
>>> "--block-drv-rw-whitelist" option of the "configure" script. All other
>>> drivers are marked as unusable (or only included as read-only with the
>>> "--block-drv-ro-whitelist" option). If an iotest is now using such a
>>> disabled block driver, it is failing - which is bad, since at least the
>>> tests in the "auto" group should be able to deal with this situation.
>>> Thus let's introduce a "_require_drivers" function that can be used by
>>> the shell tests to check for the availability of certain drivers first,
>>> and marks the test as "not run" if one of the drivers is missing.
>>
>> Well, the reasoning for generally not making blkdebug/blkverify explicit
>> requirements was that you should just have both enabled when running
>> iotests.
>
> Well, we disable blkverify in our downstream RHEL version of QEMU - so
> it would be great if the iotests could at least adapt to that missing
> driver.
I would like to say that RHEL is not a gold standard, but then I have
this series of selfish patches that specifically addresses RHEL
whitelisting issues.
It feels a bit weird to me to say “blkverify is not essential, because
RHEL disables it, but null-co is” – even though there is no reason why
anyone would need null-co except for testing either.
>> Of course, that no longer works as an argument now that we
>> unconditionally run some iotests in make check.
>>
>> But still, the question is how strict you want to be. If blkdebug
>> cannot be assumed to be present, what about null-co? What about raw?
>
> I tried to disable everything beside qcow2 - but that causes so many
> things to fail that it hardly makes sense to try to get that working.
Hm, really? I just whitelisted qcow2 and file and running the auto
group worked rather well (except for the failing tests you address here,
and the two others I mentioned).
> I think we can assume that at least null-co, qcow2 and raw are enabled.
> (If anybody still wants to try to run "make check" with one of these
> drivers disabled, I think we should rather add a superior check to
> tests/check-block.sh or tests/qemu-iotests/check instead and skip the
> iotests completely in that case).
I’m OK either way: We can add a global check, or we just make a decision
on what users definitely have to have enabled or the qemu build would be
a bit boring.
Assuming file, qcow2, and raw to be enabled seems reasonable. I’m not
so sure about null-co.
(I mean, I personally don’t really care. I never added such checks
myself, even a bit purposefully so, because it was my opinion that you
should probably have all block drivers whitelisted before running the
iotests. But then came CI and make check-block integration...)
((Also, you’ll notice that I was really inconsequential about adding
null-co checks in my “selfish patches” series, precisely because I
assumed everyone would have whitelisted null-co. So I’m indeed OK with
just making it an implicit prerequisite.))
>>> Signed-off-by: Thomas Huth
>>> ---
>>> tests/qemu-iotests/071 | 1 +
>>> tests/qemu-iotests/081 | 1 +
>>> tests/qemu-iotests/099 | 1 +
>>> tests/qemu-iotests/184 | 1 +
>>> tests/qemu-iotests/common.rc | 13 +
>>> 5 files changed, 17 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
>>> index 1cca9233d0..fab52b 100755
>>> --- a/tests/qemu-iotests/071
>>> +++ b/tests/qemu-iotests/071
>>> @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>
>>> _supported_fmt qcow2
>>> _supported_proto file
>>> +_require_drivers blkdebug blkverify
>>
>> Because this test also requires the raw driver.
>
> The test also works for me when I configured QEMU with:
>
> --block-drv-rw-whitelist="qcow2 file null-co blkdebug blkverify"
>
> i.e. the raw driver should be disabled in that case?
Ah, it’s just used by qemu-io, which of course ignores whitelisting.
>>>
>>> do_run_qemu()
>>> {
>>> diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
>>> index c418bab093..1695781bc0 100755
>>> --- a/tests/qemu-iotests/081
>>> +++ b/tests/qemu-iotests/081
>>> @@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>> _supported_fmt raw
>>> _supported_proto file
>>> _supported_os Linux
>>> +_require_drivers quorum
>>
>> This test has already a check whether quorum is supported, that should
>> be removed now.
>
> Hmm, true ... apparently that test was not working for my case ... could
> it be that qemu-img ignores the whitelist and simply says that all
> drivers are supported?
Ah, yeah. The whitelist is relevant only for the system emulator.
I forgot why we even had the existing check, then. Maybe just a mistake
to use qemu-img.
>> (Also, this test requires the raw driver.)
>
> Agreed, this test indeed does not work without 'raw'.
Re: [Qemu-block] [PATCH v10] qemu-io: add pattern file for write command
On 20.08.19 19:24, Eric Blake wrote:
> On 8/20/19 11:46 AM, Denis Plotnikov wrote:
>> The patch allows to provide a pattern file for write
>> command. There was no similar ability before.
>>
>> Signed-off-by: Denis Plotnikov
>> ---
>
>> @@ -983,8 +1057,9 @@ static int write_f(BlockBackend *blk, int argc, char
>> **argv)
>> /* Some compilers get confused and warn if this is not initialized. */
>> int64_t total = 0;
>> int pattern = 0xcd;
>> +const char *file_name = NULL;
>>
>> -while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
>> +while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {
>
> This one looks odd (I would have preserved ordering by sticking s:
> between q and u). But a maintainer could fix that.
>
>> switch (c) {
>> case 'b':
>> bflag = true;
>> @@ -1020,6 +1095,10 @@ static int write_f(BlockBackend *blk, int argc, char
>> **argv)
>> case 'z':
>> zflag = true;
>> break;
>> +case 's':
>> +sflag = true;
>> +file_name = optarg;
>> +break;
>
> Likewise, sorting the cases in the same order as the getopt() listing
> helps in finding code during later edits.
But it is in order of the getopt() listing. ;-)
>> @@ -1088,7 +1168,14 @@ static int write_f(BlockBackend *blk, int argc, char
>> **argv)
>> }
>>
>> if (!zflag) {
>> -buf = qemu_io_alloc(blk, count, pattern);
>> +if (sflag) {
>> +buf = qemu_io_alloc_from_file(blk, count, file_name);
>> +if (!buf) {
>> +return -EINVAL;
>> +}
>> +} else {
>> +buf = qemu_io_alloc(blk, count, pattern);
>> +}
>
> Pre-existing, but it is odd that qemu_io_alloc() exit()s rather than
> returning NULL on huge allocation requests that can't be met. (Then
> again, we have an early exit on any length > 2G, and 2G allocations tend
> to succeed on modern development machines). Perhaps it would be nice to
> teach qemu-io to use blk_try_blockalign for more graceful handling even
> on 32-bit platforms, but that's not the problem of your patch.
Then again, this is qemu-io. Printing an error instead of just aborting
doesn’t really help anyone.
Also, the code would be wrong without an early exit on a length >
INT_MAX. (Because pattern_len is an int, so the result of fread() might
overflow otherwise, which would be bad.)
(I just noticed that fread() might do a short read, but let’s just
ignore this at this point.)
> Option ordering is minor enough that I'm fine giving:
>
> Reviewed-by: Eric Blake
>
> Now, to figure out which maintainer should take it. Perhaps you want to
> add a patch 2/1 that adds an iotest using this new mode, to a) ensure it
> doesn't regress, and b) makes it reasonable to take in through the
> iotest tree.
Adding a test does not seem to bad of an idea, but I don’t see how that
would clarify things. Both qemu-io and the iotests are part of the
block layer core:
$ scripts/get_maintainer.pl -f qemu-io-cmds.c
Kevin Wolf (supporter:Block layer core)
Max Reitz (supporter:Block layer core)
qemu-block@nongnu.org (open list:Block layer core)
qemu-de...@nongnu.org (open list:All patches CC here)
$ scripts/get_maintainer.pl -f tests/qemu-iotests
Kevin Wolf (supporter:Block layer core)
Max Reitz (supporter:Block layer core)
qemu-block@nongnu.org (open list:Block layer core)
qemu-de...@nongnu.org (open list:All patches CC here)
So we only need to figure out whether it should be Kevin or me to take
it; but Kevin is on PTO, so that decision is simple. :-)
Therefor, I’ve changed the optstring (and switch case) order to be
alphabetical, and applied the patch to my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Thanks for the patch and the review,
Max
(I wouldn’t mind an iotest, but well. qemu-io itself is a testing
utility, so I don’t deem it important to test it.)
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] iotests: Check for enabled drivers before testing them
On 20.08.19 21:19, Thomas Huth wrote: > On 8/20/19 8:48 PM, Max Reitz wrote: >> On 20.08.19 18:01, Thomas Huth wrote: > [...] >>> Well, we disable blkverify in our downstream RHEL version of QEMU - so >>> it would be great if the iotests could at least adapt to that missing >>> driver. >> >> I would like to say that RHEL is not a gold standard > > Well, let's put it this way: The less changes we have to carry along > downstream (and thus review each time we rebase the downstream tree), > the more time we have to work on upstream. As I said, I’m guilty myself. >> It feels a bit weird to me to say “blkverify is not essential, because >> RHEL disables it, but null-co is” – even though there is no reason why >> anyone would need null-co except for testing either. > > Ok, fine for me, too, if we also declare "null-co" as optional for the > iotests - let's make sure that the tests in the "auto" group also work > without them. Well, should we or not? You said there are other tests (outside of the iotests) that break without null-co. If so, I don’t think there’s any point in making it optional here. Of course, that no longer works as an argument now that we unconditionally run some iotests in make check. But still, the question is how strict you want to be. If blkdebug cannot be assumed to be present, what about null-co? What about raw? >>> >>> I tried to disable everything beside qcow2 - but that causes so many >>> things to fail that it hardly makes sense to try to get that working. >> >> Hm, really? I just whitelisted qcow2 and file and running the auto >> group worked rather well (except for the failing tests you address here, >> and the two others I mentioned). > > IIRC I tried to run all qcow2 tests when I disabled null-co and saw lots > of failures ... but anyway, let's just focus on the "auto" tests right > now, that should be doable. OK, I didn’t bother running all. :-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v10] qemu-io: add pattern file for write command
On 8/20/19 1:24 PM, Eric Blake wrote:
> On 8/20/19 11:46 AM, Denis Plotnikov wrote:
>> The patch allows to provide a pattern file for write
>> command. There was no similar ability before.
>>
>> Signed-off-by: Denis Plotnikov
>> ---
>
>> @@ -983,8 +1057,9 @@ static int write_f(BlockBackend *blk, int argc, char
>> **argv)
>> /* Some compilers get confused and warn if this is not initialized. */
>> int64_t total = 0;
>> int pattern = 0xcd;
>> +const char *file_name = NULL;
>>
>> -while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
>> +while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {
>
> This one looks odd (I would have preserved ordering by sticking s:
> between q and u). But a maintainer could fix that.
>
>> switch (c) {
>> case 'b':
>> bflag = true;
>> @@ -1020,6 +1095,10 @@ static int write_f(BlockBackend *blk, int argc, char
>> **argv)
>> case 'z':
>> zflag = true;
>> break;
>> +case 's':
>> +sflag = true;
>> +file_name = optarg;
>> +break;
>
> Likewise, sorting the cases in the same order as the getopt() listing
> helps in finding code during later edits.
>
>> @@ -1088,7 +1168,14 @@ static int write_f(BlockBackend *blk, int argc, char
>> **argv)
>> }
>>
>> if (!zflag) {
>> -buf = qemu_io_alloc(blk, count, pattern);
>> +if (sflag) {
>> +buf = qemu_io_alloc_from_file(blk, count, file_name);
>> +if (!buf) {
>> +return -EINVAL;
>> +}
>> +} else {
>> +buf = qemu_io_alloc(blk, count, pattern);
>> +}
>
> Pre-existing, but it is odd that qemu_io_alloc() exit()s rather than
> returning NULL on huge allocation requests that can't be met. (Then
> again, we have an early exit on any length > 2G, and 2G allocations tend
> to succeed on modern development machines). Perhaps it would be nice to
> teach qemu-io to use blk_try_blockalign for more graceful handling even
> on 32-bit platforms, but that's not the problem of your patch.
>
> Option ordering is minor enough that I'm fine giving:
>
> Reviewed-by: Eric Blake
>
> Now, to figure out which maintainer should take it. Perhaps you want to
> add a patch 2/1 that adds an iotest using this new mode, to a) ensure it
> doesn't regress, and b) makes it reasonable to take in through the
> iotest tree.
>
Yes, this is a good idea. I'm sure over time we'll pick up uses of
pattern writing that will strengthen the the regression testing of the
feature, but for now a simple test case will help ensure it.
(It'll also help "document" how to use the feature for other test writers.)
Thanks!
Re: [Qemu-block] [PATCH] iotests: Check for enabled drivers before testing them
On 8/20/19 8:48 PM, Max Reitz wrote: > On 20.08.19 18:01, Thomas Huth wrote: [...] >> Well, we disable blkverify in our downstream RHEL version of QEMU - so >> it would be great if the iotests could at least adapt to that missing >> driver. > > I would like to say that RHEL is not a gold standard Well, let's put it this way: The less changes we have to carry along downstream (and thus review each time we rebase the downstream tree), the more time we have to work on upstream. > It feels a bit weird to me to say “blkverify is not essential, because > RHEL disables it, but null-co is” – even though there is no reason why > anyone would need null-co except for testing either. Ok, fine for me, too, if we also declare "null-co" as optional for the iotests - let's make sure that the tests in the "auto" group also work without them. >>> Of course, that no longer works as an argument now that we >>> unconditionally run some iotests in make check. >>> >>> But still, the question is how strict you want to be. If blkdebug >>> cannot be assumed to be present, what about null-co? What about raw? >> >> I tried to disable everything beside qcow2 - but that causes so many >> things to fail that it hardly makes sense to try to get that working. > > Hm, really? I just whitelisted qcow2 and file and running the auto > group worked rather well (except for the failing tests you address here, > and the two others I mentioned). IIRC I tried to run all qcow2 tests when I disabled null-co and saw lots of failures ... but anyway, let's just focus on the "auto" tests right now, that should be doable. Thomas signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] block: Use QEMU_IS_ALIGNED instead of reinventing it
On 8/17/19 1:53 PM, Nir Soffer wrote:
> Replace instances of:
>
> (n & (BDRV_SECTOR_SIZE - 1)) == 0)
>
> With:
>
> QEMU_IS_ALIGNED(n, BDRV_SECTOR_SIZE)
>
Do you have a magical incantation you used to find these?
> Which reveals the intent of the code better, and makes it easier to
> locate the code checking alignment.
>
> QEMU_IS_ALIGNED is implemented using %, which may be less efficient but
> it is used only in assert() except one instance, so it should not
> matter.
>
There is virtually no way these aren't optimized by the compiler.
> Signed-off-by: Nir Soffer
Therefore:
Reviewed-by: John Snow
> ---
> block/bochs.c | 4 ++--
> block/cloop.c | 4 ++--
> block/dmg.c | 4 ++--
> block/io.c| 8
> block/qcow2.c | 4 ++--
> block/vvfat.c | 8
> qemu-img.c| 2 +-
> 7 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/block/bochs.c b/block/bochs.c
> index 962f18592d..32bb83b268 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -248,8 +248,8 @@ bochs_co_preadv(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes,
> QEMUIOVector local_qiov;
> int ret;
>
> -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
>
> qemu_iovec_init(&local_qiov, qiov->niov);
> qemu_co_mutex_lock(&s->lock);
> diff --git a/block/cloop.c b/block/cloop.c
> index 384c9735bb..4de94876d4 100644
> --- a/block/cloop.c
> +++ b/block/cloop.c
> @@ -253,8 +253,8 @@ cloop_co_preadv(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes,
> int nb_sectors = bytes >> BDRV_SECTOR_BITS;
> int ret, i;
>
> -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
>
> qemu_co_mutex_lock(&s->lock);
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 45f6b28f17..4a045f2b3e 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -697,8 +697,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes,
> int nb_sectors = bytes >> BDRV_SECTOR_BITS;
> int ret, i;
>
> -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
>
> qemu_co_mutex_lock(&s->lock);
>
> diff --git a/block/io.c b/block/io.c
> index 56bbf195bb..7508703ecd 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1080,8 +1080,8 @@ static int coroutine_fn
> bdrv_driver_preadv(BlockDriverState *bs,
> sector_num = offset >> BDRV_SECTOR_BITS;
> nb_sectors = bytes >> BDRV_SECTOR_BITS;
>
> -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> assert(bytes <= BDRV_REQUEST_MAX_BYTES);
> assert(drv->bdrv_co_readv);
>
> @@ -1133,8 +1133,8 @@ static int coroutine_fn
> bdrv_driver_pwritev(BlockDriverState *bs,
> sector_num = offset >> BDRV_SECTOR_BITS;
> nb_sectors = bytes >> BDRV_SECTOR_BITS;
>
> -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> assert(bytes <= BDRV_REQUEST_MAX_BYTES);
>
> assert(drv->bdrv_co_writev);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 59cff1d4cb..41cab70e1d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2072,8 +2072,8 @@ static coroutine_fn int
> qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
> }
> if (bs->encrypted) {
> assert(s->crypto);
> -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +assert(QEMU_IS_ALIGNED(cur_bytes, BDRV_SECTOR_SIZE));
> if (qcow2_co_decrypt(bs, cluster_offset, offset,
> cluster_data, cur_bytes) < 0) {
> ret = -EIO;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index f6c28805dd..019b8f1341 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1547,8 +1547,8 @@ vvfat_co_preadv(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes,
> int nb_sectors = bytes >> BDRV_SECTOR_BITS;
> void *buf;
>
> -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +assert(QEMU_IS_ALIGNED(bytes, BDRV_
Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] backup: fix skipping unallocated clusters
On 8/20/19 4:28 AM, Vladimir Sementsov-Ogievskiy wrote: > 16.08.2019 22:11, John Snow wrote: >> >> >> On 8/14/19 12:54 PM, Vladimir Sementsov-Ogievskiy wrote: >>> >>> >>> 14 авг. 2019 г. 17:43 пользователь Vladimir Sementsov-Ogievskiy >>> написал: >>> >>> Hi all! >>> >>> There is a bug in not yet merged patch >>> "block/backup: teach TOP to never copy unallocated regions" >>> in https://github.com/jnsnow/qemu bitmaps. 04 fixes it. So, I propose >>> to put 01-03 somewhere before >>> "block/backup: teach TOP to never copy unallocated regions" >>> and squash 04 into "block/backup: teach TOP to never copy >>> unallocated regions" >>> >>> >>> Hmm, don't bother with it. Simpler is fix the bug in your commit by just >>> use skip_bytes variable when initializing dirty_end. >>> >> >> OK, just use Max's fix instead of this entire 4 patch series? >> >> --js >> > > Yes, I think it's OK > ACK, thank you!
Re: [Qemu-block] [PATCH] block: Use QEMU_IS_ALIGNED instead of reinventing it
On 17.08.19 19:53, Nir Soffer wrote: > Replace instances of: > > (n & (BDRV_SECTOR_SIZE - 1)) == 0) > > With: > > QEMU_IS_ALIGNED(n, BDRV_SECTOR_SIZE) > > Which reveals the intent of the code better, and makes it easier to > locate the code checking alignment. > > QEMU_IS_ALIGNED is implemented using %, which may be less efficient but > it is used only in assert() except one instance, so it should not > matter. > > Signed-off-by: Nir Soffer > --- > block/bochs.c | 4 ++-- > block/cloop.c | 4 ++-- > block/dmg.c | 4 ++-- > block/io.c| 8 > block/qcow2.c | 4 ++-- > block/vvfat.c | 8 > qemu-img.c| 2 +- > 7 files changed, 17 insertions(+), 17 deletions(-) Because John was speaking about a magical incantation, I found BDRV_SECTOR_MASK. There are two places in block/qcow2-cluster.c that use that to check alignment, I think those should be amended as well. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] block: Use QEMU_IS_ALIGNED instead of reinventing it
On Tue, Aug 20, 2019 at 10:30 PM John Snow wrote:
>
>
> On 8/17/19 1:53 PM, Nir Soffer wrote:
> > Replace instances of:
> >
> > (n & (BDRV_SECTOR_SIZE - 1)) == 0)
> >
> > With:
> >
> > QEMU_IS_ALIGNED(n, BDRV_SECTOR_SIZE)
> >
>
> Do you have a magical incantation you used to find these?
>
I found the first by accident, then I used:
git grep 'BDRV_SECTOR_SIZE - 1'
> Which reveals the intent of the code better, and makes it easier to
> > locate the code checking alignment.
> >
> > QEMU_IS_ALIGNED is implemented using %, which may be less efficient but
> > it is used only in assert() except one instance, so it should not
> > matter.
> >
>
> There is virtually no way these aren't optimized by the compiler.
>
Makes sense, but I did not check.
>
> > Signed-off-by: Nir Soffer
>
> Therefore:
>
> Reviewed-by: John Snow
>
> > ---
> > block/bochs.c | 4 ++--
> > block/cloop.c | 4 ++--
> > block/dmg.c | 4 ++--
> > block/io.c| 8
> > block/qcow2.c | 4 ++--
> > block/vvfat.c | 8
> > qemu-img.c| 2 +-
> > 7 files changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/block/bochs.c b/block/bochs.c
> > index 962f18592d..32bb83b268 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -248,8 +248,8 @@ bochs_co_preadv(BlockDriverState *bs, uint64_t
> offset, uint64_t bytes,
> > QEMUIOVector local_qiov;
> > int ret;
> >
> > -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> > +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> >
> > qemu_iovec_init(&local_qiov, qiov->niov);
> > qemu_co_mutex_lock(&s->lock);
> > diff --git a/block/cloop.c b/block/cloop.c
> > index 384c9735bb..4de94876d4 100644
> > --- a/block/cloop.c
> > +++ b/block/cloop.c
> > @@ -253,8 +253,8 @@ cloop_co_preadv(BlockDriverState *bs, uint64_t
> offset, uint64_t bytes,
> > int nb_sectors = bytes >> BDRV_SECTOR_BITS;
> > int ret, i;
> >
> > -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> > +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> >
> > qemu_co_mutex_lock(&s->lock);
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 45f6b28f17..4a045f2b3e 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -697,8 +697,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes,
> > int nb_sectors = bytes >> BDRV_SECTOR_BITS;
> > int ret, i;
> >
> > -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> > +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> >
> > qemu_co_mutex_lock(&s->lock);
> >
> > diff --git a/block/io.c b/block/io.c
> > index 56bbf195bb..7508703ecd 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1080,8 +1080,8 @@ static int coroutine_fn
> bdrv_driver_preadv(BlockDriverState *bs,
> > sector_num = offset >> BDRV_SECTOR_BITS;
> > nb_sectors = bytes >> BDRV_SECTOR_BITS;
> >
> > -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> > +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> > assert(bytes <= BDRV_REQUEST_MAX_BYTES);
> > assert(drv->bdrv_co_readv);
> >
> > @@ -1133,8 +1133,8 @@ static int coroutine_fn
> bdrv_driver_pwritev(BlockDriverState *bs,
> > sector_num = offset >> BDRV_SECTOR_BITS;
> > nb_sectors = bytes >> BDRV_SECTOR_BITS;
> >
> > -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > -assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> > +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> > assert(bytes <= BDRV_REQUEST_MAX_BYTES);
> >
> > assert(drv->bdrv_co_writev);
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 59cff1d4cb..41cab70e1d 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2072,8 +2072,8 @@ static coroutine_fn int
> qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
> > }
> > if (bs->encrypted) {
> > assert(s->crypto);
> > -assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > -assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > +assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> > +assert(QEMU_IS_ALIGNED(cur_bytes, BDRV_SECTOR_SIZE));
> > if (qcow2_co_decrypt(bs, cluster_offset, offset,
> > cluster_data, cur_bytes) < 0) {
> > ret = -EIO;
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index f6c28805dd..019b8f1341 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvf
Re: [Qemu-block] [PATCH] block: Use QEMU_IS_ALIGNED instead of reinventing it
On Tue, Aug 20, 2019 at 10:51 PM Max Reitz wrote: > On 17.08.19 19:53, Nir Soffer wrote: > > Replace instances of: > > > > (n & (BDRV_SECTOR_SIZE - 1)) == 0) > > > > With: > > > > QEMU_IS_ALIGNED(n, BDRV_SECTOR_SIZE) > > > > Which reveals the intent of the code better, and makes it easier to > > locate the code checking alignment. > > > > QEMU_IS_ALIGNED is implemented using %, which may be less efficient but > > it is used only in assert() except one instance, so it should not > > matter. > > > > Signed-off-by: Nir Soffer > > --- > > block/bochs.c | 4 ++-- > > block/cloop.c | 4 ++-- > > block/dmg.c | 4 ++-- > > block/io.c| 8 > > block/qcow2.c | 4 ++-- > > block/vvfat.c | 8 > > qemu-img.c| 2 +- > > 7 files changed, 17 insertions(+), 17 deletions(-) > > Because John was speaking about a magical incantation, I found > BDRV_SECTOR_MASK. There are two places in block/qcow2-cluster.c that > use that to check alignment, I think those should be amended as well. > $ git grep BDRV_SECTOR_MASK block/qcow2-cluster.c:assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0); block/qcow2-cluster.c:assert((bytes & ~BDRV_SECTOR_MASK) == 0); Right, should use QEMU_IS_ALIGNED include/block/block.h:#define BDRV_SECTOR_MASK ~(BDRV_SECTOR_SIZE - 1) include/block/block.h:#define BDRV_BLOCK_OFFSET_MASK BDRV_SECTOR_MASK migration/block.c:flags = addr & ~BDRV_SECTOR_MASK; This could use (BDRV_SECTOR_SIZE - 1). In Linux SECTOR_MASK is defined as: drivers/block/null_blk_main.c:#define SECTOR_MASK (PAGE_SECTORS - 1) It seems that qemu use use the same. I will try to handle these in v2. Nir
Re: [Qemu-block] [PATCH v3 2/8] iotests: Prefer null-co over null-aio
On 8/19/19 4:22 PM, Max Reitz wrote: > On 19.08.19 22:18, Max Reitz wrote: >> We use null-co basically everywhere in the iotests. Unless we want to >> test null-aio specifically, we should use it instead (for consistency). >> >> Signed-off-by: Max Reitz >> Reviewed-by: John Snow > > Hm, sorry, I just noticed that I probably should have dropped this R-b. :-/ > > (I mean, apart from the rebase conflict, nothing has changed, but still.) > > Max > It's fine.
Re: [Qemu-block] [PATCH] nbd: Advertise multi-conn for shared read-only connections
On Mon, Aug 19, 2019 at 9:04 PM Eric Blake wrote: > On 8/17/19 8:31 PM, Nir Soffer wrote: > >>> Also, for qemu-nbd, shouldn't we allow -e only together with -r ? > >> > >> I'm reluctant to; it might break whatever existing user is okay exposing > >> it (although such users are questionable, so maybe we can argue they > >> were already broken). Maybe it's time to start a deprecation cycle? > >> > > > > man qemu-nbd (on Centos 7.6) says: > > > >-e, --shared=num > >Allow up to num clients to share the device (default 1) > > > > I see that in qemu-img 4.1 there is a note about consistency with > writers: > > > >-e, --shared=num > >Allow up to num clients to share the device (default 1). Safe > > for readers, but for now, consistency is not guaranteed between multiple > > writers. > > But it is not clear what are the consistency guarantees. > > > > Supporting multiple writers is important. oVirt is giving the user a URL > > (since 4.3), and the user > > can use multiple connections using the same URL, each having a connection > > to the same qemu-nbd > > socket. I know that some backup vendors tried to use multiple connections > > to speed up backups, and > > they may try to do this also for restore. > > > > An interesting use case would be using multiple connections on client > side > > to write in parallel to > > same image, when every client is writing different ranges. > > Good to know. > > > > > Do we have real issue in qemu-nbd serving multiple clients writing to > > different parts of > > the same image? > > If a server advertises multi-conn on a writable image, then clients have > stronger guarantees about behavior on what happens with flush on one > client vs. write in another, to the point that you can make some better > assumptions about image consistency, including what one client will read > after another has written. But as long as multiple clients only ever > access distinct portions of the disk, then multi-conn is not important > to that client (whether for reading or for writing). > Thanks for making this clear. I think we need to document this in oVirt, so users will be careful about using multiple connections. > > So it sounds like I have no reason to deprecate qemu-nbd -e 2, even for > writable images. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org > >
Re: [Qemu-block] [Qemu-devel] [PATCH v3 4/8] iotests: Use case_skip() in skip_if_unsupported()
On 8/19/19 4:18 PM, Max Reitz wrote:
> skip_if_unsupported() should use the stronger variant case_skip(),
> because this allows it to be used even with setUp() (in a meaningful
> way).
>
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/iotests.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 2f53baf633..726f904f50 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -896,7 +896,7 @@ def skip_if_unsupported(required_formats=[],
> read_only=False):
> usf_list = list(set(required_formats) -
> set(supported_formats(read_only)))
> if usf_list:
> -case_notrun('{}: formats {} are not whitelisted'.format(
> +args[0].case_skip('{}: formats {} are not
> whitelisted'.format(
> args[0], usf_list))
> else:
> return func(*args, **kwargs)
>
Should we promote args[0] to a named argument here, because we depend on
it having a specific type? It's not truly as polymorphic as we're making
it appear.
That type here is iotests.QMPTestCase because we're relying on case_skip
being present.
def test_wrapper(test_case, *args, **kwargs):
...
return func(test_case, *args, **kwargs)
--js
Re: [Qemu-block] [Qemu-devel] [PATCH v3 8/8] iotests: Cache supported_formats()
Any particular reason? I guess an "itch."
On 8/19/19 4:18 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/iotests.py | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 8f315538e9..f6492b355f 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -884,9 +884,17 @@ def qemu_pipe(*args):
> def supported_formats(read_only=False):
> '''Set 'read_only' to True to check ro-whitelist
> Otherwise, rw-whitelist is checked'''
> -format_message = qemu_pipe("-drive", "format=help")
> -line = 1 if read_only else 0
> -return format_message.splitlines()[line].split(":")[1].split()
> +
> +if not hasattr(supported_formats, "formats"):
> +supported_formats.formats = {}
> +
> +if read_only not in supported_formats.formats:
> +format_message = qemu_pipe("-drive", "format=help")
> +line = 1 if read_only else 0
> +supported_formats.formats[read_only] = \
> +format_message.splitlines()[line].split(":")[1].split()
> +
Seems a little odd to use a boolean as a dict key, but I guess it works
and is fine.
> +return supported_formats.formats[read_only]
>
> def skip_if_unsupported(required_formats=[], read_only=False):
> '''Skip Test Decorator
>
Eh, why not.
Reviewed-by: John Snow
Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/8] iotests: Allow skipping test cases
On 8/19/19 4:18 PM, Max Reitz wrote:
> case_notrun() does not actually skip the current test case. It just
> adds a "notrun" note and then returns to the caller, who manually has to
> skip the test. Generally, skipping a test case is as simple as
> returning from the current function, but not always: For example, this
> model does not allow skipping tests already in the setUp() function.
>
> Thus, add a QMPTestCase.case_skip() function that invokes case_notrun()
> and then self.skipTest(). To make this work, we need to filter the
> information on how many test cases were skipped from the unittest
> output.
>
> Signed-off-by: Max Reitz
Hm, didn't someone else send a patch like this recently?
Ah, yes:
[Qemu-block] [PATCH v5 3/6] iotests: Add casenotrun report to bash tests
Oh, theirs is for bash. Moving along.
> ---
> tests/qemu-iotests/iotests.py | 21 ++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 84438e837c..2f53baf633 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -802,6 +802,11 @@ class QMPTestCase(unittest.TestCase):
> return self.pause_wait(job_id)
> return result
>
> +def case_skip(self, reason):
> +'''Skip this test case'''
> +case_notrun(reason)
> +self.skipTest(reason)
> +
>
> def notrun(reason):
> '''Skip this test suite'''
> @@ -813,7 +818,10 @@ def notrun(reason):
> sys.exit(0)
>
> def case_notrun(reason):
> -'''Skip this test case'''
> +'''Mark this test case as not having been run, but do not actually
> +skip it; that is left to the caller. See QMPTestCase.case_skip()
> +for a variant that actually skips the current test case.'''
> +
> # Each test in qemu-iotests has a number ("seq")
> seq = os.path.basename(sys.argv[0])
>
> @@ -904,8 +912,15 @@ def execute_unittest(output, verbosity, debug):
> unittest.main(testRunner=runner)
> finally:
> if not debug:
> -sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s',
> -r'Ran \1 tests', output.getvalue()))
> +out = output.getvalue()
> +out = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests',
> out)
> +
> +# Hide skipped tests from the reference output
> +out = re.sub(r'OK \(skipped=\d+\)', 'OK', out)
> +out_first_line, out_rest = out.split('\n', 1)
> +out = out_first_line.replace('s', '.') + '\n' + out_rest
> +
> +sys.stderr.write(out)
>
> def execute_test(test_function=None,
> supported_fmts=[], supported_oses=['linux'],
>
okey dokey.
Reviewed-by: John Snow
Re: [Qemu-block] [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093
On 8/19/19 4:18 PM, Max Reitz wrote:
> null-aio may not be whitelisted. Skip all test cases that require it.
>
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/093 | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> index 50c1e7f2ec..f03fa24a07 100755
> --- a/tests/qemu-iotests/093
> +++ b/tests/qemu-iotests/093
> @@ -24,7 +24,7 @@ import iotests
> nsec_per_sec = 10
>
> class ThrottleTestCase(iotests.QMPTestCase):
> -test_img = "null-aio://"
> +test_driver = "null-aio"
> max_drives = 3
>
> def blockstats(self, device):
> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
> return stat['rd_bytes'], stat['rd_operations'],
> stat['wr_bytes'], stat['wr_operations']
> raise Exception("Device not found for blockstats: %s" % device)
>
> +def required_drivers(self):
> +return [self.test_driver]
> +
> +@iotests.skip_if_unsupported(required_drivers)
Oh, I see why you're passing args[0] back to the callback now. Why not
just pass self.required_drivers and call it with no arguments instead?
You can get a bound version that way that doesn't need additional
arguments, and then the callback is free to take generic callables of
any kind.
> def setUp(self):
> self.vm = iotests.VM()
> for i in range(0, self.max_drives):
> -self.vm.add_drive(self.test_img, "file.read-zeroes=on")
> +self.vm.add_drive(self.test_driver + "://",
> "file.read-zeroes=on")
> self.vm.launch()
>
> def tearDown(self):
> @@ -264,7 +268,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
> self.assertEqual(self.blockstats('drive1')[0], 4096)
>
> class ThrottleTestCoroutine(ThrottleTestCase):
> -test_img = "null-co://"
> +test_driver = "null-co"
>
> class ThrottleTestGroupNames(iotests.QMPTestCase):
> max_drives = 3
> @@ -425,4 +429,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
>
>
> if __name__ == '__main__':
> +if 'null-co' not in iotests.supported_formats():
> +iotests.notrun('null-co driver support missing')
> iotests.main(supported_fmts=["raw"])
>
Re: [Qemu-block] [Qemu-devel] [PATCH v3 5/8] iotests: Let skip_if_unsupported() accept a method
On 8/19/19 4:18 PM, Max Reitz wrote:
> This lets tests use skip_if_unsupported() with a potentially variable
> list of required formats.
>
> Suggested-by: Kevin Wolf
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/iotests.py | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 726f904f50..8f315538e9 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -893,8 +893,12 @@ def skip_if_unsupported(required_formats=[],
> read_only=False):
> Runs the test if all the required formats are whitelisted'''
> def skip_test_decorator(func):
> def func_wrapper(*args, **kwargs):
> -usf_list = list(set(required_formats) -
> -set(supported_formats(read_only)))
> +if callable(required_formats):
> +fmts = required_formats(args[0])
> +else:
> +fmts = required_formats
> +
> +usf_list = list(set(fmts) - set(supported_formats(read_only)))
> if usf_list:
> args[0].case_skip('{}: formats {} are not
> whitelisted'.format(
> args[0], usf_list))
>
I am required to inform you that this is in direct violation of the
pythonista treaty of 2007; which mandates that you try to call and fail
instead of attempting to gracefully check ahead of time.
Luckily, I am not fond of such rules.
Reviewed-by: John Snow
[Qemu-block] QEMU bitmap backup usability FAQ
Hi, downstream here at Red Hat I've been fielding some questions about the usability and feature readiness of Bitmaps (and related features) in QEMU. Here are some questions I answered internally that I am copying to the list for two reasons: (1) To make sure my answers are actually correct, and (2) To share this pseudo-reference with the community at large. This is long, and mostly for reference. There's a summary at the bottom with some todo items and observations about the usability of the feature as it exists in QEMU. Before too long, I intend to send a more summarized "roadmap" mail which details all of the current and remaining work to be done in and around the bitmaps feature in QEMU. Questions: > "What format(s) is/are required for this functionality?" >From the QEMU API, any format can be used to create and author incremental backups. The only known format limitations are: 1. Persistent bitmaps cannot be created on any format except qcow2, although there are hooks to add support to other formats at a later date if desired. DANGER CAVEAT #1: Adding bitmaps to QEMU by default creates transient bitmaps instead of persistent ones. Possible TODO: Allow users to 'upgrade' transient bitmaps to persistent ones in case they made a mistake. 2. When using push backups (blockdev-backup, drive-backup), you may use any format as a target format. DANGER CAVEAT #2: without backing file and/or filesystem-less sparse support, these images will be unusable. EXAMPLE: Backing up to a raw file loses allocation information, so we can no longer distinguish between zeroes and unallocated regions. The cluster size is also lost. This file will not be usable without additional metadata recorded elsewhere.* (* This is complicated, but it is in theory possible to do a push backup to e.g. an NBD target with custom server code that saves allocation information to a metadata file, which would allow you to reconstruct backups. For instance, recording in a .json file which extents were written out would allow you to -- with a custom binary -- write this information on top of a base file to reconstruct a backup.) 3. Any format can be used for either shared storage or live storage migrations. There are TWO distinct mechanisms for migrating bitmaps: A) The bitmap is flushed to storage and re-opened on the destination. This is only supported for qcow2 and shared-storage migrations. B) The bitmap is live-migrated to the destination. This is supported for any format and can be used for either shared storage or live storage migrations. DANGER CAVEAT #3: The second bitmap migration technique there is an optional migration capability that must be enabled explicitly. Otherwise, some migration combinations may drop bitmaps. Matrix: > migrate = migrate_capability or (persistent and shared_storage) Enumerated: live storage + raw : transient + no-capability: Dropped live-storage + raw : transient + bm-capability: Migrated live-storage + qcow2 : transient + no-capability: Dropped live-storage + qcow2 : transient + bm-capability: Migrated live-storage + qcow2 : persistent + no-capability: Dropped (!) live-storage + qcow2 : persistent + bm-capability: Migrated shared-storage + raw : transient - no-capability: Dropped shared-storage + raw : transient + bm-capability: Migrated shared-storage + qcow2 : transient + no-capability: Migrated shared-storage + qcow2 : transient + bm-capability: Migrated shared-storage + qcow2 : persistent + no-capability: Migrated shared-storage + qcow2 : persistent + bm-capability: Migrated Enabling the bitmap migration capability will ALWAYS migrate the bitmap. If it's disabled, we will only migrate the bitmaps for shared storage migrations where the bitmap is persistent, which is a qcow2-only case. There is no warning or error if you attempt to migrate in a manner that loses your bitmaps. (I might be persuaded to add a case for when you are doing a live storage migration of qcow2 with persistent bitmaps, which is somewhat a conflicting case: you've asked for the bitmap to be persistent, but it seems likely that if this ever happens in practice, it's because you have neglected to ask for it to be migrated to the new host.) See iotest 169 for more details on this. At present, these are the only format limitations I am consciously aware of. From a management API/GUI perspective, it makes sense to restrict the feature set to "qcow2 only" to minimize edge cases. > "Is libvirt aware of these 'gotcha' cases?" >From talks I've had with Eric Blake and Peter Krempa, they certainly are now. > "Is it possible to make persistent the default?" Not quickly. In QEMU, not without a deprecation period or some other incompatibility. Default values are not (yet?) introspectable via the schema. We need (possibly) up to two QAPI extensions: I) The ability to return deprecation warnings when issuing a command that will cease to work in the future. This has been being discussed somewhat on-list recently.
Re: [Qemu-block] [Qemu-devel] [PATCH] block/backup: install notifier during creation
ping, y'all
On 8/9/19 4:13 PM, John Snow wrote:
> Backup jobs may yield prior to installing their handler, because of the
> job_co_entry shim which guarantees that a job won't begin work until
> we are ready to start an entire transaction.
>
> Unfortunately, this makes proving correctness about transactional
> points-in-time for backup hard to reason about. Make it explicitly clear
> by moving the handler registration to creation time, and changing the
> write notifier to a no-op until the job is started.
>
> Reported-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: John Snow
> ---
> block/backup.c | 32 +++-
> include/qemu/job.h | 5 +
> job.c | 2 +-
> 3 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 07d751aea4..4df5b95415 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -344,6 +344,13 @@ static int coroutine_fn backup_before_write_notify(
> assert(QEMU_IS_ALIGNED(req->offset, BDRV_SECTOR_SIZE));
> assert(QEMU_IS_ALIGNED(req->bytes, BDRV_SECTOR_SIZE));
>
> +/* The handler is installed at creation time; the actual point-in-time
> + * starts at job_start(). Transactions guarantee those two points are
> + * the same point in time. */
> +if (!job_started(&job->common.job)) {
> +return 0;
> +}
> +
> return backup_do_cow(job, req->offset, req->bytes, NULL, true);
> }
>
> @@ -398,6 +405,12 @@ static void backup_clean(Job *job)
> BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
> BlockDriverState *bs = blk_bs(s->common.blk);
>
> +/* cancelled before job_start: remove write_notifier */
> +if (s->before_write.notify) {
> +notifier_with_return_remove(&s->before_write);
> +s->before_write.notify = NULL;
> +}
> +
> if (s->copy_bitmap) {
> bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
> s->copy_bitmap = NULL;
> @@ -527,17 +540,8 @@ static void backup_init_copy_bitmap(BackupBlockJob *job)
> static int coroutine_fn backup_run(Job *job, Error **errp)
> {
> BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
> -BlockDriverState *bs = blk_bs(s->common.blk);
> int ret = 0;
>
> -QLIST_INIT(&s->inflight_reqs);
> -qemu_co_rwlock_init(&s->flush_rwlock);
> -
> -backup_init_copy_bitmap(s);
> -
> -s->before_write.notify = backup_before_write_notify;
> -bdrv_add_before_write_notifier(bs, &s->before_write);
> -
> if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
> int64_t offset = 0;
> int64_t count;
> @@ -572,6 +576,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
>
> out:
> notifier_with_return_remove(&s->before_write);
> +s->before_write.notify = NULL;
>
> /* wait until pending backup_do_cow() calls have completed */
> qemu_co_rwlock_wrlock(&s->flush_rwlock);
> @@ -767,6 +772,15 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> &error_abort);
> job->len = len;
>
> +/* Finally, install a write notifier that takes effect after job_start()
> */
> +backup_init_copy_bitmap(job);
> +
> +QLIST_INIT(&job->inflight_reqs);
> +qemu_co_rwlock_init(&job->flush_rwlock);
> +
> +job->before_write.notify = backup_before_write_notify;
> +bdrv_add_before_write_notifier(bs, &job->before_write);
> +
> return &job->common;
>
> error:
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 9e7cd1e4a0..733afb696b 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -394,6 +394,11 @@ void job_enter_cond(Job *job, bool(*fn)(Job *job));
> */
> void job_start(Job *job);
>
> +/**
> + * job_started returns true if the @job has started.
> + */
> +bool job_started(Job *job);
> +
> /**
> * @job: The job to enter.
> *
> diff --git a/job.c b/job.c
> index 28dd48f8a5..745af659ff 100644
> --- a/job.c
> +++ b/job.c
> @@ -243,7 +243,7 @@ bool job_is_completed(Job *job)
> return false;
> }
>
> -static bool job_started(Job *job)
> +bool job_started(Job *job)
> {
> return job->co;
> }
>
[Qemu-block] [PATCH v3 2/4] iotest 258: use script_main
Since this one is nicely factored to use a single entry point, use script_main to run the tests. --- tests/qemu-iotests/258 | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258 index b84cf02254..1372522c7a 100755 --- a/tests/qemu-iotests/258 +++ b/tests/qemu-iotests/258 @@ -23,11 +23,6 @@ import iotests from iotests import log, qemu_img, qemu_io_silent, \ filter_qmp_testfiles, filter_qmp_imgfmt -# Need backing file and change-backing-file support -iotests.verify_image_format(supported_fmts=['qcow2', 'qed']) -iotests.verify_platform(['linux']) - - # Returns a node for blockdev-add def node(node_name, path, backing=None, fmt=None, throttle=None): if fmt is None: @@ -160,4 +155,5 @@ def main(): test_concurrent_finish(False) if __name__ == '__main__': -main() +# Need backing file and change-backing-file support +iotests.script_main(main, supported_fmts=['qcow2', 'qed']) -- 2.21.0
[Qemu-block] [PATCH v3 1/4] iotests: add script_initialize
Like script_main, but doesn't require a single point of entry.
Replace all existing initialization sections with this drop-in replacement.
This brings debug support to all existing script-style iotests.
Note: supported_oses=['linux'] was omitted, as it is a default argument.
---
tests/qemu-iotests/149| 3 +-
tests/qemu-iotests/194| 3 +-
tests/qemu-iotests/202| 3 +-
tests/qemu-iotests/203| 3 +-
tests/qemu-iotests/206| 2 +-
tests/qemu-iotests/207| 2 +-
tests/qemu-iotests/208| 2 +-
tests/qemu-iotests/209| 2 +-
tests/qemu-iotests/210| 2 +-
tests/qemu-iotests/211| 2 +-
tests/qemu-iotests/212| 2 +-
tests/qemu-iotests/213| 2 +-
tests/qemu-iotests/216| 3 +-
tests/qemu-iotests/218| 2 +-
tests/qemu-iotests/219| 2 +-
tests/qemu-iotests/222| 5 ++-
tests/qemu-iotests/224| 3 +-
tests/qemu-iotests/228| 3 +-
tests/qemu-iotests/234| 3 +-
tests/qemu-iotests/235| 4 +--
tests/qemu-iotests/236| 2 +-
tests/qemu-iotests/237| 2 +-
tests/qemu-iotests/238| 2 ++
tests/qemu-iotests/242| 2 +-
tests/qemu-iotests/246| 2 +-
tests/qemu-iotests/248| 2 +-
tests/qemu-iotests/254| 2 +-
tests/qemu-iotests/255| 2 +-
tests/qemu-iotests/256| 2 +-
tests/qemu-iotests/262| 3 +-
tests/qemu-iotests/iotests.py | 58 +++
31 files changed, 71 insertions(+), 61 deletions(-)
diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 4f363f295f..9fa97966c5 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -383,8 +383,7 @@ def test_once(config, qemu_img=False):
# Obviously we only work with the luks image format
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_platform()
+iotests.script_initialize(supported_fmts=['luks'])
# We need sudo in order to run cryptsetup to create
# dm-crypt devices. This is safe to use on any
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index d746ab1e21..c8aeb6d0e4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,8 +21,7 @@
import iotests
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'])
with iotests.FilePath('source.img') as source_img_path, \
iotests.FilePath('dest.img') as dest_img_path, \
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 581ca34d79..1271ac9459 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -24,8 +24,7 @@
import iotests
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'])
with iotests.FilePath('disk0.img') as disk0_img_path, \
iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index 4874a1a0d8..c40fe231ea 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -24,8 +24,7 @@
import iotests
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'])
with iotests.FilePath('disk0.img') as disk0_img_path, \
iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index 5bb738bf23..23ff2f624b 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -23,7 +23,7 @@
import iotests
from iotests import imgfmt
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
def blockdev_create(vm, options):
result = vm.qmp_log('blockdev-create',
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index ec8c1d06f0..ab9e3b6747 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,7 +24,7 @@ import iotests
import subprocess
import re
-iotests.verify_image_format(supported_fmts=['raw'])
+iotests.script_initialize(supported_fmts=['raw'])
iotests.verify_protocol(supported=['ssh'])
def filter_hash(qmsg):
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1e202388dc..dfce6f9fe4 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -22,7 +22,7 @@
import iotests
-iotests.verify_image_format(supported_fmts=['generic'])
+iotests.script_initialize(supported_fmts=['generic'])
with iotests.FilePath('disk.img') as disk_img_path, \
iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
diff --git a/tests/qemu-iotests/209 b/tests/qemu-iotests/209
index 259e991ec6..a77f884166 100755
--- a/tests/qemu-iotests/209
+++ b/tests/qemu-iotests/209
@@ -22,7 +22,7 @@ import iotests
from iotests import qemu_img_create, qemu_io, qemu_img_verbose, qemu_nbd, \
fi
[Qemu-block] [PATCH v3 0/4] iotests: use python logging
This series uses python logging to enable output conditionally on iotests.log(). We unify an initialization call (which also enables debugging output for those tests with -d) and then make the switch inside of iotests. It will help alleviate the need to create logged/unlogged versions of all the various helpers we have made. V3: - Rebased for 4.1+; now based on main branch. V2: - Added all of the other python tests I missed to use script_initialize - Refactored the common setup as per Ehabkost's suggestion - Added protocol arguments to common initialization, but this isn't strictly required. John Snow (4): iotests: add script_initialize iotest 258: use script_main iotests: add protocol support to initialization info iotests: use python logging for iotests.log() tests/qemu-iotests/030| 4 +- tests/qemu-iotests/149| 3 +- tests/qemu-iotests/194| 3 +- tests/qemu-iotests/202| 3 +- tests/qemu-iotests/203| 3 +- tests/qemu-iotests/206| 2 +- tests/qemu-iotests/207| 4 +- tests/qemu-iotests/208| 2 +- tests/qemu-iotests/209| 2 +- tests/qemu-iotests/210| 4 +- tests/qemu-iotests/211| 4 +- tests/qemu-iotests/212| 4 +- tests/qemu-iotests/213| 4 +- tests/qemu-iotests/216| 3 +- tests/qemu-iotests/218| 2 +- tests/qemu-iotests/219| 2 +- tests/qemu-iotests/222| 5 +- tests/qemu-iotests/224| 3 +- tests/qemu-iotests/228| 3 +- tests/qemu-iotests/234| 3 +- tests/qemu-iotests/235| 4 +- tests/qemu-iotests/236| 2 +- tests/qemu-iotests/237| 2 +- tests/qemu-iotests/238| 2 + tests/qemu-iotests/242| 2 +- tests/qemu-iotests/245| 1 + tests/qemu-iotests/245.out| 24 tests/qemu-iotests/246| 2 +- tests/qemu-iotests/248| 2 +- tests/qemu-iotests/254| 2 +- tests/qemu-iotests/255| 2 +- tests/qemu-iotests/256| 2 +- tests/qemu-iotests/258| 8 +-- tests/qemu-iotests/262| 3 +- tests/qemu-iotests/iotests.py | 108 ++ 35 files changed, 124 insertions(+), 105 deletions(-) -- 2.21.0
[Qemu-block] [PATCH v3 4/4] iotests: use python logging for iotests.log()
We can turn logging on/off globally instead of per-function.
Remove use_log from run_job, and use python logging to turn on
diffable output when we run through a script entry point.
iotest 245 changes output order due to buffering reasons.
---
tests/qemu-iotests/030| 4 +--
tests/qemu-iotests/245| 1 +
tests/qemu-iotests/245.out| 24 +-
tests/qemu-iotests/iotests.py | 47 +--
4 files changed, 43 insertions(+), 33 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1b69f318c6..a382cb430b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
self.assert_qmp(result, 'return', {})
-self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
-self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+self.vm.run_job(job='drive0', auto_dismiss=True)
+self.vm.run_job(job='node4', auto_dismiss=True)
self.assert_no_active_block_jobs()
# Test a block-stream and a block-commit job in parallel
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bc1ceb9792..3bc29acb33 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1000,4 +1000,5 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'backing': 'hd2'})
if __name__ == '__main__':
+iotests.activate_logging()
iotests.main(supported_fmts=["qcow2"])
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index a19de5214d..15c3630e92 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,17 +1,17 @@
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0,
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0,
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0,
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
..
--
Ran 18 tests
OK
-{"execute": "job-finalize", "arguments": {"id": "commit0"}}
-{"return": {}}
-{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0,
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0,
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0,
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 661d7f93bf..b97cc2fab2 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -35,6 +35,13 @@ from collections import OrderedDict
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu import qtest
+# Use this logger for logging messages directly from the iotests module
+logger = logging.getLogger(__name__)
+logger.addHandler(logging.NullHandler())
+
+# Use this logger for messages that ought to be used for diff output.
+test_logger = logging.getLogger('.'.join((__name__, 'iotest')))
+test_logger.addHandler(logging.NullH
[Qemu-block] [PATCH v3 3/4] iotests: add protocol support to initialization info
This will add supported_protocols and unsupported_protocols to all of
iotests.main, iotests.script_main, and iotests.script_initialize.
---
tests/qemu-iotests/207| 4 ++--
tests/qemu-iotests/210| 4 ++--
tests/qemu-iotests/211| 4 ++--
tests/qemu-iotests/212| 4 ++--
tests/qemu-iotests/213| 4 ++--
tests/qemu-iotests/iotests.py | 5 -
6 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index ab9e3b6747..35d98f2736 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,8 +24,8 @@ import iotests
import subprocess
import re
-iotests.script_initialize(supported_fmts=['raw'])
-iotests.verify_protocol(supported=['ssh'])
+iotests.script_initialize(supported_fmts=['raw'],
+ supported_protocols=['ssh'])
def filter_hash(qmsg):
def _filter(key, value):
diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index 5a7013cd34..d9fe780c07 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -23,8 +23,8 @@
import iotests
from iotests import imgfmt
-iotests.script_initialize(supported_fmts=['luks'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['luks'],
+ supported_protocols=['file'])
def blockdev_create(vm, options):
result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index 4d6aac497f..155fac4e87 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -23,8 +23,8 @@
import iotests
from iotests import imgfmt
-iotests.script_initialize(supported_fmts=['vdi'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['vdi'],
+ supported_protocols=['file'])
def blockdev_create(vm, options):
result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212
index ec35bceb11..67e5a1dbb5 100755
--- a/tests/qemu-iotests/212
+++ b/tests/qemu-iotests/212
@@ -23,8 +23,8 @@
import iotests
from iotests import imgfmt
-iotests.script_initialize(supported_fmts=['parallels'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['parallels'],
+ supported_protocols=['file'])
def blockdev_create(vm, options):
result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213
index 3d2c024375..23f387ab63 100755
--- a/tests/qemu-iotests/213
+++ b/tests/qemu-iotests/213
@@ -23,8 +23,8 @@
import iotests
from iotests import imgfmt
-iotests.script_initialize(supported_fmts=['vhdx'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['vhdx'],
+ supported_protocols=['file'])
def blockdev_create(vm, options):
result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 2970d7304a..661d7f93bf 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -925,7 +925,9 @@ def execute_unittest(debug=False):
def execute_setup_common(supported_fmts=[],
supported_oses=['linux'],
supported_cache_modes=[],
- unsupported_fmts=[]):
+ unsupported_fmts=[],
+ supported_protocols=[],
+ unsupported_protocols=[]):
"""
Perform necessary setup for either script-style or unittest-style tests.
"""
@@ -941,6 +943,7 @@ def execute_setup_common(supported_fmts=[],
verify_image_format(supported_fmts, unsupported_fmts)
verify_platform(supported_oses)
verify_cache_mode(supported_cache_modes)
+verify_protocol(supported_protocols, unsupported_protocols)
debug = '-d' in sys.argv
if debug:
--
2.21.0
Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/4] iotests: use python logging
Patchew URL: https://patchew.org/QEMU/20190820235243.26092-1-js...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH v3 0/4] iotests: use python logging Message-id: 20190820235243.26092-1-js...@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20190820235243.26092-1-js...@redhat.com -> patchew/20190820235243.26092-1-js...@redhat.com Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone' Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc' Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers' Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF' Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2' Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe' Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios' Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware' Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi' Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode' Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios' Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa' Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios' Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot' Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot' Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex' Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp' Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3' Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3' Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into 'capstone'... Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf' Cloning into 'dtc'... Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536' Cloning into 'roms/QemuMacDrivers'... Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266' Cloning into 'roms/SLOF'... Submodule path 'roms/SLOF': checked out 'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3' Cloning into 'roms/edk2'... Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54' Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3' Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl' Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'... Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037' Cloning into 'CryptoPkg/Library/OpensslLib/openssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687' Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl' Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5' Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography' Cloning into 'boringssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f' Cloning into 'krb5'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168' Cloning into 'pyca-cryptography'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f' Cloning into 'roms/ipxe'... Submodule path 'roms/ipxe': ch
