[PULL 13/15] block-backend: drop INT_MAX restriction from blk_check_byte_request()
From: Vladimir Sementsov-Ogievskiy
blk_check_bytes_request is called from blk_co_do_preadv,
blk_co_do_pwritev_part, blk_co_do_pdiscard and blk_co_copy_range
before (maybe) calling throttle_group_co_io_limits_intercept() (which
has int64_t argument) and then calling corresponding bdrv_co_ function.
bdrv_co_ functions are OK with int64_t bytes as well.
So dropping the check for INT_MAX we just get same restrictions as in
bdrv_ layer: discard and write-zeroes goes through
bdrv_check_qiov_request() and are allowed to be 64bit. Other requests
go through bdrv_check_request32() and still restricted by INT_MAX
boundary.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-13-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
Signed-off-by: Eric Blake
---
block/block-backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 59746eda45b0..39cd99df2b3c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1166,7 +1166,7 @@ static int blk_check_byte_request(BlockBackend *blk,
int64_t offset,
{
int64_t len;
-if (bytes < 0 || bytes > INT_MAX) {
+if (bytes < 0) {
return -EIO;
}
--
2.31.1
[PULL 10/15] block-backend: convert blk_co_copy_range to int64_t bytes
From: Vladimir Sementsov-Ogievskiy
Function is updated so that parameter type becomes wider, so all
callers should be OK with it.
Look at blk_co_copy_range() itself: bytes is passed only to
blk_check_byte_request() and bdrv_co_copy_range(), which already have
int64_t bytes parameter, so we are OK.
Note that requests exceeding INT_MAX are still restricted by
blk_check_byte_request().
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-10-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
[eblake: grammar tweaks]
Signed-off-by: Eric Blake
---
include/sysemu/block-backend.h | 2 +-
block/block-backend.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3294d1b07a33..9ccf9f1a8025 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -270,7 +270,7 @@ void blk_unregister_buf(BlockBackend *blk, void *host);
int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
BlockBackend *blk_out, int64_t off_out,
- int bytes, BdrvRequestFlags read_flags,
+ int64_t bytes, BdrvRequestFlags read_flags,
BdrvRequestFlags write_flags);
const BdrvChild *blk_root(BlockBackend *blk);
diff --git a/block/block-backend.c b/block/block-backend.c
index ee20ae5f0f28..0746be89842b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2418,7 +2418,7 @@ void blk_unregister_buf(BlockBackend *blk, void *host)
int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
BlockBackend *blk_out, int64_t off_out,
- int bytes, BdrvRequestFlags read_flags,
+ int64_t bytes, BdrvRequestFlags read_flags,
BdrvRequestFlags write_flags)
{
int r;
--
2.31.1
[PULL 08/15] block-backend: drop blk_prw, use block-coroutine-wrapper
From: Vladimir Sementsov-Ogievskiy
Let's drop hand-made coroutine wrappers and use coroutine wrapper
generation like in block/io.c.
Now, blk_foo() functions are written in same way as blk_co_foo() ones,
but wrap blk_do_foo() instead of blk_co_do_foo().
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-8-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
[eblake: spelling fix]
Signed-off-by: Eric Blake
---
block/coroutines.h| 30
block/block-backend.c | 155 --
2 files changed, 90 insertions(+), 95 deletions(-)
diff --git a/block/coroutines.h b/block/coroutines.h
index 35a6c4985782..c8c14a29c834 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -75,4 +75,34 @@ int coroutine_fn
nbd_co_do_establish_connection(BlockDriverState *bs, Error **errp);
+int generated_co_wrapper
+blk_do_preadv(BlockBackend *blk, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags);
+int coroutine_fn
+blk_co_do_preadv(BlockBackend *blk, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags);
+
+
+int generated_co_wrapper
+blk_do_pwritev_part(BlockBackend *blk, int64_t offset, int64_t bytes,
+QEMUIOVector *qiov, size_t qiov_offset,
+BdrvRequestFlags flags);
+int coroutine_fn
+blk_co_do_pwritev_part(BlockBackend *blk, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, size_t qiov_offset,
+ BdrvRequestFlags flags);
+
+int generated_co_wrapper
+blk_do_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
+int coroutine_fn
+blk_co_do_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
+
+int generated_co_wrapper
+blk_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
+int coroutine_fn
+blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
+
+int generated_co_wrapper blk_do_flush(BlockBackend *blk);
+int coroutine_fn blk_co_do_flush(BlockBackend *blk);
+
#endif /* BLOCK_COROUTINES_INT_H */
diff --git a/block/block-backend.c b/block/block-backend.c
index cecac6748593..2e6ccce7ef2d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -14,6 +14,7 @@
#include "sysemu/block-backend.h"
#include "block/block_int.h"
#include "block/blockjob.h"
+#include "block/coroutines.h"
#include "block/throttle-groups.h"
#include "hw/qdev-core.h"
#include "sysemu/blockdev.h"
@@ -1204,7 +1205,7 @@ static void coroutine_fn
blk_wait_while_drained(BlockBackend *blk)
}
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
-static int coroutine_fn
+int coroutine_fn
blk_co_do_preadv(BlockBackend *blk, int64_t offset, int64_t bytes,
QEMUIOVector *qiov, BdrvRequestFlags flags)
{
@@ -1249,7 +1250,7 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t
offset,
}
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
-static int coroutine_fn
+int coroutine_fn
blk_co_do_pwritev_part(BlockBackend *blk, int64_t offset, int64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset,
BdrvRequestFlags flags)
@@ -1306,6 +1307,20 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk,
int64_t offset,
return blk_co_pwritev_part(blk, offset, bytes, qiov, 0, flags);
}
+static int coroutine_fn blk_pwritev_part(BlockBackend *blk, int64_t offset,
+ int64_t bytes,
+ QEMUIOVector *qiov, size_t
qiov_offset,
+ BdrvRequestFlags flags)
+{
+int ret;
+
+blk_inc_in_flight(blk);
+ret = blk_do_pwritev_part(blk, offset, bytes, qiov, qiov_offset, flags);
+blk_dec_in_flight(blk);
+
+return ret;
+}
+
typedef struct BlkRwCo {
BlockBackend *blk;
int64_t offset;
@@ -1314,58 +1329,11 @@ typedef struct BlkRwCo {
BdrvRequestFlags flags;
} BlkRwCo;
-static void blk_read_entry(void *opaque)
-{
-BlkRwCo *rwco = opaque;
-QEMUIOVector *qiov = rwco->iobuf;
-
-rwco->ret = blk_co_do_preadv(rwco->blk, rwco->offset, qiov->size,
- qiov, rwco->flags);
-aio_wait_kick();
-}
-
-static void blk_write_entry(void *opaque)
-{
-BlkRwCo *rwco = opaque;
-QEMUIOVector *qiov = rwco->iobuf;
-
-rwco->ret = blk_co_do_pwritev_part(rwco->blk, rwco->offset, qiov->size,
- qiov, 0, rwco->flags);
-aio_wait_kick();
-}
-
-static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf,
- int64_t bytes, CoroutineEntry co_entry,
- BdrvRequestFlags flags)
-{
-QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
-BlkRwCo rwco = {
-.blk= blk,
-.offset = offset,
-.iobuf = ,
-.flags = flags,
-.ret= NOT_DONE,
-};
-
-blk_inc_in_flight(blk);
-if
[PULL 09/15] block-backend: convert blk_foo wrappers to use int64_t bytes parameter
From: Vladimir Sementsov-Ogievskiy
Convert blk_pdiscard, blk_pwrite_compressed, blk_pwrite_zeroes.
These are just wrappers for functions with int64_t argument, so allow
passing int64_t as well. Parameter type becomes wider so all callers
should be OK with it.
Note that requests exceeding INT_MAX are still restricted by
blk_check_byte_request().
Note also that we don't (and are not going to) convert blk_pwrite and
blk_pread: these functions return number of bytes on success, so to
update them, we should change return type to int64_t as well, which
will lead to investigating and updating all callers which is too much.
So, blk_pread and blk_pwrite remain unchanged.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-9-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
[eblake: grammar tweaks]
Signed-off-by: Eric Blake
---
include/sysemu/block-backend.h | 6 +++---
block/block-backend.c | 10 +-
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3efa0256395f..3294d1b07a33 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -155,7 +155,7 @@ static inline int coroutine_fn blk_co_pwrite(BlockBackend
*blk, int64_t offset,
}
int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
- int bytes, BdrvRequestFlags flags);
+ int64_t bytes, BdrvRequestFlags flags);
BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
int bytes, BdrvRequestFlags flags,
BlockCompletionFunc *cb, void *opaque);
@@ -246,10 +246,10 @@ void *blk_aio_get(const AIOCBInfo *aiocb_info,
BlockBackend *blk,
int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
int64_t bytes, BdrvRequestFlags flags);
int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
- int bytes);
+ int64_t bytes);
int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
PreallocMode prealloc, BdrvRequestFlags flags, Error **errp);
-int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
+int blk_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
int64_t pos, int size);
int blk_load_vmstate(BlockBackend *blk, uint8_t *buf, int64_t pos, int size);
diff --git a/block/block-backend.c b/block/block-backend.c
index 2e6ccce7ef2d..ee20ae5f0f28 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1330,7 +1330,7 @@ typedef struct BlkRwCo {
} BlkRwCo;
int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
- int bytes, BdrvRequestFlags flags)
+ int64_t bytes, BdrvRequestFlags flags)
{
return blk_pwritev_part(blk, offset, bytes, NULL, 0,
flags | BDRV_REQ_ZERO_WRITE);
@@ -1637,7 +1637,7 @@ int coroutine_fn blk_co_pdiscard(BlockBackend *blk,
int64_t offset,
return ret;
}
-int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
+int blk_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
{
int ret;
@@ -2186,10 +2186,10 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend
*blk, int64_t offset,
}
int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
- int count)
+ int64_t bytes)
{
-QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, count);
-return blk_pwritev_part(blk, offset, count, , 0,
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
+return blk_pwritev_part(blk, offset, bytes, , 0,
BDRV_REQ_WRITE_COMPRESSED);
}
--
2.31.1
[PULL 02/15] block-backend: blk_check_byte_request(): int64_t bytes
From: Vladimir Sementsov-Ogievskiy
Rename size and make it int64_t to correspond to modern block layer,
which always uses int64_t for offset and bytes (not in blk layer yet,
which is a task for following commits).
All callers pass int or unsigned int.
So, for bytes in [0, INT_MAX] nothing is changed, for negative bytes we
now fail on "bytes < 0" check instead of "bytes > INT_MAX" check.
Note, that blk_check_byte_request() still doesn't allow requests
exceeding INT_MAX.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-2-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
Signed-off-by: Eric Blake
---
block/block-backend.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index ba2b5ebb1008..2c62210687e7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1161,11 +1161,11 @@ void blk_set_disable_request_queuing(BlockBackend *blk,
bool disable)
}
static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
- size_t size)
+ int64_t bytes)
{
int64_t len;
-if (size > INT_MAX) {
+if (bytes < 0 || bytes > INT_MAX) {
return -EIO;
}
@@ -1183,7 +1183,7 @@ static int blk_check_byte_request(BlockBackend *blk,
int64_t offset,
return len;
}
-if (offset > len || len - offset < size) {
+if (offset > len || len - offset < bytes) {
return -EIO;
}
}
--
2.31.1
[PULL 04/15] block-backend: convert blk_co_pwritev_part to int64_t bytes
From: Vladimir Sementsov-Ogievskiy
We convert blk_do_pwritev_part() and some wrappers:
blk_co_pwritev_part(), blk_co_pwritev(), blk_co_pwrite_zeroes().
All functions are converted so that the parameter type becomes wider, so
all callers should be OK with it.
Look at blk_do_pwritev_part() body:
bytes is passed to:
- trace_blk_co_pwritev (we update it here)
- blk_check_byte_request, throttle_group_co_io_limits_intercept,
bdrv_co_pwritev_part - all already have int64_t argument.
Note that requests exceeding INT_MAX are still restricted by
blk_check_byte_request().
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-4-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
[eblake: grammar tweaks]
Signed-off-by: Eric Blake
---
include/sysemu/block-backend.h | 6 +++---
block/block-backend.c | 8
block/trace-events | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 442fd705bb78..91457a081ec1 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -129,11 +129,11 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t
offset,
int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
- unsigned int bytes,
+ int64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset,
BdrvRequestFlags flags);
int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
- unsigned int bytes, QEMUIOVector *qiov,
+ int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
static inline int coroutine_fn blk_co_pread(BlockBackend *blk, int64_t offset,
@@ -243,7 +243,7 @@ int blk_get_open_flags_from_root_state(BlockBackend *blk);
void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
BlockCompletionFunc *cb, void *opaque);
int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
- int bytes, BdrvRequestFlags flags);
+ int64_t bytes, BdrvRequestFlags flags);
int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
int bytes);
int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
diff --git a/block/block-backend.c b/block/block-backend.c
index 3199f84e96d9..105f0afff970 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1250,7 +1250,7 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t
offset,
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
static int coroutine_fn
-blk_do_pwritev_part(BlockBackend *blk, int64_t offset, unsigned int bytes,
+blk_do_pwritev_part(BlockBackend *blk, int64_t offset, int64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset,
BdrvRequestFlags flags)
{
@@ -1286,7 +1286,7 @@ blk_do_pwritev_part(BlockBackend *blk, int64_t offset,
unsigned int bytes,
}
int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
- unsigned int bytes,
+ int64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset,
BdrvRequestFlags flags)
{
@@ -1300,7 +1300,7 @@ int coroutine_fn blk_co_pwritev_part(BlockBackend *blk,
int64_t offset,
}
int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
-unsigned int bytes, QEMUIOVector *qiov,
+int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags)
{
return blk_co_pwritev_part(blk, offset, bytes, qiov, 0, flags);
@@ -2214,7 +2214,7 @@ void *blk_aio_get(const AIOCBInfo *aiocb_info,
BlockBackend *blk,
}
int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
- int bytes, BdrvRequestFlags flags)
+ int64_t bytes, BdrvRequestFlags flags)
{
return blk_co_pwritev(blk, offset, bytes, NULL,
flags | BDRV_REQ_ZERO_WRITE);
diff --git a/block/trace-events b/block/trace-events
index ff397485..ab56edacb4fc 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -6,7 +6,7 @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
# block-backend.c
blk_co_preadv(void *blk, void *bs, int64_t offset, int64_t bytes, int flags)
"blk %p bs %p offset %"PRId64" bytes %" PRId64 " flags 0x%x"
-blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int
flags)
[PULL 12/15] block-backend: blk_pread, blk_pwrite: rename count parameter to bytes
From: Vladimir Sementsov-Ogievskiy
To be consistent with declarations in include/sysemu/block-backend.h.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-12-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
Signed-off-by: Eric Blake
---
block/block-backend.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index c889d0f97c45..59746eda45b0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1477,27 +1477,27 @@ BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk,
int64_t offset,
flags | BDRV_REQ_ZERO_WRITE, cb, opaque);
}
-int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count)
+int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int bytes)
{
int ret;
-QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, count);
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
blk_inc_in_flight(blk);
-ret = blk_do_preadv(blk, offset, count, , 0);
+ret = blk_do_preadv(blk, offset, bytes, , 0);
blk_dec_in_flight(blk);
-return ret < 0 ? ret : count;
+return ret < 0 ? ret : bytes;
}
-int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count,
+int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int bytes,
BdrvRequestFlags flags)
{
int ret;
-QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, count);
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
-ret = blk_pwritev_part(blk, offset, count, , 0, flags);
+ret = blk_pwritev_part(blk, offset, bytes, , 0, flags);
-return ret < 0 ? ret : count;
+return ret < 0 ? ret : bytes;
}
int64_t blk_getlength(BlockBackend *blk)
--
2.31.1
[PULL 01/15] qcow2: Silence clang -m32 compiler warning
From: Hanna Reitz
With -m32, size_t is generally only a uint32_t. That makes clang
complain that in the assertion
assert(qiov->size <= INT64_MAX);
the range of the type of qiov->size (size_t) is too small for any of its
values to ever exceed INT64_MAX.
Cast qiov->size to uint64_t to silence clang.
Fixes: f7ef38dd1310d7d9db76d0aa16899cbc5744f36d
("block: use int64_t instead of uint64_t in driver read
handlers")
Signed-off-by: Hanna Reitz
Message-Id: <20211011155031.149158-1-hre...@redhat.com>
Reviewed-by: Eric Blake
Signed-off-by: Eric Blake
---
block/qcow2-cluster.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 5727f92dcb39..21884a1ab9ab 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -513,7 +513,8 @@ static int coroutine_fn
do_perform_cow_read(BlockDriverState *bs,
*/
assert(src_cluster_offset <= INT64_MAX);
assert(src_cluster_offset + offset_in_cluster <= INT64_MAX);
-assert(qiov->size <= INT64_MAX);
+/* Cast qiov->size to uint64_t to silence a compiler warning on -m32 */
+assert((uint64_t)qiov->size <= INT64_MAX);
bdrv_check_qiov_request(src_cluster_offset + offset_in_cluster, qiov->size,
qiov, 0, _abort);
/*
--
2.31.1
[PULL 07/15] block-coroutine-wrapper.py: support BlockBackend first argument
From: Vladimir Sementsov-Ogievskiy
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-7-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
Signed-off-by: Eric Blake
---
scripts/block-coroutine-wrapper.py | 12 ++--
block/coroutines.h | 3 +++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/scripts/block-coroutine-wrapper.py
b/scripts/block-coroutine-wrapper.py
index 85dbeb9ecf9c..08be813407b6 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -100,12 +100,20 @@ def snake_to_camel(func_name: str) -> str:
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
assert func.return_type == 'int'
-assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *']
+assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *',
+ 'BlockBackend *']
subsystem, subname = func.name.split('_', 1)
name = f'{subsystem}_co_{subname}'
-bs = 'bs' if func.args[0].type == 'BlockDriverState *' else 'child->bs'
+
+t = func.args[0].type
+if t == 'BlockDriverState *':
+bs = 'bs'
+elif t == 'BdrvChild *':
+bs = 'child->bs'
+else:
+bs = 'blk_bs(blk)'
struct_name = snake_to_camel(name)
return f"""\
diff --git a/block/coroutines.h b/block/coroutines.h
index 514d169d23d6..35a6c4985782 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -27,6 +27,9 @@
#include "block/block_int.h"
+/* For blk_bs() in generated block/block-gen.c */
+#include "sysemu/block-backend.h"
+
int coroutine_fn bdrv_co_check(BlockDriverState *bs,
BdrvCheckResult *res, BdrvCheckMode fix);
int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
--
2.31.1
[PULL 11/15] block-backend: convert blk_aio_ functions to int64_t bytes paramter
From: Vladimir Sementsov-Ogievskiy
1. Convert bytes in BlkAioEmAIOCB:
aio->bytes is only passed to already int64_t interfaces, and set in
blk_aio_prwv, which is updated here.
2. For all updated functions the parameter type becomes wider so callers
are safe.
3. In blk_aio_prwv we only store bytes to BlkAioEmAIOCB, which is
updated here.
4. Other updated functions are wrappers on blk_aio_prwv.
Note that blk_aio_preadv and blk_aio_pwritev become safer: before this
commit, it's theoretically possible to pass qiov with size exceeding
INT_MAX, which than converted to int argument of blk_aio_prwv. Now it's
converted to int64_t which is a lot better. Still add assertions.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-11-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
[eblake: tweak assertion and grammar]
Signed-off-by: Eric Blake
---
include/sysemu/block-backend.h | 4 ++--
block/block-backend.c | 13 -
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9ccf9f1a8025..b5409a6b4553 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -157,7 +157,7 @@ static inline int coroutine_fn blk_co_pwrite(BlockBackend
*blk, int64_t offset,
int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
int64_t bytes, BdrvRequestFlags flags);
BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
- int bytes, BdrvRequestFlags flags,
+ int64_t bytes, BdrvRequestFlags flags,
BlockCompletionFunc *cb, void *opaque);
int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags);
int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int bytes);
@@ -174,7 +174,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t
offset,
BlockCompletionFunc *cb, void *opaque);
BlockAIOCB *blk_aio_flush(BlockBackend *blk,
BlockCompletionFunc *cb, void *opaque);
-BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int bytes,
+BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
BlockCompletionFunc *cb, void *opaque);
void blk_aio_cancel(BlockAIOCB *acb);
void blk_aio_cancel_async(BlockAIOCB *acb);
diff --git a/block/block-backend.c b/block/block-backend.c
index 0746be89842b..c889d0f97c45 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1380,7 +1380,7 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
typedef struct BlkAioEmAIOCB {
BlockAIOCB common;
BlkRwCo rwco;
-int bytes;
+int64_t bytes;
bool has_returned;
} BlkAioEmAIOCB;
@@ -1412,7 +1412,8 @@ static void blk_aio_complete_bh(void *opaque)
blk_aio_complete(acb);
}
-static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
+static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset,
+int64_t bytes,
void *iobuf, CoroutineEntry co_entry,
BdrvRequestFlags flags,
BlockCompletionFunc *cb, void *opaque)
@@ -1469,10 +1470,10 @@ static void blk_aio_write_entry(void *opaque)
}
BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
- int count, BdrvRequestFlags flags,
+ int64_t bytes, BdrvRequestFlags flags,
BlockCompletionFunc *cb, void *opaque)
{
-return blk_aio_prwv(blk, offset, count, NULL, blk_aio_write_entry,
+return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_write_entry,
flags | BDRV_REQ_ZERO_WRITE, cb, opaque);
}
@@ -1530,6 +1531,7 @@ BlockAIOCB *blk_aio_preadv(BlockBackend *blk, int64_t
offset,
QEMUIOVector *qiov, BdrvRequestFlags flags,
BlockCompletionFunc *cb, void *opaque)
{
+assert((uint64_t)qiov->size <= INT64_MAX);
return blk_aio_prwv(blk, offset, qiov->size, qiov,
blk_aio_read_entry, flags, cb, opaque);
}
@@ -1538,6 +1540,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t
offset,
QEMUIOVector *qiov, BdrvRequestFlags flags,
BlockCompletionFunc *cb, void *opaque)
{
+assert(qiov->size <= INT64_MAX);
return blk_aio_prwv(blk, offset, qiov->size, qiov,
blk_aio_write_entry, flags, cb, opaque);
}
@@ -1618,7 +1621,7 @@ static void blk_aio_pdiscard_entry(void *opaque)
}
BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
- int64_t offset, int bytes,
+ int64_t offset, int64_t bytes,
BlockCompletionFunc
[PULL 06/15] block-backend: rename _do_ helper functions to _co_do_
From: Vladimir Sementsov-Ogievskiy
This is a preparation to the following commit, to use automatic
coroutine wrapper generation.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-6-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
Signed-off-by: Eric Blake
---
block/block-backend.c | 52 +--
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 1e21498b2c4a..cecac6748593 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1205,8 +1205,8 @@ static void coroutine_fn
blk_wait_while_drained(BlockBackend *blk)
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
static int coroutine_fn
-blk_do_preadv(BlockBackend *blk, int64_t offset, int64_t bytes,
- QEMUIOVector *qiov, BdrvRequestFlags flags)
+blk_co_do_preadv(BlockBackend *blk, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
int ret;
BlockDriverState *bs;
@@ -1242,7 +1242,7 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t
offset,
int ret;
blk_inc_in_flight(blk);
-ret = blk_do_preadv(blk, offset, bytes, qiov, flags);
+ret = blk_co_do_preadv(blk, offset, bytes, qiov, flags);
blk_dec_in_flight(blk);
return ret;
@@ -1250,9 +1250,9 @@ int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t
offset,
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
static int coroutine_fn
-blk_do_pwritev_part(BlockBackend *blk, int64_t offset, int64_t bytes,
-QEMUIOVector *qiov, size_t qiov_offset,
-BdrvRequestFlags flags)
+blk_co_do_pwritev_part(BlockBackend *blk, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, size_t qiov_offset,
+ BdrvRequestFlags flags)
{
int ret;
BlockDriverState *bs;
@@ -1293,7 +1293,7 @@ int coroutine_fn blk_co_pwritev_part(BlockBackend *blk,
int64_t offset,
int ret;
blk_inc_in_flight(blk);
-ret = blk_do_pwritev_part(blk, offset, bytes, qiov, qiov_offset, flags);
+ret = blk_co_do_pwritev_part(blk, offset, bytes, qiov, qiov_offset, flags);
blk_dec_in_flight(blk);
return ret;
@@ -1319,8 +1319,8 @@ static void blk_read_entry(void *opaque)
BlkRwCo *rwco = opaque;
QEMUIOVector *qiov = rwco->iobuf;
-rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, qiov->size,
- qiov, rwco->flags);
+rwco->ret = blk_co_do_preadv(rwco->blk, rwco->offset, qiov->size,
+ qiov, rwco->flags);
aio_wait_kick();
}
@@ -1329,8 +1329,8 @@ static void blk_write_entry(void *opaque)
BlkRwCo *rwco = opaque;
QEMUIOVector *qiov = rwco->iobuf;
-rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, qiov->size,
-qiov, 0, rwco->flags);
+rwco->ret = blk_co_do_pwritev_part(rwco->blk, rwco->offset, qiov->size,
+ qiov, 0, rwco->flags);
aio_wait_kick();
}
@@ -1483,8 +1483,8 @@ static void blk_aio_read_entry(void *opaque)
QEMUIOVector *qiov = rwco->iobuf;
assert(qiov->size == acb->bytes);
-rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, acb->bytes,
- qiov, rwco->flags);
+rwco->ret = blk_co_do_preadv(rwco->blk, rwco->offset, acb->bytes,
+ qiov, rwco->flags);
blk_aio_complete(acb);
}
@@ -1495,8 +1495,8 @@ static void blk_aio_write_entry(void *opaque)
QEMUIOVector *qiov = rwco->iobuf;
assert(!qiov || qiov->size == acb->bytes);
-rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, acb->bytes,
-qiov, 0, rwco->flags);
+rwco->ret = blk_co_do_pwritev_part(rwco->blk, rwco->offset, acb->bytes,
+ qiov, 0, rwco->flags);
blk_aio_complete(acb);
}
@@ -1583,7 +1583,7 @@ void blk_aio_cancel_async(BlockAIOCB *acb)
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
static int coroutine_fn
-blk_do_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
+blk_co_do_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
{
blk_wait_while_drained(blk);
@@ -1599,7 +1599,7 @@ static void blk_ioctl_entry(void *opaque)
BlkRwCo *rwco = opaque;
QEMUIOVector *qiov = rwco->iobuf;
-rwco->ret = blk_do_ioctl(rwco->blk, rwco->offset, qiov->iov[0].iov_base);
+rwco->ret = blk_co_do_ioctl(rwco->blk, rwco->offset,
qiov->iov[0].iov_base);
aio_wait_kick();
}
@@ -1613,7 +1613,7 @@ static void blk_aio_ioctl_entry(void *opaque)
BlkAioEmAIOCB *acb = opaque;
BlkRwCo *rwco = >rwco;
-rwco->ret = blk_do_ioctl(rwco->blk, rwco->offset, rwco->iobuf);
+rwco->ret = blk_co_do_ioctl(rwco->blk, rwco->offset, rwco->iobuf);
blk_aio_complete(acb);
}
@@ -1626,7
[PULL 03/15] block-backend: make blk_co_preadv() 64bit
From: Vladimir Sementsov-Ogievskiy
For both updated functions, the type of bytes becomes wider, so all callers
should be OK with it.
blk_co_preadv() only passes its arguments to blk_do_preadv().
blk_do_preadv() passes bytes to:
- trace_blk_co_preadv, which is updated too
- blk_check_byte_request, throttle_group_co_io_limits_intercept,
bdrv_co_preadv, which are already int64_t.
Note that requests exceeding INT_MAX are still restricted by
blk_check_byte_request().
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-3-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
[eblake: grammar tweaks]
Signed-off-by: Eric Blake
---
include/sysemu/block-backend.h | 2 +-
block/block-backend.c | 4 ++--
block/trace-events | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 82bae551617b..442fd705bb78 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -126,7 +126,7 @@ BlockBackend *blk_by_dev(void *dev);
BlockBackend *blk_by_qdev_id(const char *id, Error **errp);
void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
- unsigned int bytes, QEMUIOVector *qiov,
+ int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
unsigned int bytes,
diff --git a/block/block-backend.c b/block/block-backend.c
index 2c62210687e7..3199f84e96d9 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1205,7 +1205,7 @@ static void coroutine_fn
blk_wait_while_drained(BlockBackend *blk)
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
static int coroutine_fn
-blk_do_preadv(BlockBackend *blk, int64_t offset, unsigned int bytes,
+blk_do_preadv(BlockBackend *blk, int64_t offset, int64_t bytes,
QEMUIOVector *qiov, BdrvRequestFlags flags)
{
int ret;
@@ -1236,7 +1236,7 @@ blk_do_preadv(BlockBackend *blk, int64_t offset, unsigned
int bytes,
}
int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
- unsigned int bytes, QEMUIOVector *qiov,
+ int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags)
{
int ret;
diff --git a/block/trace-events b/block/trace-events
index f2d0a9b62a7e..ff397485 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -5,7 +5,7 @@ bdrv_open_common(void *bs, const char *filename, int flags,
const char *format_n
bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
# block-backend.c
-blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int
flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x"
+blk_co_preadv(void *blk, void *bs, int64_t offset, int64_t bytes, int flags)
"blk %p bs %p offset %"PRId64" bytes %" PRId64 " flags 0x%x"
blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int
flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x"
blk_root_attach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
--
2.31.1
[PULL 05/15] block-backend: convert blk_co_pdiscard to int64_t bytes
From: Vladimir Sementsov-Ogievskiy
We updated blk_do_pdiscard() and its wrapper blk_co_pdiscard(). Both
functions are updated so that the parameter type becomes wider, so all
callers should be OK with it.
Look at blk_do_pdiscard(): bytes is passed only to
blk_check_byte_request() and bdrv_co_pdiscard(), which already have
int64_t bytes parameter, so we are OK.
Note that requests exceeding INT_MAX are still restricted by
blk_check_byte_request().
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20211006131718.214235-5-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake
[eblake: grammar tweaks]
Signed-off-by: Eric Blake
---
include/sysemu/block-backend.h | 3 ++-
block/block-backend.c | 5 +++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 91457a081ec1..3efa0256395f 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -181,7 +181,8 @@ void blk_aio_cancel_async(BlockAIOCB *acb);
int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
BlockCompletionFunc *cb, void *opaque);
-int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
+int coroutine_fn blk_co_pdiscard(BlockBackend *blk, int64_t offset,
+ int64_t bytes);
int blk_co_flush(BlockBackend *blk);
int blk_flush(BlockBackend *blk);
int blk_commit_all(void);
diff --git a/block/block-backend.c b/block/block-backend.c
index 105f0afff970..1e21498b2c4a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1626,7 +1626,7 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned
long int req, void *buf,
/* To be called between exactly one pair of blk_inc/dec_in_flight() */
static int coroutine_fn
-blk_do_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
+blk_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
{
int ret;
@@ -1657,7 +1657,8 @@ BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
cb, opaque);
}
-int coroutine_fn blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes)
+int coroutine_fn blk_co_pdiscard(BlockBackend *blk, int64_t offset,
+ int64_t bytes)
{
int ret;
--
2.31.1
Re: [PULL 37/40] monitor: Tidy up find_device_state()
On 10/15/21 4:08 AM, Christian Borntraeger wrote:
Am 13.10.21 um 11:07 schrieb Paolo Bonzini:
From: Markus Armbruster
Commit 6287d827d4 "monitor: allow device_del to accept QOM paths"
extended find_device_state() to accept QOM paths in addition to qdev
IDs. This added a checked conversion to TYPE_DEVICE at the end, which
duplicates the check done for the qdev ID case earlier, except it sets
a *different* error: GenericError "ID is not a hotpluggable device"
when passed a QOM path, and DeviceNotFound "Device 'ID' not found"
when passed a qdev ID. Fortunately, the latter won't happen as long
as we add only devices to /machine/peripheral/.
Earlier, commit b6cc36abb2 "qdev: device_del: Search for to be
unplugged device in 'peripheral' container" rewrote the lookup by qdev
ID to use QOM instead of qdev_find_recursive(), so it can handle
buss-less devices. It does so by constructing an absolute QOM path.
Works, but object_resolve_path_component() is easier. Switching to it
also gets rid of the unclean duplication described above.
While there, avoid converting to TYPE_DEVICE twice, first to check
whether it's possible, and then for real.
This one broke qemu iotest 280 on s390:
280 fail [13:06:19] [13:06:19] 0.3s (last: 0.3s) output mismatch (see
280.out.bad)
--- /home/cborntra/REPOS/qemu/tests/qemu-iotests/280.out
+++ 280.out.bad
@@ -37,14 +37,14 @@
=== Resume the VM and simulate a write request ===
{"execute": "cont", "arguments": {}}
{"return": {}}
-{"return": ""}
+{"return": "Error: Device 'vda/virtio-backend' not found\r\n"}
Hmm, this test doesn't seem to have been attempted during staging:
https://gitlab.com/qemu-project/qemu/-/jobs/1681194907
Is there something extra that needs to be installed on s390x.ci.qemu.org to have this test
run?
r~
Re: [PATCH 04/15] pcie: Add callback preceding SR-IOV VFs update
On Fri, Oct 15, 2021 at 06:24:14PM +0200, Lukasz Maniak wrote:
> On Wed, Oct 13, 2021 at 05:10:35AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Oct 12, 2021 at 06:06:46PM +0200, Lukasz Maniak wrote:
> > > On Tue, Oct 12, 2021 at 03:25:12AM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, Oct 07, 2021 at 06:23:55PM +0200, Lukasz Maniak wrote:
> > > > > PCIe devices implementing SR-IOV may need to perform certain actions
> > > > > before the VFs are unrealized or vice versa.
> > > > >
> > > > > Signed-off-by: Lukasz Maniak
> > > >
> > > > Callbacks are annoying and easy to misuse though.
> > > > VFs are enabled through a config cycle, we generally just
> > > > have devices invoke the capability handler.
> > > > E.g.
> > > >
> > > > static void pci_bridge_dev_write_config(PCIDevice *d,
> > > > uint32_t address, uint32_t val,
> > > > int len)
> > > > {
> > > > pci_bridge_write_config(d, address, val, len);
> > > > if (msi_present(d)) {
> > > > msi_write_config(d, address, val, len);
> > > > }
> > > > }
> > > >
> > > > this makes it easy to do whatever you want before/after
> > > > the write. You can also add a helper to check
> > > > that SRIOV is being enabled/disabled if necessary.
> > > >
> > > > > ---
> > > > > docs/pcie_sriov.txt | 2 +-
> > > > > hw/pci/pcie_sriov.c | 14 +-
> > > > > include/hw/pci/pcie_sriov.h | 8 +++-
> > > > > 3 files changed, 21 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/docs/pcie_sriov.txt b/docs/pcie_sriov.txt
> > > > > index f5e891e1d4..63ca1a7b8e 100644
> > > > > --- a/docs/pcie_sriov.txt
> > > > > +++ b/docs/pcie_sriov.txt
> > > > > @@ -57,7 +57,7 @@ setting up a BAR for a VF.
> > > > >/* Add and initialize the SR/IOV capability */
> > > > >pcie_sriov_pf_init(d, 0x200, "your_virtual_dev",
> > > > > vf_devid, initial_vfs, total_vfs,
> > > > > - fun_offset, stride);
> > > > > + fun_offset, stride, pre_vfs_update_cb);
> > > > >
> > > > >/* Set up individual VF BARs (parameters as for normal BARs) */
> > > > >pcie_sriov_pf_init_vf_bar( ... )
> > > > > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> > > > > index 501a1ff433..cac2aee061 100644
> > > > > --- a/hw/pci/pcie_sriov.c
> > > > > +++ b/hw/pci/pcie_sriov.c
> > > > > @@ -30,7 +30,8 @@ static void unregister_vfs(PCIDevice *dev);
> > > > > void pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
> > > > > const char *vfname, uint16_t vf_dev_id,
> > > > > uint16_t init_vfs, uint16_t total_vfs,
> > > > > -uint16_t vf_offset, uint16_t vf_stride)
> > > > > +uint16_t vf_offset, uint16_t vf_stride,
> > > > > +SriovVfsUpdate pre_vfs_update)
> > > > > {
> > > > > uint8_t *cfg = dev->config + offset;
> > > > > uint8_t *wmask;
> > > > > @@ -41,6 +42,7 @@ void pcie_sriov_pf_init(PCIDevice *dev, uint16_t
> > > > > offset,
> > > > > dev->exp.sriov_pf.num_vfs = 0;
> > > > > dev->exp.sriov_pf.vfname = g_strdup(vfname);
> > > > > dev->exp.sriov_pf.vf = NULL;
> > > > > +dev->exp.sriov_pf.pre_vfs_update = pre_vfs_update;
> > > > >
> > > > > pci_set_word(cfg + PCI_SRIOV_VF_OFFSET, vf_offset);
> > > > > pci_set_word(cfg + PCI_SRIOV_VF_STRIDE, vf_stride);
> > > > > @@ -180,6 +182,11 @@ static void register_vfs(PCIDevice *dev)
> > > > > assert(dev->exp.sriov_pf.vf);
> > > > >
> > > > > trace_sriov_register_vfs(SRIOV_ID(dev), num_vfs);
> > > > > +
> > > > > +if (dev->exp.sriov_pf.pre_vfs_update) {
> > > > > +dev->exp.sriov_pf.pre_vfs_update(dev,
> > > > > dev->exp.sriov_pf.num_vfs, num_vfs);
> > > > > +}
> > > > > +
> > > > > for (i = 0; i < num_vfs; i++) {
> > > > > dev->exp.sriov_pf.vf[i] = register_vf(dev, devfn,
> > > > > dev->exp.sriov_pf.vfname, i);
> > > > > if (!dev->exp.sriov_pf.vf[i]) {
> > > > > @@ -198,6 +205,11 @@ static void unregister_vfs(PCIDevice *dev)
> > > > > uint16_t i;
> > > > >
> > > > > trace_sriov_unregister_vfs(SRIOV_ID(dev), num_vfs);
> > > > > +
> > > > > +if (dev->exp.sriov_pf.pre_vfs_update) {
> > > > > +dev->exp.sriov_pf.pre_vfs_update(dev,
> > > > > dev->exp.sriov_pf.num_vfs, 0);
> > > > > +}
> > > > > +
> > > > > for (i = 0; i < num_vfs; i++) {
> > > > > PCIDevice *vf = dev->exp.sriov_pf.vf[i];
> > > > > object_property_set_bool(OBJECT(vf), "realized", false,
> > > > > _err);
> > > > > diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
> > > > > index 0974f00054..9ab48b79c0 100644
> > > > > --- a/include/hw/pci/pcie_sriov.h
> > > > > +++ b/include/hw/pci/pcie_sriov.h
> > > > > @@ -13,11 +13,16 @@
> > > > > #ifndef QEMU_PCIE_SRIOV_H
> > > > > #define QEMU_PCIE_SRIOV_H
> > > > >
> > > > > +typedef
Re: [PATCH 04/15] pcie: Add callback preceding SR-IOV VFs update
On Wed, Oct 13, 2021 at 05:10:35AM -0400, Michael S. Tsirkin wrote:
> On Tue, Oct 12, 2021 at 06:06:46PM +0200, Lukasz Maniak wrote:
> > On Tue, Oct 12, 2021 at 03:25:12AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Oct 07, 2021 at 06:23:55PM +0200, Lukasz Maniak wrote:
> > > > PCIe devices implementing SR-IOV may need to perform certain actions
> > > > before the VFs are unrealized or vice versa.
> > > >
> > > > Signed-off-by: Lukasz Maniak
> > >
> > > Callbacks are annoying and easy to misuse though.
> > > VFs are enabled through a config cycle, we generally just
> > > have devices invoke the capability handler.
> > > E.g.
> > >
> > > static void pci_bridge_dev_write_config(PCIDevice *d,
> > > uint32_t address, uint32_t val,
> > > int len)
> > > {
> > > pci_bridge_write_config(d, address, val, len);
> > > if (msi_present(d)) {
> > > msi_write_config(d, address, val, len);
> > > }
> > > }
> > >
> > > this makes it easy to do whatever you want before/after
> > > the write. You can also add a helper to check
> > > that SRIOV is being enabled/disabled if necessary.
> > >
> > > > ---
> > > > docs/pcie_sriov.txt | 2 +-
> > > > hw/pci/pcie_sriov.c | 14 +-
> > > > include/hw/pci/pcie_sriov.h | 8 +++-
> > > > 3 files changed, 21 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/docs/pcie_sriov.txt b/docs/pcie_sriov.txt
> > > > index f5e891e1d4..63ca1a7b8e 100644
> > > > --- a/docs/pcie_sriov.txt
> > > > +++ b/docs/pcie_sriov.txt
> > > > @@ -57,7 +57,7 @@ setting up a BAR for a VF.
> > > >/* Add and initialize the SR/IOV capability */
> > > >pcie_sriov_pf_init(d, 0x200, "your_virtual_dev",
> > > > vf_devid, initial_vfs, total_vfs,
> > > > - fun_offset, stride);
> > > > + fun_offset, stride, pre_vfs_update_cb);
> > > >
> > > >/* Set up individual VF BARs (parameters as for normal BARs) */
> > > >pcie_sriov_pf_init_vf_bar( ... )
> > > > diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> > > > index 501a1ff433..cac2aee061 100644
> > > > --- a/hw/pci/pcie_sriov.c
> > > > +++ b/hw/pci/pcie_sriov.c
> > > > @@ -30,7 +30,8 @@ static void unregister_vfs(PCIDevice *dev);
> > > > void pcie_sriov_pf_init(PCIDevice *dev, uint16_t offset,
> > > > const char *vfname, uint16_t vf_dev_id,
> > > > uint16_t init_vfs, uint16_t total_vfs,
> > > > -uint16_t vf_offset, uint16_t vf_stride)
> > > > +uint16_t vf_offset, uint16_t vf_stride,
> > > > +SriovVfsUpdate pre_vfs_update)
> > > > {
> > > > uint8_t *cfg = dev->config + offset;
> > > > uint8_t *wmask;
> > > > @@ -41,6 +42,7 @@ void pcie_sriov_pf_init(PCIDevice *dev, uint16_t
> > > > offset,
> > > > dev->exp.sriov_pf.num_vfs = 0;
> > > > dev->exp.sriov_pf.vfname = g_strdup(vfname);
> > > > dev->exp.sriov_pf.vf = NULL;
> > > > +dev->exp.sriov_pf.pre_vfs_update = pre_vfs_update;
> > > >
> > > > pci_set_word(cfg + PCI_SRIOV_VF_OFFSET, vf_offset);
> > > > pci_set_word(cfg + PCI_SRIOV_VF_STRIDE, vf_stride);
> > > > @@ -180,6 +182,11 @@ static void register_vfs(PCIDevice *dev)
> > > > assert(dev->exp.sriov_pf.vf);
> > > >
> > > > trace_sriov_register_vfs(SRIOV_ID(dev), num_vfs);
> > > > +
> > > > +if (dev->exp.sriov_pf.pre_vfs_update) {
> > > > +dev->exp.sriov_pf.pre_vfs_update(dev,
> > > > dev->exp.sriov_pf.num_vfs, num_vfs);
> > > > +}
> > > > +
> > > > for (i = 0; i < num_vfs; i++) {
> > > > dev->exp.sriov_pf.vf[i] = register_vf(dev, devfn,
> > > > dev->exp.sriov_pf.vfname, i);
> > > > if (!dev->exp.sriov_pf.vf[i]) {
> > > > @@ -198,6 +205,11 @@ static void unregister_vfs(PCIDevice *dev)
> > > > uint16_t i;
> > > >
> > > > trace_sriov_unregister_vfs(SRIOV_ID(dev), num_vfs);
> > > > +
> > > > +if (dev->exp.sriov_pf.pre_vfs_update) {
> > > > +dev->exp.sriov_pf.pre_vfs_update(dev,
> > > > dev->exp.sriov_pf.num_vfs, 0);
> > > > +}
> > > > +
> > > > for (i = 0; i < num_vfs; i++) {
> > > > PCIDevice *vf = dev->exp.sriov_pf.vf[i];
> > > > object_property_set_bool(OBJECT(vf), "realized", false,
> > > > _err);
> > > > diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
> > > > index 0974f00054..9ab48b79c0 100644
> > > > --- a/include/hw/pci/pcie_sriov.h
> > > > +++ b/include/hw/pci/pcie_sriov.h
> > > > @@ -13,11 +13,16 @@
> > > > #ifndef QEMU_PCIE_SRIOV_H
> > > > #define QEMU_PCIE_SRIOV_H
> > > >
> > > > +typedef void (*SriovVfsUpdate)(PCIDevice *dev, uint16_t prev_num_vfs,
> > > > + uint16_t num_vfs);
> > > > +
> > > > struct PCIESriovPF {
> > > > uint16_t num_vfs; /* Number of virtual functions created
> > > > */
> > > > uint8_t
Re: iotest 030 SIGSEGV
15.10.2021 12:38, Paolo Bonzini wrote: On 14/10/21 18:14, Vladimir Sementsov-Ogievskiy wrote: iotest 30 failing is a long story.. And as I understand the main source of all these crashes is that we do diffreent graph modifications simultaneously from parallel block jobs. In past I sent RFC series with global mutext, to fix a subset of the problem: https://patchew.org/QEMU/20201120161622.1537-1-vsement...@virtuozzo.com/ [just look at patch 5: https://patchew.org/QEMU/20201120161622.1537-1-vsement...@virtuozzo.com/20201120161622.1537-6-vsement...@virtuozzo.com/] Can you explain the way they interleave, and where the job callbacks are yielding in the middle of graph manipulations? Not exactly, and I don't think it worth to recover this concrete old problem about permissions: too much changes since it were made, especially in block permission update system. So, I can only refer to my old comments on it: OK, after some debugging and looking at block-graph dumps I tend to think that this a race between finish (.prepare) of mirror and block-stream. They do graph updates. Nothing prevents interleaving of graph-updating operations (note that bdrv_replace_child_noperm may do aio_poll). And nothing protects two processes of graph-update from intersection. and aio_poll at start of mirror_exit_common is my addition. But anyway the problem is here: we do call mirror_prepare inside of stream_prepare! (https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg05181.html) At this link there is a good core dump, where we are in stream_prepare, we do bdrv_change_backing_file() which finally call bdrv_pwritev(), which lead to aio_poll, during which we switch to mirror_prepare(). and 2. The core problem is that graph modification functions may trigger context-switch due to nested aio_polls.. which leads to (for example) nested permission updates.. (https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg05212.html) The problem with a CoMutex is that plenty of graph manipulations happen outside coroutines, and if coroutines such as stream_co_clean yield the monitor can do graph manipulations of its own. So if the solution could be "no yielding in the middle of graph manipulations", that would be much better. In fact, maybe the coroutine API should have a way to assert "no-yield regions" (similar to how Linux croaks if you call a sleeping function while preemption is disabled). More assertions = more bugs found early. Not sure that it's possible to fix bdrv_change_backing_file() in this way.. If some graph modifications connected with updating metadata in images, we want to write data and therefore - to yield. And the whole operation, both updating metadata in the image and updating the graph should be protected from interleaving with another graph-modifying operation. Not sure was it good enough to try to recover it. I didn't look close at Emanuele's "block layer: split block APIs in global state and I/O". Wasn't there something on protecting graph operations? In his series, graph operations are supposed to operate from the main thread (which they do) but he didn't cover the case of coroutines that yield. Paolo Maybe, it's possible to develop a critical section, a kind of mutex, that can be used both inside and outside of coroutines? So that coroutine will yield until it can acquire the mutex, non-coroutine will poll. -- Best regards, Vladimir
Re: [PATCH v2 00/15] qdev: Add JSON -device
Am 08.10.2021 um 15:34 hat Kevin Wolf geschrieben: > It's still a long way until we'll have QAPIfied devices, but there are > some improvements that we can already make now to make the future switch > easier. > > One important part of this is having code paths without QemuOpts, which > we want to get rid of and replace with the keyval parser in the long > run. This series adds support for JSON syntax to -device, which bypasses > QemuOpts. > > While we're not using QAPI yet, devices are based on QOM, so we already > do have type checks and an implied schema. JSON syntax supported now can > be supported by QAPI later and regarding command line compatibility, > actually switching to it becomes an implementation detail this way (of > course, it will still add valuable user-visible features like > introspection and documentation). > > Apart from making things more future proof, this also immediately adds > a way to do non-scalar properties on the command line. nvme could have > used list support recently, and the lack of it in -device led to some > rather unnatural solution in the first version (doing the relationship > between a device and objects backwards) and loss of features in the > following. With this series, using a list as a device property should be > possible without any weird tricks. > > Unfortunately, even QMP device_add goes through QemuOpts before this > series, which destroys any type safety QOM provides and also can't > support non-scalar properties. This is a bug, but it turns out that > libvirt actually relies on it and passes only strings for everything. > So this series still leaves device_add alone until libvirt is fixed. Thanks for the review and testing, applied to my tree. Kevin
Re: [PATCH 4/4] configure: remove dead EXESUF variable
On 10/15/21 15:36, Paolo Bonzini wrote: > On 15/10/21 14:38, Philippe Mathieu-Daudé wrote: >> On 10/15/21 12:07, Paolo Bonzini wrote: >>> Signed-off-by: Paolo Bonzini >>> --- >>> .gitlab-ci.d/crossbuild-template.yml | 2 +- >>> configure | 3 --- >>> 2 files changed, 1 insertion(+), 4 deletions(-) >> >> Maybe squash in patch #2 or place as #3 mentioning >> "the previous commit"? > > Anywhere you place it it's wrong. :) Squashing in #2 I dislike because > of the functional change in .gilab-ci.d (in truth, the variable is > *almost* dead!). Having the change as #4 makes it survive one patch > longer than it should, on the other hand having it as #3 separates > similar changes to "check-block.sh". Indeed. > What I will actually do in the final submission is not include the TAP > patch and submit it separately. Then for this one I can indeed add that > it was used only in the implementation of check-block. OK then, Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 4/4] configure: remove dead EXESUF variable
On 15/10/21 14:38, Philippe Mathieu-Daudé wrote: On 10/15/21 12:07, Paolo Bonzini wrote: Signed-off-by: Paolo Bonzini --- .gitlab-ci.d/crossbuild-template.yml | 2 +- configure| 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) Maybe squash in patch #2 or place as #3 mentioning "the previous commit"? Anywhere you place it it's wrong. :) Squashing in #2 I dislike because of the functional change in .gilab-ci.d (in truth, the variable is *almost* dead!). Having the change as #4 makes it survive one patch longer than it should, on the other hand having it as #3 separates similar changes to "check-block.sh". What I will actually do in the final submission is not include the TAP patch and submit it separately. Then for this one I can indeed add that it was used only in the implementation of check-block. Paolo
Re: [PATCH v2 00/15] qdev: Add JSON -device
On Fri, Oct 08, 2021 at 15:34:27 +0200, Kevin Wolf wrote: > It's still a long way until we'll have QAPIfied devices, but there are > some improvements that we can already make now to make the future switch > easier. > > One important part of this is having code paths without QemuOpts, which > we want to get rid of and replace with the keyval parser in the long > run. This series adds support for JSON syntax to -device, which bypasses > QemuOpts. > > While we're not using QAPI yet, devices are based on QOM, so we already > do have type checks and an implied schema. JSON syntax supported now can > be supported by QAPI later and regarding command line compatibility, > actually switching to it becomes an implementation detail this way (of > course, it will still add valuable user-visible features like > introspection and documentation). > > Apart from making things more future proof, this also immediately adds > a way to do non-scalar properties on the command line. nvme could have > used list support recently, and the lack of it in -device led to some > rather unnatural solution in the first version (doing the relationship > between a device and objects backwards) and loss of features in the > following. With this series, using a list as a device property should be > possible without any weird tricks. > > Unfortunately, even QMP device_add goes through QemuOpts before this > series, which destroys any type safety QOM provides and also can't > support non-scalar properties. This is a bug, but it turns out that > libvirt actually relies on it and passes only strings for everything. > So this series still leaves device_add alone until libvirt is fixed. I've tested hotplug and standard -device with many (not all) devices libvirt uses and seems that both '-device JSON' and monitor work properly. I'll enable '-device JSON' in libvirt once this hits upstream. > > v2: > - Drop type safe QMP device_add because libvirt gets it wrong, too I've actually applied this patch too to make sure that libvirt is working properly in both modes. It works now well with this too. I'm not sure about the compatibility promise here, but libvirt is now prepared even for teh strict version of 'device_add'. Libvirt's requirements state that new libvirt needs to be used with new qemu and thus from our view it's okay to do it even now. > - More network patches to eliminate dependencies on the legacy QemuOpts > data structures which would not contain all devices any more after > this series. Fix some bugs there as a side effect. > - Remove an unnecessary use of ERRP_GUARD() > - Replaced error handling patch for qdev_set_id() with Damien's > - Drop the deprecation patch until libvirt uses the new JSON syntax Series: Tested-by: Peter Krempa
Re: [PATCH 4/4] configure: remove dead EXESUF variable
On 10/15/21 12:07, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini > --- > .gitlab-ci.d/crossbuild-template.yml | 2 +- > configure| 3 --- > 2 files changed, 1 insertion(+), 4 deletions(-) Maybe squash in patch #2 or place as #3 mentioning "the previous commit"?
Re: Storage controller access to data
Am 12.10.2021 um 19:52 hat Nada Lachtar geschrieben: > I’m working on a project that requires me to read the data being sent > to storage and it to a file for analysis. To be more specific, I’m > trying to analyze the data in the phase of being written to the > storage disk, thus, I’m trying to read the data when it’s going > through a storage controller on x86 device. I’ve been looking into the > implementation of different storage controllers, but I need help to > pinpoint where I can read such data. Did you consider getting the data not from the frontend (i.e. the implementation of the storage controller emulation), but from the backend (i.e. the -blockdev configuration)? For example, there is the blklogwrites block driver that creates a log file that contains all the write requests that were made. Maybe this provides already what you need. If you need more flexibility than this, you could use an NBD connection as the backend and have a custom NBD server that can process the data in whatever way you need. Kevin
Re: [PULL 37/40] monitor: Tidy up find_device_state()
Am 13.10.21 um 11:07 schrieb Paolo Bonzini:
From: Markus Armbruster
Commit 6287d827d4 "monitor: allow device_del to accept QOM paths"
extended find_device_state() to accept QOM paths in addition to qdev
IDs. This added a checked conversion to TYPE_DEVICE at the end, which
duplicates the check done for the qdev ID case earlier, except it sets
a *different* error: GenericError "ID is not a hotpluggable device"
when passed a QOM path, and DeviceNotFound "Device 'ID' not found"
when passed a qdev ID. Fortunately, the latter won't happen as long
as we add only devices to /machine/peripheral/.
Earlier, commit b6cc36abb2 "qdev: device_del: Search for to be
unplugged device in 'peripheral' container" rewrote the lookup by qdev
ID to use QOM instead of qdev_find_recursive(), so it can handle
buss-less devices. It does so by constructing an absolute QOM path.
Works, but object_resolve_path_component() is easier. Switching to it
also gets rid of the unclean duplication described above.
While there, avoid converting to TYPE_DEVICE twice, first to check
whether it's possible, and then for real.
This one broke qemu iotest 280 on s390:
280 fail [13:06:19] [13:06:19] 0.3s (last: 0.3s) output mismatch
(see 280.out.bad)
--- /home/cborntra/REPOS/qemu/tests/qemu-iotests/280.out
+++ 280.out.bad
@@ -37,14 +37,14 @@
=== Resume the VM and simulate a write request ===
{"execute": "cont", "arguments": {}}
{"return": {}}
-{"return": ""}
+{"return": "Error: Device 'vda/virtio-backend' not found\r\n"}
=== Commit it to the backing file ===
{"execute": "block-commit", "arguments": {"auto-dismiss": false, "device": "top-fmt", "job-id":
"job0", "top-node": "top-fmt"}}
{"return": {}}
{"execute": "job-complete", "arguments": {"id": "job0"}}
{"return": {}}
-{"data": {"device": "job0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "job0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED",
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
Failures: 280
Failed 1 of 1 iotests
Signed-off-by: Markus Armbruster
Reviewed-by: Damien Hedde
Reviewed-by: Daniel P. Berrangé
Message-Id: <20210916111707.84999-1-arm...@redhat.com>
Signed-off-by: Paolo Bonzini
---
softmmu/qdev-monitor.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0705f00846..3df99ce9fc 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -836,16 +836,12 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
Error **errp)
static DeviceState *find_device_state(const char *id, Error **errp)
{
Object *obj;
+DeviceState *dev;
if (id[0] == '/') {
obj = object_resolve_path(id, NULL);
} else {
-char *root_path = object_get_canonical_path(qdev_get_peripheral());
-char *path = g_strdup_printf("%s/%s", root_path, id);
-
-g_free(root_path);
-obj = object_resolve_path_type(path, TYPE_DEVICE, NULL);
-g_free(path);
+obj = object_resolve_path_component(qdev_get_peripheral(), id);
}
if (!obj) {
@@ -854,12 +850,13 @@ static DeviceState *find_device_state(const char *id,
Error **errp)
return NULL;
}
-if (!object_dynamic_cast(obj, TYPE_DEVICE)) {
+dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
+if (!dev) {
error_setg(errp, "%s is not a hotpluggable device", id);
return NULL;
}
-return DEVICE(obj);
+return dev;
}
void qdev_unplug(DeviceState *dev, Error **errp)
[PATCH 4/4] configure: remove dead EXESUF variable
Signed-off-by: Paolo Bonzini
---
.gitlab-ci.d/crossbuild-template.yml | 2 +-
configure| 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/.gitlab-ci.d/crossbuild-template.yml
b/.gitlab-ci.d/crossbuild-template.yml
index 10d22dcf6c..87cb8b7518 100644
--- a/.gitlab-ci.d/crossbuild-template.yml
+++ b/.gitlab-ci.d/crossbuild-template.yml
@@ -12,7 +12,7 @@
mips64-softmmu ppc-softmmu riscv32-softmmu sh4-softmmu
sparc-softmmu xtensa-softmmu $CROSS_SKIP_TARGETS"
- make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS
-- if grep -q "EXESUF=.exe" config-host.mak;
+- if test -f qemu-io.exe;
then make installer;
version="$(git describe --match v[0-9]*)";
mv -v qemu-setup*.exe qemu-setup-${version}.exe;
diff --git a/configure b/configure
index 039467c04b..a4489bc23f 100755
--- a/configure
+++ b/configure
@@ -308,7 +308,6 @@ fortify_source="$default_feature"
strip_opt="yes"
mingw32="no"
gcov="no"
-EXESUF=""
modules="no"
module_upgrades="no"
prefix="/usr/local"
@@ -713,7 +712,6 @@ else
fi
if test "$mingw32" = "yes" ; then
- EXESUF=".exe"
# MinGW needs -mthreads for TLS and macro _MT.
CONFIGURE_CFLAGS="-mthreads $CONFIGURE_CFLAGS"
write_c_skeleton;
@@ -3814,7 +3812,6 @@ echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
echo "GLIB_LIBS=$glib_libs" >> $config_host_mak
echo "QEMU_LDFLAGS=$QEMU_LDFLAGS" >> $config_host_mak
echo "LD_I386_EMULATION=$ld_i386_emulation" >> $config_host_mak
-echo "EXESUF=$EXESUF" >> $config_host_mak
echo "LIBS_QGA=$libs_qga" >> $config_host_mak
if test "$rng_none" = "yes"; then
--
2.31.1
[PATCH 3/4] check-block: replace -makecheck with TAP output
Let "meson test" take care of showing the results of the individual tests,
consistently with other output from "make check V=1". Use TAP comments
so that the environment is included in the logs.
Signed-off-by: Paolo Bonzini
---
tests/check-block.sh | 6 ++--
tests/qemu-iotests/check | 6 ++--
tests/qemu-iotests/meson.build | 1 +
tests/qemu-iotests/testenv.py| 30 ++--
tests/qemu-iotests/testrunner.py | 48
5 files changed, 46 insertions(+), 45 deletions(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index 8895163625..159a040037 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -17,8 +17,8 @@ if [ "$#" -ne 0 ]; then
fi
skip() {
-echo "$*"
-exit 77
+echo "1..0 #SKIP $*"
+exit 0
}
if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then
@@ -75,7 +75,7 @@ export PYTHONUTF8=1
ret=0
for fmt in $format_list ; do
-${PYTHON} ./check -makecheck -$fmt $group || ret=1
+${PYTHON} ./check -tap -$fmt $group || ret=1
done
exit $ret
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index da1bfb839e..a8a26fcaaf 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -32,8 +32,8 @@ def make_argparser() -> argparse.ArgumentParser:
p.add_argument('-n', '--dry-run', action='store_true',
help='show me, do not run tests')
-p.add_argument('-makecheck', action='store_true',
- help='pretty print output for make check')
+p.add_argument('-tap', action='store_true',
+ help='produce TAP output')
p.add_argument('-d', dest='debug', action='store_true', help='debug')
p.add_argument('-p', dest='print', action='store_true',
@@ -161,7 +161,7 @@ if __name__ == '__main__':
if args.dry_run:
print('\n'.join(tests))
else:
-with TestRunner(env, makecheck=args.makecheck,
+with TestRunner(env, tap=args.tap,
color=args.color) as tr:
paths = [os.path.join(env.source_iotests, t) for t in tests]
ok = tr.run_tests(paths)
diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index c59c17a9a9..d93d378b11 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -7,6 +7,7 @@ if have_tools
endforeach
test('qemu-iotests', sh, args: [files('../check-block.sh')],
depends: qemu_iotests_binaries,
+ protocol: 'tap',
suite: 'block',
timeout: 0,
is_parallel: false)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index c33454fa68..6f8ae4197d 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -287,21 +287,21 @@ def __enter__(self) -> 'TestEnv':
def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None:
self.close()
-def print_env(self) -> None:
+def print_env(self, prefix='') -> None:
template = """\
-QEMU -- "{QEMU_PROG}" {QEMU_OPTIONS}
-QEMU_IMG -- "{QEMU_IMG_PROG}" {QEMU_IMG_OPTIONS}
-QEMU_IO -- "{QEMU_IO_PROG}" {QEMU_IO_OPTIONS}
-QEMU_NBD -- "{QEMU_NBD_PROG}" {QEMU_NBD_OPTIONS}
-IMGFMT-- {IMGFMT}{imgopts}
-IMGPROTO -- {IMGPROTO}
-PLATFORM -- {platform}
-TEST_DIR -- {TEST_DIR}
-SOCK_DIR -- {SOCK_DIR}
-GDB_OPTIONS -- {GDB_OPTIONS}
-VALGRIND_QEMU -- {VALGRIND_QEMU}
-PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
-"""
+{prefix}QEMU -- "{QEMU_PROG}" {QEMU_OPTIONS}
+{prefix}QEMU_IMG -- "{QEMU_IMG_PROG}" {QEMU_IMG_OPTIONS}
+{prefix}QEMU_IO -- "{QEMU_IO_PROG}" {QEMU_IO_OPTIONS}
+{prefix}QEMU_NBD -- "{QEMU_NBD_PROG}" {QEMU_NBD_OPTIONS}
+{prefix}IMGFMT-- {IMGFMT}{imgopts}
+{prefix}IMGPROTO -- {IMGPROTO}
+{prefix}PLATFORM -- {platform}
+{prefix}TEST_DIR -- {TEST_DIR}
+{prefix}SOCK_DIR -- {SOCK_DIR}
+{prefix}GDB_OPTIONS -- {GDB_OPTIONS}
+{prefix}VALGRIND_QEMU -- {VALGRIND_QEMU}
+{prefix}PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
+{prefix}"""
args = collections.defaultdict(str, self.get_env())
@@ -310,5 +310,5 @@ def print_env(self) -> None:
u = os.uname()
args['platform'] = f'{u.sysname}/{u.machine} {u.nodename} {u.release}'
-
+args['prefix'] = prefix
print(template.format_map(args))
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 0e29c2fddd..3ef14af1fa 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -126,10 +126,10 @@ def __init__(self, status: str, description: str = '',
class TestRunner(ContextManager['TestRunner']):
-def __init__(self, env: TestEnv, makecheck: bool = False,
+def __init__(self, env: TestEnv, tap: bool = False,
color: str = 'auto') -> None:
self.env = env
-self.makecheck = makecheck
+self.tap = tap
self.last_elapsed =
[RFC PATCH 0/4] Replace custom test harness with "meson test"
Hi all, Starting with Meson 0.57, "meson test" has all features of QEMU's makefile-based harness and more. In particular, some features that were added to reach feature parity are: * print reproducer command line for each failed test right below the test * keep the output of multiple (non-TAP) tests together in verbose mode, similar to "make --output-sync target" * report on TAP subtests as they are encountered It also includes nicer handling of test interruption, logging of the run in the meson-logs/ subdirectory, and a progress report/spinner. For these reasons it would be nice to adopt it and remove the Perl scripts that we have to present TAP output nicely. While at it, this series also changes qemu-iotests to be described in tests/qemu-iotests/meson.build and it replaces the current "-makecheck" output style with TAP. This is an RFC just to let people try it out and because I still haven't sorted out timeouts (e.g. OpenBSD is insanely slow on some tests). But in theory, if you want to try it out, the only things you need to know are: * while "make check" does not timeout tests like it doesn't right now, "meson test" does respect timeouts, so you could get failures because of that. That can and probably will change from RFC to final. * CTRL+C will only interrupt the longest running test. Pressing CTRL+C repeatedly three times (which you would likely do anyway, that's how things work) interrupts the whole run * Right now "make check-block" only does a single test run just like "../tests/check-block.sh", but it would be possible to add the thorough suite to "meson test --suite block" as well. * If you were using make check-report.tap and similar, they are replaced by targets like make check-report.junit.xml. This is because Gitlab is able to parse the resulting XML and include on the website a report of which tests failed. An example of non-verbose "make check" output is available at https://gitlab.com/bonzini/qemu/-/jobs/1680980620. A verbose run instead is like https://asciinema.org/a/e5irnEszSnAheOHM30exbo3F6 (does not include check-block). Paolo Paolo Bonzini (4): build: use "meson test" as the test harness build: make check-block a meson test check-block: replace -makecheck with TAP output configure: remove dead EXESUF variable .gitlab-ci.d/crossbuild-template.yml | 2 +- Makefile | 3 +- configure| 3 - meson.build | 5 +- scripts/mtest2make.py| 111 +++- scripts/tap-driver.pl| 379 --- scripts/tap-merge.pl | 111 tests/Makefile.include | 15 +- tests/check-block.sh | 28 +- tests/fp/meson.build | 2 +- tests/meson.build| 1 + tests/qemu-iotests/check | 6 +- tests/qemu-iotests/meson.build | 14 + tests/qemu-iotests/testenv.py| 30 +-- tests/qemu-iotests/testrunner.py | 48 ++-- 15 files changed, 121 insertions(+), 637 deletions(-) delete mode 100755 scripts/tap-driver.pl delete mode 100755 scripts/tap-merge.pl create mode 100644 tests/qemu-iotests/meson.build -- 2.31.1
[PATCH 1/4] build: use "meson test" as the test harness
"meson test" starting with version 0.57 is just as capable and easy to
use as QEMU's own TAP driver. All existing options for "make check"
work. The only required code change involves how to mark "slow" tests.
scripts/mtest2make.py is changed to invoke "meson test" using a trick
similar to how the Makefile already invokes ninja, i.e. turning the
MAKECMDGOALS into command line options. Tests and their dependencies
are still rebuilt correctly before "meson test" is run.
The rules for .tap output are replaced by JUnit XML; GitLab is able to
parse that output and present it in the CI pipeline report.
Signed-off-by: Paolo Bonzini
---
Makefile | 3 +-
meson.build | 5 +-
scripts/mtest2make.py | 105
scripts/tap-driver.pl | 379 --
scripts/tap-merge.pl | 111 -
tests/fp/meson.build | 2 +-
6 files changed, 42 insertions(+), 563 deletions(-)
delete mode 100755 scripts/tap-driver.pl
delete mode 100755 scripts/tap-merge.pl
diff --git a/Makefile b/Makefile
index fe9415ac64..c680b5e176 100644
--- a/Makefile
+++ b/Makefile
@@ -145,7 +145,8 @@ NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if
$(MAKE.k), -k0) \
$(filter-out -j, $(lastword -j1 $(filter -l% -j%, $(MAKEFLAGS \
ninja-cmd-goals = $(or $(MAKECMDGOALS), all)
-ninja-cmd-goals += $(foreach t, $(.tests), $(.test.deps.$t))
+ninja-cmd-goals += $(foreach t, $(.check.build-suites) $(.check.mtest-suites),
$(.check-$t.deps))
+ninja-cmd-goals += $(foreach t, $(.bench.build-suites) $(.check.mtest-suites),
$(.bench-$t.deps))
makefile-targets := build.ninja ctags TAGS cscope dist clean uninstall
# "ninja -t targets" also lists all prerequisites. If build system
diff --git a/meson.build b/meson.build
index 6b7487b725..c7af557afb 100644
--- a/meson.build
+++ b/meson.build
@@ -1,8 +1,11 @@
project('qemu', ['c'], meson_version: '>=0.58.2',
default_options: ['warning_level=1', 'c_std=gnu11', 'cpp_std=gnu++11',
'b_colorout=auto',
- 'b_staticpic=false'],
+ 'b_staticpic=false', 'stdsplit=false'],
version: files('VERSION'))
+add_test_setup('quick', exclude_suites: 'slow', is_default: true)
+add_test_setup('slow', env: ['G_TEST_SLOW=1'])
+
not_found = dependency('', required: false)
keyval = import('keyval')
ss = import('sourceset')
diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index 02c0453e67..b33c1d48df 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -13,101 +13,68 @@
class Suite(object):
def __init__(self):
-self.tests = list()
-self.slow_tests = list()
-self.executables = set()
+self.deps = set()
print('''
SPEED = quick
-# $1 = environment, $2 = test command, $3 = test name, $4 = dir
-.test-human-tap = $1 $(if $4,(cd $4 && $2),$2) -m $(SPEED) < /dev/null |
./scripts/tap-driver.pl --test-name="$3" $(if $(V),,--show-failures-only)
-.test-human-exitcode = $1 $(PYTHON) scripts/test-driver.py $(if $4,-C$4) $(if
$(V),--verbose) -- $2 < /dev/null
-.test-tap-tap = $1 $(if $4,(cd $4 && $2),$2) < /dev/null | sed "s/^[a-z][a-z]*
[0-9]*/& $3/" || true
-.test-tap-exitcode = printf "%s\\n" 1..1 "`$1 $(if $4,(cd $4 && $2),$2) <
/dev/null > /dev/null || echo "not "`ok 1 $3"
-.test.human-print = echo $(if $(V),'$1 $2','Running test $3') &&
-.test.env = MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))}
+.mtestargs = --no-rebuild -t 0
+.mtestargs += $(subst -j,--num-processes , $(filter-out -j, $(lastword -j1
$(filter -j%, $(MAKEFLAGS)
+ifneq ($(SPEED), quick)
+.mtestargs += --setup $(SPEED)
+endif
-# $1 = test name, $2 = test target (human or tap)
-.test.run = $(call
.test.$2-print,$(.test.env.$1),$(.test.cmd.$1),$(.test.name.$1)) $(call
.test-$2-$(.test.driver.$1),$(.test.env.$1),$(.test.cmd.$1),$(.test.name.$1),$(.test.dir.$1))
-
-.test.output-format = human
+.check.mtestargs = $(MTESTARGS) $(.mtestargs) $(if
$(V),--verbose,--print-errorlogs)
+.bench.mtestargs = $(MTESTARGS) $(.mtestargs) --benchmark --verbose
''')
introspect = json.load(sys.stdin)
-i = 0
def process_tests(test, targets, suites):
-global i
-env = ' '.join(('%s=%s' % (shlex.quote(k), shlex.quote(v))
-for k, v in test['env'].items()))
executable = test['cmd'][0]
try:
executable = os.path.relpath(executable)
except:
pass
-if test['workdir'] is not None:
-try:
-test['cmd'][0] = os.path.relpath(executable, test['workdir'])
-except:
-test['cmd'][0] = executable
-else:
-test['cmd'][0] = executable
-cmd = ' '.join((shlex.quote(x) for x in test['cmd']))
-driver = test['protocol'] if 'protocol' in test else 'exitcode'
-
-i += 1
-if test['workdir'] is not None:
-print('.test.dir.%d := %s' % (i, shlex.quote(test['workdir'])))
deps = (targets.get(x, []) for x in test['depends'])
[PATCH 2/4] build: make check-block a meson test
"meson test" supports can be asked to run tests verbosely. This
makes it usable also for qemu-iotests's own harness.
This lets "make check-block" reuse mtest2make.py's infrastructure to
find and build test dependencies. In the future, it will also enable
producing TAP output, for consistency with all other "make check"
tests.
Signed-off-by: Paolo Bonzini
---
meson.build| 4 ++--
scripts/mtest2make.py | 8 +++-
tests/Makefile.include | 15 ++-
tests/check-block.sh | 26 --
tests/meson.build | 1 +
tests/qemu-iotests/meson.build | 13 +
6 files changed, 37 insertions(+), 30 deletions(-)
create mode 100644 tests/qemu-iotests/meson.build
diff --git a/meson.build b/meson.build
index c7af557afb..540d367cfc 100644
--- a/meson.build
+++ b/meson.build
@@ -3,8 +3,8 @@ project('qemu', ['c'], meson_version: '>=0.58.2',
'b_staticpic=false', 'stdsplit=false'],
version: files('VERSION'))
-add_test_setup('quick', exclude_suites: 'slow', is_default: true)
-add_test_setup('slow', env: ['G_TEST_SLOW=1'])
+add_test_setup('quick', exclude_suites: ['block', 'slow'], is_default: true)
+add_test_setup('slow', exclude_suites: ['block'], env: ['G_TEST_SLOW=1'])
not_found = dependency('', required: false)
keyval = import('keyval')
diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index b33c1d48df..7e03532bbb 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -64,13 +64,15 @@ def emit_prolog(suites, prefix):
print(f'{prefix}-report.junit.xml $(all-{prefix}-xml):
{prefix}-report%.junit.xml: run-ninja')
print(f'\t$(MAKE) {prefix}$* MTESTARGS="$(MTESTARGS) --logbase
{prefix}-report$*" && ln -f meson-logs/$@ .')
-def emit_suite(name, suite, prefix):
+def emit_suite_deps(name, suite, prefix):
deps = ' '.join(suite.deps)
print(f'.{prefix}-{name}.deps = {deps}')
print(f'ifneq ($(filter {prefix}-build, $(MAKECMDGOALS)),)')
print(f'.{prefix}.build-suites += {name}')
print(f'endif')
+def emit_suite(name, suite, prefix):
+emit_suite_deps(name, suite, prefix)
targets = f'{prefix}-{name} {prefix}-report-{name}.junit.xml {prefix}
{prefix}-report.junit.xml'
print(f'ifneq ($(filter {targets}, $(MAKECMDGOALS)),)')
print(f'.{prefix}.mtest-suites += {name}')
@@ -82,6 +84,10 @@ def emit_suite(name, suite, prefix):
testsuites = defaultdict(Suite)
for test in introspect['tests']:
process_tests(test, targets, testsuites)
+# HACK: check-block is a separate target so that it runs with --verbose;
+# only write the dependencies
+emit_suite_deps('block', testsuites['block'], 'check')
+del testsuites['block']
emit_prolog(testsuites, 'check')
for name, suite in testsuites.items():
emit_suite(name, suite, 'check')
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 8434a33fe6..00a1696bde 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -149,19 +149,8 @@ check:
ifeq ($(CONFIG_TOOLS)$(CONFIG_POSIX),yy)
check: check-block
-export PYTHON
-
-ifneq ($(filter check check-block check-build, $(MAKECMDGOALS)),)
-ninja-cmd-goals += \
- qemu-img$(EXESUF) \
- qemu-io$(EXESUF) \
- qemu-nbd$(EXESUF) \
- storage-daemon/qemu-storage-daemon$(EXESUF) \
- $(filter qemu-system-%, $(ninja-targets))
-endif
-
-check-block: $(SRC_PATH)/tests/check-block.sh run-ninja
- @$<
+check-block: run-ninja
+ $(MESON) test --no-rebuild --verbose --num-processes 1 --suite block
endif
check-build: run-ninja
diff --git a/tests/check-block.sh b/tests/check-block.sh
index f86cb863de..8895163625 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -16,9 +16,13 @@ if [ "$#" -ne 0 ]; then
format_list="$@"
fi
+skip() {
+echo "$*"
+exit 77
+}
+
if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then
-echo "GPROF is enabled ==> Not running the qemu-iotests."
-exit 0
+skip "GPROF is enabled ==> Not running the qemu-iotests."
fi
# Disable tests with any sanitizer except for specific ones
@@ -36,36 +40,30 @@ for j in ${ALLOWED_SANITIZE_FLAGS}; do
done
if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
# Have a sanitize flag that is not allowed, stop
-echo "Sanitizers are enabled ==> Not running the qemu-iotests."
-exit 0
+skip "Sanitizers are enabled ==> Not running the qemu-iotests."
fi
if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
-echo "No qemu-system binary available ==> Not running the qemu-iotests."
-exit 0
+skip "No qemu-system binary available ==> Not running the qemu-iotests."
fi
if ! command -v bash >/dev/null 2>&1 ; then
-echo "bash not available ==> Not running the qemu-iotests."
-exit 0
+skip "bash not available ==> Not running the qemu-iotests."
fi
if LANG=C bash --version | grep -q 'GNU bash, version [123]' ; then
-echo
Re: [PATCH v3 02/25] include/block/block: split header into I/O and global state API
diff --git a/include/block/block-common.h b/include/block/block-common.h new file mode 100644 index 00..4f1fd8de21 --- /dev/null +++ b/include/block/block-common.h @@ -0,0 +1,389 @@ +#ifndef BLOCK_COMMON_H +#define BLOCK_COMMON_H As a new file, it probably deserves a copyright/license blurb copied from the file it is split out of. diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h new file mode 100644 index 00..b57e275da9 --- /dev/null +++ b/include/block/block-global-state.h @@ -0,0 +1,263 @@ +#ifndef BLOCK_GLOBAL_STATE_H +#define BLOCK_GLOBAL_STATE_H Likewise, here and in all other newly-split files in your series. In general, as you might have seen, I kept the same copyright/license from the original file I split. But block.h seems to be the only header with no license. +++ b/include/block/block.h @@ -1,864 +1,9 @@ #ifndef BLOCK_H #define BLOCK_H Oh. There wasn't one to copy from :( Well, now's as good a time to fix that as any. So now the question is which one to use, because I see 2 different types of copyrights templates: - long version copyright, used in block_int.h, blockjob_int.h and many others /* * QEMU System Emulator block driver * * Copyright (c) 2003 Fabrice Bellard * * Permission is hereby granted, free of charge, to any person obtaining a copy [...] * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR [...] */ - short version, used in block-backend.h and many others /* * QEMU Block backends * * Copyright (C) 2014-2016 Red Hat, Inc. * * Authors: * * * This work is licensed under the terms of the GNU LGPL, version 2.1 * or later. See the COPYING.LIB file in the top-level directory. */ Maybe since we are talking about block.h we should stick to the same format as block_int.h? I am not sure though. Thank you, Emanuele
Re: iotest 030 SIGSEGV
On 14/10/21 18:14, Vladimir Sementsov-Ogievskiy wrote: iotest 30 failing is a long story.. And as I understand the main source of all these crashes is that we do diffreent graph modifications simultaneously from parallel block jobs. In past I sent RFC series with global mutext, to fix a subset of the problem: https://patchew.org/QEMU/20201120161622.1537-1-vsement...@virtuozzo.com/ [just look at patch 5: https://patchew.org/QEMU/20201120161622.1537-1-vsement...@virtuozzo.com/20201120161622.1537-6-vsement...@virtuozzo.com/] Can you explain the way they interleave, and where the job callbacks are yielding in the middle of graph manipulations? The problem with a CoMutex is that plenty of graph manipulations happen outside coroutines, and if coroutines such as stream_co_clean yield the monitor can do graph manipulations of its own. So if the solution could be "no yielding in the middle of graph manipulations", that would be much better. In fact, maybe the coroutine API should have a way to assert "no-yield regions" (similar to how Linux croaks if you call a sleeping function while preemption is disabled). More assertions = more bugs found early. Not sure was it good enough to try to recover it. I didn't look close at Emanuele's "block layer: split block APIs in global state and I/O". Wasn't there something on protecting graph operations? In his series, graph operations are supposed to operate from the main thread (which they do) but he didn't cover the case of coroutines that yield. Paolo
Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
Am 13.10.2021 um 23:37 hat Eric Blake geschrieben:
> On Wed, Oct 13, 2021 at 03:10:38PM +0200, Damien Hedde wrote:
> > > > @@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error
> > > > **errp)
> > > > }
> > > > }
> > > > -qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
> > > > +/*
> > > > + * set dev's parent and register its id.
> > > > + * If it fails it means the id is already taken.
> > > > + */
> > > > +if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) {
> > > > +goto err_del_dev;
> > >
> > > ...nor on this, which means the g_strdup() leaks on failure.
> > >
> >
> > Since we strdup the id just before calling qdev_set_id.
> > Maybe we should do the the strdup in qdev_set_id (and free things on error
> > there too). It seems simplier than freeing things on the callee side just in
> > case of an error.
>
> Indeed. If we expected qdev_set_id() to be passed something that it
> can later free, we would have used 'char *'; but because we used
> 'const char *' for that parameter, it really does make more sense for
> the callers to pass in any string and for qdev_set_id() to do the
> necessary strdup()ing, as well as clean up on error.
Since this seems to be the only thing in the series that needs to be
addressed, I'll do the minimal fix while applying (adding g_free() to
the error path in qemu_opts_id()) and then we can clean up on top.
Kevin
