* [PATCH v4 0/5] coroutines: generate wrapper code
@ 2020-05-25 10:07 Vladimir Sementsov-Ogievskiy
2020-05-25 10:07 ` [PATCH v4 1/5] block/io: refactor coroutine wrappers Vladimir Sementsov-Ogievskiy
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-25 10:07 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, fam, vsementsov, ehabkost, qemu-devel, mreitz, stefanha,
crosa, den
Hi all!
The aim of the series is to reduce code-duplication and writing
parameters structure-packing by hand around coroutine function wrappers.
It's an alternative to "[PATCH v3] block: Factor out bdrv_run_co()"
patch.
Benefits:
- no code duplication
- less indirection
v4:
01: wording in commit message + Eric's r-b
02: add copyright header into new header
03: - add copyright headers into new headers (most funny is
generated-co-wrapper.h)
- fix Makefiles (hope this will help patchew, code builds for me
even without fixes, I don't know why)
- fix extra new-line at the end of generated block/block-gen.c
For convenience I attach generated block/block-gen.c file below.
Vladimir Sementsov-Ogievskiy (5):
block/io: refactor coroutine wrappers
block: declare some coroutine functions in block/coroutines.h
block: generate coroutine-wrapper code
block: drop bdrv_prwv
block/io: refactor save/load vmstate
Makefile | 6 +
Makefile.objs | 2 +-
block/block-gen.h | 55 ++++
block/coroutines.h | 66 +++++
include/block/block.h | 25 +-
include/block/generated-co-wrapper.h | 35 +++
block.c | 78 +-----
block/io.c | 383 ++++-----------------------
tests/test-bdrv-drain.c | 2 +-
block/Makefile.objs | 1 +
scripts/coroutine-wrapper.py | 168 ++++++++++++
11 files changed, 401 insertions(+), 420 deletions(-)
create mode 100644 block/block-gen.h
create mode 100644 block/coroutines.h
create mode 100644 include/block/generated-co-wrapper.h
create mode 100755 scripts/coroutine-wrapper.py
=== Generated by these series block/block-gen.c ===
/*
* File is generated by scripts/coroutine-wrapper.py
*/
#include "qemu/osdep.h"
#include "block/coroutines.h"
#include "block/block-gen.h"
/*
* Wrappers for bdrv_co_truncate
*/
typedef struct BdrvCoTruncate {
BdrvPollCo poll_state;
BdrvChild *child;
int64_t offset;
bool exact;
PreallocMode prealloc;
BdrvRequestFlags flags;
Error **errp;
} BdrvCoTruncate;
static void coroutine_fn bdrv_co_truncate_entry(void *opaque)
{
BdrvCoTruncate *s = opaque;
s->poll_state.ret = bdrv_co_truncate(s->child, s->offset, s->exact, s->prealloc, s->flags, s->errp);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact, PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
{
if (qemu_in_coroutine()) {
return bdrv_co_truncate(child, offset, exact, prealloc, flags, errp);
} else {
BdrvCoTruncate s = {
.poll_state.bs = child->bs,
.poll_state.in_progress = true,
.child = child,
.offset = offset,
.exact = exact,
.prealloc = prealloc,
.flags = flags,
.errp = errp,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_truncate_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_check
*/
typedef struct BdrvCoCheck {
BdrvPollCo poll_state;
BlockDriverState *bs;
BdrvCheckResult *res;
BdrvCheckMode fix;
} BdrvCoCheck;
static void coroutine_fn bdrv_co_check_entry(void *opaque)
{
BdrvCoCheck *s = opaque;
s->poll_state.ret = bdrv_co_check(s->bs, s->res, s->fix);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
{
if (qemu_in_coroutine()) {
return bdrv_co_check(bs, res, fix);
} else {
BdrvCoCheck s = {
.poll_state.bs = bs,
.poll_state.in_progress = true,
.bs = bs,
.res = res,
.fix = fix,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_check_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_invalidate_cache
*/
typedef struct BdrvCoInvalidateCache {
BdrvPollCo poll_state;
BlockDriverState *bs;
Error **errp;
} BdrvCoInvalidateCache;
static void coroutine_fn bdrv_co_invalidate_cache_entry(void *opaque)
{
BdrvCoInvalidateCache *s = opaque;
bdrv_co_invalidate_cache(s->bs, s->errp);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
{
if (qemu_in_coroutine()) {
bdrv_co_invalidate_cache(bs, errp);
} else {
BdrvCoInvalidateCache s = {
.poll_state.bs = bs,
.poll_state.in_progress = true,
.bs = bs,
.errp = errp,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_invalidate_cache_entry, &s);
bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_flush
*/
typedef struct BdrvCoFlush {
BdrvPollCo poll_state;
BlockDriverState *bs;
} BdrvCoFlush;
static void coroutine_fn bdrv_co_flush_entry(void *opaque)
{
BdrvCoFlush *s = opaque;
s->poll_state.ret = bdrv_co_flush(s->bs);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_flush(BlockDriverState *bs)
{
if (qemu_in_coroutine()) {
return bdrv_co_flush(bs);
} else {
BdrvCoFlush s = {
.poll_state.bs = bs,
.poll_state.in_progress = true,
.bs = bs,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_flush_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_pdiscard
*/
typedef struct BdrvCoPdiscard {
BdrvPollCo poll_state;
BdrvChild *child;
int64_t offset;
int64_t bytes;
} BdrvCoPdiscard;
static void coroutine_fn bdrv_co_pdiscard_entry(void *opaque)
{
BdrvCoPdiscard *s = opaque;
s->poll_state.ret = bdrv_co_pdiscard(s->child, s->offset, s->bytes);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes)
{
if (qemu_in_coroutine()) {
return bdrv_co_pdiscard(child, offset, bytes);
} else {
BdrvCoPdiscard s = {
.poll_state.bs = child->bs,
.poll_state.in_progress = true,
.child = child,
.offset = offset,
.bytes = bytes,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_pdiscard_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_readv_vmstate
*/
typedef struct BdrvCoReadvVmstate {
BdrvPollCo poll_state;
BlockDriverState *bs;
QEMUIOVector *qiov;
int64_t pos;
} BdrvCoReadvVmstate;
static void coroutine_fn bdrv_co_readv_vmstate_entry(void *opaque)
{
BdrvCoReadvVmstate *s = opaque;
s->poll_state.ret = bdrv_co_readv_vmstate(s->bs, s->qiov, s->pos);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
{
if (qemu_in_coroutine()) {
return bdrv_co_readv_vmstate(bs, qiov, pos);
} else {
BdrvCoReadvVmstate s = {
.poll_state.bs = bs,
.poll_state.in_progress = true,
.bs = bs,
.qiov = qiov,
.pos = pos,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_readv_vmstate_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_writev_vmstate
*/
typedef struct BdrvCoWritevVmstate {
BdrvPollCo poll_state;
BlockDriverState *bs;
QEMUIOVector *qiov;
int64_t pos;
} BdrvCoWritevVmstate;
static void coroutine_fn bdrv_co_writev_vmstate_entry(void *opaque)
{
BdrvCoWritevVmstate *s = opaque;
s->poll_state.ret = bdrv_co_writev_vmstate(s->bs, s->qiov, s->pos);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
{
if (qemu_in_coroutine()) {
return bdrv_co_writev_vmstate(bs, qiov, pos);
} else {
BdrvCoWritevVmstate s = {
.poll_state.bs = bs,
.poll_state.in_progress = true,
.bs = bs,
.qiov = qiov,
.pos = pos,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_writev_vmstate_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_preadv
*/
typedef struct BdrvCoPreadv {
BdrvPollCo poll_state;
BdrvChild *child;
int64_t offset;
unsigned int bytes;
QEMUIOVector *qiov;
BdrvRequestFlags flags;
} BdrvCoPreadv;
static void coroutine_fn bdrv_co_preadv_entry(void *opaque)
{
BdrvCoPreadv *s = opaque;
s->poll_state.ret = bdrv_co_preadv(s->child, s->offset, s->bytes, s->qiov, s->flags);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_preadv(BdrvChild *child, int64_t offset, unsigned int bytes, QEMUIOVector *qiov, BdrvRequestFlags flags)
{
if (qemu_in_coroutine()) {
return bdrv_co_preadv(child, offset, bytes, qiov, flags);
} else {
BdrvCoPreadv s = {
.poll_state.bs = child->bs,
.poll_state.in_progress = true,
.child = child,
.offset = offset,
.bytes = bytes,
.qiov = qiov,
.flags = flags,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_preadv_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_pwritev
*/
typedef struct BdrvCoPwritev {
BdrvPollCo poll_state;
BdrvChild *child;
int64_t offset;
unsigned int bytes;
QEMUIOVector *qiov;
BdrvRequestFlags flags;
} BdrvCoPwritev;
static void coroutine_fn bdrv_co_pwritev_entry(void *opaque)
{
BdrvCoPwritev *s = opaque;
s->poll_state.ret = bdrv_co_pwritev(s->child, s->offset, s->bytes, s->qiov, s->flags);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_pwritev(BdrvChild *child, int64_t offset, unsigned int bytes, QEMUIOVector *qiov, BdrvRequestFlags flags)
{
if (qemu_in_coroutine()) {
return bdrv_co_pwritev(child, offset, bytes, qiov, flags);
} else {
BdrvCoPwritev s = {
.poll_state.bs = child->bs,
.poll_state.in_progress = true,
.child = child,
.offset = offset,
.bytes = bytes,
.qiov = qiov,
.flags = flags,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_pwritev_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
/*
* Wrappers for bdrv_co_common_block_status_above
*/
typedef struct BdrvCoCommonBlockStatusAbove {
BdrvPollCo poll_state;
BlockDriverState *bs;
BlockDriverState *base;
bool want_zero;
int64_t offset;
int64_t bytes;
int64_t *pnum;
int64_t *map;
BlockDriverState **file;
} BdrvCoCommonBlockStatusAbove;
static void coroutine_fn bdrv_co_common_block_status_above_entry(void *opaque)
{
BdrvCoCommonBlockStatusAbove *s = opaque;
s->poll_state.ret = bdrv_co_common_block_status_above(s->bs, s->base, s->want_zero, s->offset, s->bytes, s->pnum, s->map, s->file);
s->poll_state.in_progress = false;
bdrv_poll_co__on_exit();
}
int bdrv_common_block_status_above(BlockDriverState *bs, BlockDriverState *base, bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum, int64_t *map, BlockDriverState **file)
{
if (qemu_in_coroutine()) {
return bdrv_co_common_block_status_above(bs, base, want_zero, offset, bytes, pnum, map, file);
} else {
BdrvCoCommonBlockStatusAbove s = {
.poll_state.bs = bs,
.poll_state.in_progress = true,
.bs = bs,
.base = base,
.want_zero = want_zero,
.offset = offset,
.bytes = bytes,
.pnum = pnum,
.map = map,
.file = file,
};
s.poll_state.co = qemu_coroutine_create(bdrv_co_common_block_status_above_entry, &s);
return bdrv_poll_co(&s.poll_state);
}
}
=== End of generated block/block-gen.c ===
--
2.21.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4 1/5] block/io: refactor coroutine wrappers
2020-05-25 10:07 [PATCH v4 0/5] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
@ 2020-05-25 10:07 ` Vladimir Sementsov-Ogievskiy
2020-05-26 19:24 ` Eric Blake
2020-05-25 10:07 ` [PATCH v4 2/5] block: declare some coroutine functions in block/coroutines.h Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-25 10:07 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, fam, vsementsov, ehabkost, qemu-devel, mreitz, stefanha,
crosa, den
Most of our coroutine wrappers already follow this convention:
We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as
the core function, and a wrapper 'bdrv_<something>(<same argument
list>)' which does a polling loop.
The only outsiders are the bdrv_prwv_co and
bdrv_common_block_status_above wrappers. Let's refactor them to behave
as the others, it simplifies further conversion of coroutine wrappers.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/io.c | 61 +++++++++++++++++++++++++++++-------------------------
1 file changed, 33 insertions(+), 28 deletions(-)
diff --git a/block/io.c b/block/io.c
index 121ce17a49..bd00a70b47 100644
--- a/block/io.c
+++ b/block/io.c
@@ -900,28 +900,32 @@ typedef struct RwCo {
BdrvRequestFlags flags;
} RwCo;
+static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
+ QEMUIOVector *qiov, bool is_write,
+ BdrvRequestFlags flags)
+{
+ if (is_write) {
+ return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
+ } else {
+ return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
+ }
+}
+
static void coroutine_fn bdrv_rw_co_entry(void *opaque)
{
RwCo *rwco = opaque;
- if (!rwco->is_write) {
- rwco->ret = bdrv_co_preadv(rwco->child, rwco->offset,
- rwco->qiov->size, rwco->qiov,
- rwco->flags);
- } else {
- rwco->ret = bdrv_co_pwritev(rwco->child, rwco->offset,
- rwco->qiov->size, rwco->qiov,
- rwco->flags);
- }
+ rwco->ret = bdrv_co_prwv(rwco->child, rwco->offset, rwco->qiov,
+ rwco->is_write, rwco->flags);
aio_wait_kick();
}
/*
* Process a vectored synchronous request using coroutines
*/
-static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
- QEMUIOVector *qiov, bool is_write,
- BdrvRequestFlags flags)
+static int bdrv_prwv(BdrvChild *child, int64_t offset,
+ QEMUIOVector *qiov, bool is_write,
+ BdrvRequestFlags flags)
{
Coroutine *co;
RwCo rwco = {
@@ -949,8 +953,7 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
{
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
- return bdrv_prwv_co(child, offset, &qiov, true,
- BDRV_REQ_ZERO_WRITE | flags);
+ return bdrv_prwv(child, offset, &qiov, true, BDRV_REQ_ZERO_WRITE | flags);
}
/*
@@ -999,7 +1002,7 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
{
int ret;
- ret = bdrv_prwv_co(child, offset, qiov, false, 0);
+ ret = bdrv_prwv(child, offset, qiov, false, 0);
if (ret < 0) {
return ret;
}
@@ -1023,7 +1026,7 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
{
int ret;
- ret = bdrv_prwv_co(child, offset, qiov, true, 0);
+ ret = bdrv_prwv(child, offset, qiov, true, 0);
if (ret < 0) {
return ret;
}
@@ -2443,14 +2446,15 @@ early_out:
return ret;
}
-static int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- bool want_zero,
- int64_t offset,
- int64_t bytes,
- int64_t *pnum,
- int64_t *map,
- BlockDriverState **file)
+static int coroutine_fn
+bdrv_co_common_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BlockDriverState *p;
int ret = 0;
@@ -2488,10 +2492,11 @@ static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
{
BdrvCoBlockStatusData *data = opaque;
- data->ret = bdrv_co_block_status_above(data->bs, data->base,
- data->want_zero,
- data->offset, data->bytes,
- data->pnum, data->map, data->file);
+ data->ret = bdrv_co_common_block_status_above(data->bs, data->base,
+ data->want_zero,
+ data->offset, data->bytes,
+ data->pnum, data->map,
+ data->file);
data->done = true;
aio_wait_kick();
}
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/5] block: declare some coroutine functions in block/coroutines.h
2020-05-25 10:07 [PATCH v4 0/5] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
2020-05-25 10:07 ` [PATCH v4 1/5] block/io: refactor coroutine wrappers Vladimir Sementsov-Ogievskiy
@ 2020-05-25 10:07 ` Vladimir Sementsov-Ogievskiy
2020-05-26 19:50 ` Eric Blake
2020-05-25 10:07 ` [PATCH v4 3/5] block: generate coroutine-wrapper code Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-25 10:07 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, fam, vsementsov, ehabkost, qemu-devel, mreitz, stefanha,
crosa, den
We are going to keep coroutine-wrappers code (structure-packing
parameters, BDRV_POLL wrapper functions) in a separate auto-generated
files. So, we'll need a header with declaration of original _co_
functions, for those which are static now. As well, we'll need
declarations for wrapper functions. Do these declarations now, as a
preparation step.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/coroutines.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
block.c | 8 +++---
block/io.c | 34 +++++++++++------------
3 files changed, 88 insertions(+), 21 deletions(-)
create mode 100644 block/coroutines.h
diff --git a/block/coroutines.h b/block/coroutines.h
new file mode 100644
index 0000000000..74278cfef2
--- /dev/null
+++ b/block/coroutines.h
@@ -0,0 +1,67 @@
+/*
+ * Block layer I/O functions
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef BLOCK_COROUTINES_INT_H
+#define BLOCK_COROUTINES_INT_H
+
+#include "block/block_int.h"
+
+int coroutine_fn bdrv_co_check(BlockDriverState *bs,
+ BdrvCheckResult *res, BdrvCheckMode fix);
+void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
+
+int coroutine_fn
+bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
+ bool is_write, BdrvRequestFlags flags);
+int
+bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
+ bool is_write, BdrvRequestFlags flags);
+
+int coroutine_fn
+bdrv_co_common_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file);
+int
+bdrv_common_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file);
+
+int coroutine_fn
+bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
+ bool is_read);
+int
+bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
+ bool is_read);
+
+#endif /* BLOCK_COROUTINES_INT_H */
diff --git a/block.c b/block.c
index 8416376c9b..7f06e82880 100644
--- a/block.c
+++ b/block.c
@@ -48,6 +48,7 @@
#include "qemu/timer.h"
#include "qemu/cutils.h"
#include "qemu/id.h"
+#include "block/coroutines.h"
#ifdef CONFIG_BSD
#include <sys/ioctl.h>
@@ -4625,8 +4626,8 @@ static void bdrv_delete(BlockDriverState *bs)
* free of errors) or -errno when an internal error occurred. The results of the
* check are stored in res.
*/
-static int coroutine_fn bdrv_co_check(BlockDriverState *bs,
- BdrvCheckResult *res, BdrvCheckMode fix)
+int coroutine_fn bdrv_co_check(BlockDriverState *bs,
+ BdrvCheckResult *res, BdrvCheckMode fix)
{
if (bs->drv == NULL) {
return -ENOMEDIUM;
@@ -5643,8 +5644,7 @@ void bdrv_init_with_whitelist(void)
bdrv_init();
}
-static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
- Error **errp)
+void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
{
BdrvChild *child, *parent;
uint64_t perm, shared_perm;
diff --git a/block/io.c b/block/io.c
index bd00a70b47..f5b6ce3bf6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -29,6 +29,7 @@
#include "block/blockjob.h"
#include "block/blockjob_int.h"
#include "block/block_int.h"
+#include "block/coroutines.h"
#include "qemu/cutils.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
@@ -900,9 +901,9 @@ typedef struct RwCo {
BdrvRequestFlags flags;
} RwCo;
-static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
- QEMUIOVector *qiov, bool is_write,
- BdrvRequestFlags flags)
+int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
+ QEMUIOVector *qiov, bool is_write,
+ BdrvRequestFlags flags)
{
if (is_write) {
return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
@@ -923,9 +924,9 @@ static void coroutine_fn bdrv_rw_co_entry(void *opaque)
/*
* Process a vectored synchronous request using coroutines
*/
-static int bdrv_prwv(BdrvChild *child, int64_t offset,
- QEMUIOVector *qiov, bool is_write,
- BdrvRequestFlags flags)
+int bdrv_prwv(BdrvChild *child, int64_t offset,
+ QEMUIOVector *qiov, bool is_write,
+ BdrvRequestFlags flags)
{
Coroutine *co;
RwCo rwco = {
@@ -2446,7 +2447,7 @@ early_out:
return ret;
}
-static int coroutine_fn
+int coroutine_fn
bdrv_co_common_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
bool want_zero,
@@ -2506,12 +2507,12 @@ static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
*
* See bdrv_co_block_status_above() for details.
*/
-static int bdrv_common_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- bool want_zero, int64_t offset,
- int64_t bytes, int64_t *pnum,
- int64_t *map,
- BlockDriverState **file)
+int bdrv_common_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
Coroutine *co;
BdrvCoBlockStatusData data = {
@@ -2638,7 +2639,7 @@ typedef struct BdrvVmstateCo {
int ret;
} BdrvVmstateCo;
-static int coroutine_fn
+int coroutine_fn
bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read)
{
@@ -2670,9 +2671,8 @@ static void coroutine_fn bdrv_co_rw_vmstate_entry(void *opaque)
aio_wait_kick();
}
-static inline int
-bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
- bool is_read)
+int bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
+ bool is_read)
{
if (qemu_in_coroutine()) {
return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 3/5] block: generate coroutine-wrapper code
2020-05-25 10:07 [PATCH v4 0/5] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
2020-05-25 10:07 ` [PATCH v4 1/5] block/io: refactor coroutine wrappers Vladimir Sementsov-Ogievskiy
2020-05-25 10:07 ` [PATCH v4 2/5] block: declare some coroutine functions in block/coroutines.h Vladimir Sementsov-Ogievskiy
@ 2020-05-25 10:07 ` Vladimir Sementsov-Ogievskiy
2020-05-26 20:51 ` [PATCH] fixup! " Eric Blake
2020-05-26 21:30 ` [PATCH v4 3/5] " Eric Blake
2020-05-25 10:08 ` [PATCH v4 4/5] block: drop bdrv_prwv Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-25 10:07 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, fam, vsementsov, ehabkost, qemu-devel, mreitz, stefanha,
crosa, den
We have a very frequent pattern of creating coroutine from function
with several arguments:
- create structure to pack parameters
- create _entry function to call original function taking parameters
from struct
- do different magic to handle completion: set ret to NOT_DONE or
EINPROGRESS, use separate bool for void functions
- fill the struct and create coroutine from _entry function and this
struct as a parameter
- do coroutine enter and BDRV_POLL_WHILE loop
Let's reduce code duplication. Here:
Functional part (BDRV_POLL_WHILE loop, aio_wait_kick()) moved to
(non-generated) block/block-gen.h
Mechanical part (arguments packing, different kind of needed wrappers)
are generated from template by scripts/coroutine-wrapper.py to
resulting file block/block-gen.c
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
Makefile | 6 +
Makefile.objs | 2 +-
block/block-gen.h | 55 ++++++
block/coroutines.h | 7 +-
include/block/block.h | 17 +-
include/block/generated-co-wrapper.h | 35 ++++
block.c | 70 --------
block/io.c | 260 ---------------------------
block/Makefile.objs | 1 +
scripts/coroutine-wrapper.py | 168 +++++++++++++++++
10 files changed, 281 insertions(+), 340 deletions(-)
create mode 100644 block/block-gen.h
create mode 100644 include/block/generated-co-wrapper.h
create mode 100755 scripts/coroutine-wrapper.py
diff --git a/Makefile b/Makefile
index 40e4f7677b..ec15b8ea89 100644
--- a/Makefile
+++ b/Makefile
@@ -159,6 +159,8 @@ generated-files-$(CONFIG_TRACE_UST) += trace-ust-all.c
generated-files-y += module_block.h
+generated-files-y += block/block-gen.c
+
TRACE_HEADERS = trace-root.h $(trace-events-subdirs:%=%/trace.h)
TRACE_SOURCES = trace-root.c $(trace-events-subdirs:%=%/trace.c)
TRACE_DTRACE =
@@ -175,6 +177,10 @@ generated-files-y += $(TRACE_SOURCES)
generated-files-y += $(BUILD_DIR)/trace-events-all
generated-files-y += .git-submodule-status
+COROUTINE_HEADERS = include/block/block.h block/coroutines.h
+block/block-gen.c: $(COROUTINE_HEADERS) $(SRC_PATH)/scripts/coroutine-wrapper.py
+ $(call quiet-command, cat $(COROUTINE_HEADERS) | $(SRC_PATH)/scripts/coroutine-wrapper.py > $@,"GEN","$(TARGET_DIR)$@")
+
trace-group-name = $(shell dirname $1 | sed -e 's/[^a-zA-Z0-9]/_/g')
tracetool-y = $(SRC_PATH)/scripts/tracetool.py
diff --git a/Makefile.objs b/Makefile.objs
index 99774cfd25..8cb20f94c3 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -14,7 +14,7 @@ chardev-obj-y = chardev/
authz-obj-y = authz/
block-obj-y = block/ block/monitor/ nbd/ scsi/
-block-obj-y += block.o blockjob.o job.o
+block-obj-y += block.o blockjob.o job.o block/block-gen.o
block-obj-y += qemu-io-cmds.o
block-obj-$(CONFIG_REPLICATION) += replication.o
diff --git a/block/block-gen.h b/block/block-gen.h
new file mode 100644
index 0000000000..11a44247b7
--- /dev/null
+++ b/block/block-gen.h
@@ -0,0 +1,55 @@
+/*
+ * Block layer I/O functions
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2020 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef BLOCK_BLOCK_GEN_H
+#define BLOCK_BLOCK_GEN_H
+
+#include "block/block_int.h"
+
+/* This function is called at the end of generated coroutine entries. */
+static inline void bdrv_poll_co__on_exit(void)
+{
+ aio_wait_kick();
+}
+
+/* Base structure for argument packing structures */
+typedef struct BdrvPollCo {
+ BlockDriverState *bs;
+ bool in_progress;
+ int ret;
+ Coroutine *co; /* Keep pointer here for debugging */
+} BdrvPollCo;
+
+static inline int bdrv_poll_co(BdrvPollCo *s)
+{
+ assert(!qemu_in_coroutine());
+
+ bdrv_coroutine_enter(s->bs, s->co);
+ BDRV_POLL_WHILE(s->bs, s->in_progress);
+
+ return s->ret;
+}
+
+#endif /* BLOCK_BLOCK_GEN_H */
diff --git a/block/coroutines.h b/block/coroutines.h
index 74278cfef2..e2047697d6 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -26,6 +26,7 @@
#define BLOCK_COROUTINES_INT_H
#include "block/block_int.h"
+#include "block/generated-co-wrapper.h"
int coroutine_fn bdrv_co_check(BlockDriverState *bs,
BdrvCheckResult *res, BdrvCheckMode fix);
@@ -34,7 +35,7 @@ void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
int coroutine_fn
bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
bool is_write, BdrvRequestFlags flags);
-int
+int generated_co_wrapper
bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
bool is_write, BdrvRequestFlags flags);
@@ -47,7 +48,7 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
int64_t *pnum,
int64_t *map,
BlockDriverState **file);
-int
+int generated_co_wrapper
bdrv_common_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
bool want_zero,
@@ -60,7 +61,7 @@ bdrv_common_block_status_above(BlockDriverState *bs,
int coroutine_fn
bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read);
-int
+int generated_co_wrapper
bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read);
diff --git a/include/block/block.h b/include/block/block.h
index 25e299605e..aed6ffcc4f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -9,6 +9,7 @@
#include "block/dirty-bitmap.h"
#include "block/blockjob.h"
#include "qemu/hbitmap.h"
+#include "block/generated-co-wrapper.h"
/* block.c */
typedef struct BlockDriver BlockDriver;
@@ -398,8 +399,9 @@ void bdrv_refresh_filename(BlockDriverState *bs);
int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
PreallocMode prealloc, BdrvRequestFlags flags,
Error **errp);
-int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
- PreallocMode prealloc, BdrvRequestFlags flags, Error **errp);
+int generated_co_wrapper
+bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
+ PreallocMode prealloc, BdrvRequestFlags flags, Error **errp);
int64_t bdrv_nb_sectors(BlockDriverState *bs);
int64_t bdrv_getlength(BlockDriverState *bs);
@@ -441,7 +443,8 @@ typedef enum {
BDRV_FIX_ERRORS = 2,
} BdrvCheckMode;
-int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
+int generated_co_wrapper bdrv_check(BlockDriverState *bs, BdrvCheckResult *res,
+ BdrvCheckMode fix);
/* The units of offset and total_work_size may be chosen arbitrarily by the
* block driver; total_work_size may change during the course of the amendment
@@ -464,12 +467,13 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb);
int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf);
/* Invalidate any cached metadata used by image formats */
-void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
+void generated_co_wrapper bdrv_invalidate_cache(BlockDriverState *bs,
+ Error **errp);
void bdrv_invalidate_cache_all(Error **errp);
int bdrv_inactivate_all(void);
/* Ensure contents are flushed to disk. */
-int bdrv_flush(BlockDriverState *bs);
+int generated_co_wrapper bdrv_flush(BlockDriverState *bs);
int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
int bdrv_flush_all(void);
void bdrv_close_all(void);
@@ -484,7 +488,8 @@ void bdrv_drain_all(void);
AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \
cond); })
-int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
+int generated_co_wrapper bdrv_pdiscard(BdrvChild *child, int64_t offset,
+ int64_t bytes);
int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
int bdrv_has_zero_init_1(BlockDriverState *bs);
int bdrv_has_zero_init(BlockDriverState *bs);
diff --git a/include/block/generated-co-wrapper.h b/include/block/generated-co-wrapper.h
new file mode 100644
index 0000000000..62c6e053ba
--- /dev/null
+++ b/include/block/generated-co-wrapper.h
@@ -0,0 +1,35 @@
+/*
+ * Block layer I/O functions
+ *
+ * Copyright (c) 2020 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef BLOCK_GENERATED_CO_WRAPPER_H
+#define BLOCK_GENERATED_CO_WRAPPER_H
+
+/*
+ * generated_co_wrapper
+ * Function specifier, which does nothing but marking functions to be
+ * generated by scripts/coroutine-wrapper.py
+ */
+#define generated_co_wrapper
+
+#endif /* BLOCK_GENERATED_CO_WRAPPER_H */
diff --git a/block.c b/block.c
index 7f06e82880..c1132ab323 100644
--- a/block.c
+++ b/block.c
@@ -4640,43 +4640,6 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
return bs->drv->bdrv_co_check(bs, res, fix);
}
-typedef struct CheckCo {
- BlockDriverState *bs;
- BdrvCheckResult *res;
- BdrvCheckMode fix;
- int ret;
-} CheckCo;
-
-static void coroutine_fn bdrv_check_co_entry(void *opaque)
-{
- CheckCo *cco = opaque;
- cco->ret = bdrv_co_check(cco->bs, cco->res, cco->fix);
- aio_wait_kick();
-}
-
-int bdrv_check(BlockDriverState *bs,
- BdrvCheckResult *res, BdrvCheckMode fix)
-{
- Coroutine *co;
- CheckCo cco = {
- .bs = bs,
- .res = res,
- .ret = -EINPROGRESS,
- .fix = fix,
- };
-
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_check_co_entry(&cco);
- } else {
- co = qemu_coroutine_create(bdrv_check_co_entry, &cco);
- bdrv_coroutine_enter(bs, co);
- BDRV_POLL_WHILE(bs, cco.ret == -EINPROGRESS);
- }
-
- return cco.ret;
-}
-
/*
* Return values:
* 0 - success
@@ -5721,39 +5684,6 @@ void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
}
}
-typedef struct InvalidateCacheCo {
- BlockDriverState *bs;
- Error **errp;
- bool done;
-} InvalidateCacheCo;
-
-static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque)
-{
- InvalidateCacheCo *ico = opaque;
- bdrv_co_invalidate_cache(ico->bs, ico->errp);
- ico->done = true;
- aio_wait_kick();
-}
-
-void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
-{
- Coroutine *co;
- InvalidateCacheCo ico = {
- .bs = bs,
- .done = false,
- .errp = errp
- };
-
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_invalidate_cache_co_entry(&ico);
- } else {
- co = qemu_coroutine_create(bdrv_invalidate_cache_co_entry, &ico);
- bdrv_coroutine_enter(bs, co);
- BDRV_POLL_WHILE(bs, !ico.done);
- }
-}
-
void bdrv_invalidate_cache_all(Error **errp)
{
BlockDriverState *bs;
diff --git a/block/io.c b/block/io.c
index f5b6ce3bf6..f9700cc897 100644
--- a/block/io.c
+++ b/block/io.c
@@ -892,15 +892,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
return 0;
}
-typedef struct RwCo {
- BdrvChild *child;
- int64_t offset;
- QEMUIOVector *qiov;
- bool is_write;
- int ret;
- BdrvRequestFlags flags;
-} RwCo;
-
int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
QEMUIOVector *qiov, bool is_write,
BdrvRequestFlags flags)
@@ -912,43 +903,6 @@ int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
}
}
-static void coroutine_fn bdrv_rw_co_entry(void *opaque)
-{
- RwCo *rwco = opaque;
-
- rwco->ret = bdrv_co_prwv(rwco->child, rwco->offset, rwco->qiov,
- rwco->is_write, rwco->flags);
- aio_wait_kick();
-}
-
-/*
- * Process a vectored synchronous request using coroutines
- */
-int bdrv_prwv(BdrvChild *child, int64_t offset,
- QEMUIOVector *qiov, bool is_write,
- BdrvRequestFlags flags)
-{
- Coroutine *co;
- RwCo rwco = {
- .child = child,
- .offset = offset,
- .qiov = qiov,
- .is_write = is_write,
- .ret = NOT_DONE,
- .flags = flags,
- };
-
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_rw_co_entry(&rwco);
- } else {
- co = qemu_coroutine_create(bdrv_rw_co_entry, &rwco);
- bdrv_coroutine_enter(child->bs, co);
- BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
- }
- return rwco.ret;
-}
-
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags)
{
@@ -2223,20 +2177,6 @@ int bdrv_flush_all(void)
return result;
}
-
-typedef struct BdrvCoBlockStatusData {
- BlockDriverState *bs;
- BlockDriverState *base;
- bool want_zero;
- int64_t offset;
- int64_t bytes;
- int64_t *pnum;
- int64_t *map;
- BlockDriverState **file;
- int ret;
- bool done;
-} BdrvCoBlockStatusData;
-
int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
bool want_zero,
int64_t offset,
@@ -2488,56 +2428,6 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
return ret;
}
-/* Coroutine wrapper for bdrv_block_status_above() */
-static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
-{
- BdrvCoBlockStatusData *data = opaque;
-
- data->ret = bdrv_co_common_block_status_above(data->bs, data->base,
- data->want_zero,
- data->offset, data->bytes,
- data->pnum, data->map,
- data->file);
- data->done = true;
- aio_wait_kick();
-}
-
-/*
- * Synchronous wrapper around bdrv_co_block_status_above().
- *
- * See bdrv_co_block_status_above() for details.
- */
-int bdrv_common_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- bool want_zero, int64_t offset,
- int64_t bytes, int64_t *pnum,
- int64_t *map,
- BlockDriverState **file)
-{
- Coroutine *co;
- BdrvCoBlockStatusData data = {
- .bs = bs,
- .base = base,
- .want_zero = want_zero,
- .offset = offset,
- .bytes = bytes,
- .pnum = pnum,
- .map = map,
- .file = file,
- .done = false,
- };
-
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_block_status_above_co_entry(&data);
- } else {
- co = qemu_coroutine_create(bdrv_block_status_above_co_entry, &data);
- bdrv_coroutine_enter(bs, co);
- BDRV_POLL_WHILE(bs, !data.done);
- }
- return data.ret;
-}
-
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriverState **file)
@@ -2631,14 +2521,6 @@ int bdrv_is_allocated_above(BlockDriverState *top,
return 0;
}
-typedef struct BdrvVmstateCo {
- BlockDriverState *bs;
- QEMUIOVector *qiov;
- int64_t pos;
- bool is_read;
- int ret;
-} BdrvVmstateCo;
-
int coroutine_fn
bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read)
@@ -2664,34 +2546,6 @@ bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
return ret;
}
-static void coroutine_fn bdrv_co_rw_vmstate_entry(void *opaque)
-{
- BdrvVmstateCo *co = opaque;
- co->ret = bdrv_co_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);
- aio_wait_kick();
-}
-
-int bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
- bool is_read)
-{
- if (qemu_in_coroutine()) {
- return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
- } else {
- BdrvVmstateCo data = {
- .bs = bs,
- .qiov = qiov,
- .pos = pos,
- .is_read = is_read,
- .ret = -EINPROGRESS,
- };
- Coroutine *co = qemu_coroutine_create(bdrv_co_rw_vmstate_entry, &data);
-
- bdrv_coroutine_enter(bs, co);
- BDRV_POLL_WHILE(bs, data.ret == -EINPROGRESS);
- return data.ret;
- }
-}
-
int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
int64_t pos, int size)
{
@@ -2767,20 +2621,6 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb)
/**************************************************************/
/* Coroutine block device emulation */
-typedef struct FlushCo {
- BlockDriverState *bs;
- int ret;
-} FlushCo;
-
-
-static void coroutine_fn bdrv_flush_co_entry(void *opaque)
-{
- FlushCo *rwco = opaque;
-
- rwco->ret = bdrv_co_flush(rwco->bs);
- aio_wait_kick();
-}
-
int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
{
int current_gen;
@@ -2893,40 +2733,6 @@ early_exit:
return ret;
}
-int bdrv_flush(BlockDriverState *bs)
-{
- Coroutine *co;
- FlushCo flush_co = {
- .bs = bs,
- .ret = NOT_DONE,
- };
-
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_flush_co_entry(&flush_co);
- } else {
- co = qemu_coroutine_create(bdrv_flush_co_entry, &flush_co);
- bdrv_coroutine_enter(bs, co);
- BDRV_POLL_WHILE(bs, flush_co.ret == NOT_DONE);
- }
-
- return flush_co.ret;
-}
-
-typedef struct DiscardCo {
- BdrvChild *child;
- int64_t offset;
- int64_t bytes;
- int ret;
-} DiscardCo;
-static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque)
-{
- DiscardCo *rwco = opaque;
-
- rwco->ret = bdrv_co_pdiscard(rwco->child, rwco->offset, rwco->bytes);
- aio_wait_kick();
-}
-
int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
int64_t bytes)
{
@@ -3041,28 +2847,6 @@ out:
return ret;
}
-int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes)
-{
- Coroutine *co;
- DiscardCo rwco = {
- .child = child,
- .offset = offset,
- .bytes = bytes,
- .ret = NOT_DONE,
- };
-
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_pdiscard_co_entry(&rwco);
- } else {
- co = qemu_coroutine_create(bdrv_pdiscard_co_entry, &rwco);
- bdrv_coroutine_enter(child->bs, co);
- BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
- }
-
- return rwco.ret;
-}
-
int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
{
BlockDriver *drv = bs->drv;
@@ -3460,47 +3244,3 @@ out:
return ret;
}
-
-typedef struct TruncateCo {
- BdrvChild *child;
- int64_t offset;
- bool exact;
- PreallocMode prealloc;
- BdrvRequestFlags flags;
- Error **errp;
- int ret;
-} TruncateCo;
-
-static void coroutine_fn bdrv_truncate_co_entry(void *opaque)
-{
- TruncateCo *tco = opaque;
- tco->ret = bdrv_co_truncate(tco->child, tco->offset, tco->exact,
- tco->prealloc, tco->flags, tco->errp);
- aio_wait_kick();
-}
-
-int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
- PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
-{
- Coroutine *co;
- TruncateCo tco = {
- .child = child,
- .offset = offset,
- .exact = exact,
- .prealloc = prealloc,
- .flags = flags,
- .errp = errp,
- .ret = NOT_DONE,
- };
-
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_truncate_co_entry(&tco);
- } else {
- co = qemu_coroutine_create(bdrv_truncate_co_entry, &tco);
- bdrv_coroutine_enter(child->bs, co);
- BDRV_POLL_WHILE(child->bs, tco.ret == NOT_DONE);
- }
-
- return tco.ret;
-}
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 3635b6b4c1..05e4d033c1 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -45,6 +45,7 @@ block-obj-y += crypto.o
block-obj-y += aio_task.o
block-obj-y += backup-top.o
block-obj-y += filter-compress.o
+block-obj-y += block-gen.o
common-obj-y += monitor/
block-obj-y += stream.o
diff --git a/scripts/coroutine-wrapper.py b/scripts/coroutine-wrapper.py
new file mode 100755
index 0000000000..72115f7dd8
--- /dev/null
+++ b/scripts/coroutine-wrapper.py
@@ -0,0 +1,168 @@
+#!/usr/bin/env python3
+#
+# Generate coroutine wrappers for block subsystem.
+#
+# Copyright (c) 2020 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import re
+from typing import List, Iterator
+
+header = '''/*
+ * File is generated by scripts/coroutine-wrapper.py
+ */
+
+#include "qemu/osdep.h"
+#include "block/coroutines.h"
+#include "block/block-gen.h"'''
+
+template = """
+
+/*
+ * Wrappers for $name$
+ */
+
+typedef struct $struct_name$ {
+ BdrvPollCo poll_state;
+ $fields$
+} $struct_name$;
+
+static void coroutine_fn $name$_entry(void *opaque)
+{
+ $struct_name$ *s = opaque;
+
+ $assign_ret$$name$($args_from_s$);
+
+ s->poll_state.in_progress = false;
+
+ bdrv_poll_co__on_exit();
+}
+
+$ret_type$ $wrapper_name$($args_def$)
+{
+ if (qemu_in_coroutine()) {
+ $do_return$$name$($arg_names$);
+ } else {
+ $struct_name$ s = {
+ .poll_state.bs = $bs$,
+ .poll_state.in_progress = true,
+
+ $initializers$
+ };
+
+ s.poll_state.co = qemu_coroutine_create($name$_entry, &s);
+
+ $do_return$bdrv_poll_co(&s.poll_state);
+ }
+}"""
+
+# We want to use python string.format() formatter, which uses curly brackets
+# as separators. But it's not comfortable with C. So, we used dollars instead,
+# and now is the time to escape curly brackets and convert dollars.
+template = template.replace('{', '{{').replace('}', '}}')
+template = re.sub(r'\$(\w+)\$', r'{\1}', template)
+
+
+class ParamDecl:
+ param_re = re.compile(r'(?P<decl>'
+ r'(?P<type>.*[ *])'
+ r'(?P<name>[a-z][a-z0-9_]*)'
+ r')')
+
+ def __init__(self, param_decl: str) -> None:
+ m = self.param_re.match(param_decl.strip())
+ self.decl = m.group('decl')
+ self.type = m.group('type')
+ self.name = m.group('name')
+
+
+class FuncDecl:
+ def __init__(self, return_type: str, name: str, args: str) -> None:
+ self.return_type = return_type.strip()
+ self.name = name.strip()
+ self.args: List[ParamDecl] = []
+ self.args = [ParamDecl(arg) for arg in args.split(',')]
+
+ def get_args_decl(self) -> str:
+ return ', '.join(arg.decl for arg in self.args)
+
+ def get_arg_names(self) -> str:
+ return ', '.join(arg.name for arg in self.args)
+
+ def gen_struct_fields(self) -> str:
+ return '\n '.join(f'{arg.decl};' for arg in self.args)
+
+ def gen_struct_initializers(self, indent: int) -> str:
+ sep = '\n' + ' ' * indent
+ return sep.join(f'.{a.name} = {a.name},' for a in self.args)
+
+
+# Match wrappers declaration, with generated_co_wrapper mark
+func_decl_re = re.compile(r'^(?P<return_type>(int|void))'
+ r'\s*generated_co_wrapper\s*'
+ r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
+ r'\((?P<args>[^)]*)\);$', re.MULTILINE)
+
+
+def func_decl_iter(text: str) -> Iterator:
+ for m in func_decl_re.finditer(text):
+ yield FuncDecl(return_type=m.group('return_type'),
+ name=m.group('wrapper_name'),
+ args=m.group('args'))
+
+
+def struct_name(func_name: str) -> str:
+ """some_function_name -> SomeFunctionName"""
+ words = func_name.split('_')
+ words = [w[0].upper() + w[1:] for w in words]
+ return ''.join(words)
+
+
+def make_wrapper(func: FuncDecl) -> str:
+ assert func.name.startswith('bdrv_')
+ co_name = 'bdrv_co_' + func.name[5:]
+
+ has_ret = func.return_type != 'void'
+
+ params = {
+ 'name': co_name,
+ 'do_return': 'return ' if has_ret else '',
+ 'assign_ret': 's->poll_state.ret = ' if has_ret else '',
+ 'struct_name': struct_name(co_name),
+ 'wrapper_name': func.name,
+ 'ret_type': func.return_type,
+ 'args_def': func.get_args_decl(),
+ 'arg_names': func.get_arg_names(),
+ 'fields': func.gen_struct_fields(),
+ 'initializers': func.gen_struct_initializers(12),
+ 'args_from_s': ', '.join(f's->{a.name}' for a in func.args),
+ }
+
+ if func.args[0].type == 'BlockDriverState *':
+ params['bs'] = 'bs'
+ else:
+ assert func.args[0].type == 'BdrvChild *'
+ params['bs'] = 'child->bs'
+
+ return template.format(**params)
+
+
+if __name__ == '__main__':
+ import sys
+
+ print(header)
+ for func in func_decl_iter(sys.stdin.read()):
+ print(make_wrapper(func))
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 4/5] block: drop bdrv_prwv
2020-05-25 10:07 [PATCH v4 0/5] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2020-05-25 10:07 ` [PATCH v4 3/5] block: generate coroutine-wrapper code Vladimir Sementsov-Ogievskiy
@ 2020-05-25 10:08 ` Vladimir Sementsov-Ogievskiy
2020-05-26 21:34 ` Eric Blake
2020-05-25 10:08 ` [PATCH v4 5/5] block/io: refactor save/load vmstate Vladimir Sementsov-Ogievskiy
2020-05-25 13:14 ` [PATCH v4 0/5] coroutines: generate wrapper code no-reply
5 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-25 10:08 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, fam, vsementsov, ehabkost, qemu-devel, mreitz, stefanha,
crosa, den
Now, when we are not more paying extra code for coroutine wrappers,
there no more sence in extra indirection layer: bdrv_prwv(). Let's drop
it and instead genereate pure bdrv_preadv() and bdrv_pwritev().
Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on
success, auto generated functions will instead return zero, as their
_co_ prototype. Still, it's simple to make the conversion safe: the
only external user of bdrv_pwritev() is test-bdrv-drain, and it is
comfortable enough with bdrv_co_pwritev() instead. So prototypes are
moved to local block/coroutines.h. Next, the only internal use is
bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on
success.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/coroutines.h | 10 ++++-----
include/block/block.h | 2 --
block/io.c | 49 ++++++++---------------------------------
tests/test-bdrv-drain.c | 2 +-
4 files changed, 15 insertions(+), 48 deletions(-)
diff --git a/block/coroutines.h b/block/coroutines.h
index e2047697d6..233b8b3694 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -32,12 +32,12 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
BdrvCheckResult *res, BdrvCheckMode fix);
void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
-int coroutine_fn
-bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
- bool is_write, BdrvRequestFlags flags);
int generated_co_wrapper
-bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
- bool is_write, BdrvRequestFlags flags);
+bdrv_preadv(BdrvChild *child, int64_t offset, unsigned int bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags);
+int generated_co_wrapper
+bdrv_pwritev(BdrvChild *child, int64_t offset, unsigned int bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags);
int coroutine_fn
bdrv_co_common_block_status_above(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index aed6ffcc4f..dce99f8453 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -379,9 +379,7 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags);
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes);
-int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes);
-int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
const void *buf, int count);
/*
diff --git a/block/io.c b/block/io.c
index f9700cc897..305ee7219a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -892,23 +892,11 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
return 0;
}
-int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
- QEMUIOVector *qiov, bool is_write,
- BdrvRequestFlags flags)
-{
- if (is_write) {
- return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
- } else {
- return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
- }
-}
-
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags)
{
- QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
-
- return bdrv_prwv(child, offset, &qiov, true, BDRV_REQ_ZERO_WRITE | flags);
+ return bdrv_pwritev(child, offset, bytes, NULL,
+ BDRV_REQ_ZERO_WRITE | flags);
}
/*
@@ -952,41 +940,19 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
}
}
-/* return < 0 if error. See bdrv_pwrite() for the return codes */
-int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
-{
- int ret;
-
- ret = bdrv_prwv(child, offset, qiov, false, 0);
- if (ret < 0) {
- return ret;
- }
-
- return qiov->size;
-}
-
/* See bdrv_pwrite() for the return codes */
int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
{
+ int ret;
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
if (bytes < 0) {
return -EINVAL;
}
- return bdrv_preadv(child, offset, &qiov);
-}
-
-int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
-{
- int ret;
+ ret = bdrv_preadv(child, offset, bytes, &qiov, 0);
- ret = bdrv_prwv(child, offset, qiov, true, 0);
- if (ret < 0) {
- return ret;
- }
-
- return qiov->size;
+ return ret < 0 ? ret : bytes;
}
/* Return no. of bytes on success or < 0 on error. Important errors are:
@@ -997,13 +963,16 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
*/
int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
{
+ int ret;
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
if (bytes < 0) {
return -EINVAL;
}
- return bdrv_pwritev(child, offset, &qiov);
+ ret = bdrv_pwritev(child, offset, bytes, &qiov, 0);
+
+ return ret < 0 ? ret : bytes;
}
/*
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 1107271840..1595bbc92e 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -1872,7 +1872,7 @@ static int coroutine_fn bdrv_replace_test_co_preadv(BlockDriverState *bs,
}
s->io_co = NULL;
- ret = bdrv_preadv(bs->backing, offset, qiov);
+ ret = bdrv_co_preadv(bs->backing, offset, bytes, qiov, 0);
s->has_read = true;
/* Wake up drain_co if it runs */
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 5/5] block/io: refactor save/load vmstate
2020-05-25 10:07 [PATCH v4 0/5] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2020-05-25 10:08 ` [PATCH v4 4/5] block: drop bdrv_prwv Vladimir Sementsov-Ogievskiy
@ 2020-05-25 10:08 ` Vladimir Sementsov-Ogievskiy
2020-05-26 21:36 ` Eric Blake
2020-05-25 13:14 ` [PATCH v4 0/5] coroutines: generate wrapper code no-reply
5 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-25 10:08 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, fam, vsementsov, ehabkost, qemu-devel, mreitz, stefanha,
crosa, den
Like for read/write in a previous commit, drop extra indirection layer,
generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/coroutines.h | 10 +++----
include/block/block.h | 6 ++--
block/io.c | 67 ++++++++++++++++++++++---------------------
3 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/block/coroutines.h b/block/coroutines.h
index 233b8b3694..fd0dd6a5e6 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -58,11 +58,9 @@ bdrv_common_block_status_above(BlockDriverState *bs,
int64_t *map,
BlockDriverState **file);
-int coroutine_fn
-bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
- bool is_read);
-int generated_co_wrapper
-bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
- bool is_read);
+int coroutine_fn bdrv_co_readv_vmstate(BlockDriverState *bs,
+ QEMUIOVector *qiov, int64_t pos);
+int coroutine_fn bdrv_co_writev_vmstate(BlockDriverState *bs,
+ QEMUIOVector *qiov, int64_t pos);
#endif /* BLOCK_COROUTINES_INT_H */
diff --git a/include/block/block.h b/include/block/block.h
index dce99f8453..618c0b76b5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -568,8 +568,10 @@ int path_has_protocol(const char *path);
int path_is_absolute(const char *path);
char *path_combine(const char *base_path, const char *filename);
-int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
-int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
+int generated_co_wrapper
+bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
+int generated_co_wrapper
+bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
int64_t pos, int size);
diff --git a/block/io.c b/block/io.c
index 305ee7219a..8c1da3c335 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2491,66 +2491,67 @@ int bdrv_is_allocated_above(BlockDriverState *top,
}
int coroutine_fn
-bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
- bool is_read)
+bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
{
BlockDriver *drv = bs->drv;
int ret = -ENOTSUP;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+
bdrv_inc_in_flight(bs);
- if (!drv) {
- ret = -ENOMEDIUM;
- } else if (drv->bdrv_load_vmstate) {
- if (is_read) {
- ret = drv->bdrv_load_vmstate(bs, qiov, pos);
- } else {
- ret = drv->bdrv_save_vmstate(bs, qiov, pos);
- }
+ if (drv->bdrv_load_vmstate) {
+ ret = drv->bdrv_load_vmstate(bs, qiov, pos);
} else if (bs->file) {
- ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
+ ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos);
}
bdrv_dec_in_flight(bs);
+
return ret;
}
-int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
- int64_t pos, int size)
+int coroutine_fn
+bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
{
- QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
- int ret;
+ BlockDriver *drv = bs->drv;
+ int ret = -ENOTSUP;
- ret = bdrv_writev_vmstate(bs, &qiov, pos);
- if (ret < 0) {
- return ret;
+ if (!drv) {
+ return -ENOMEDIUM;
}
- return size;
-}
+ bdrv_inc_in_flight(bs);
-int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
-{
- return bdrv_rw_vmstate(bs, qiov, pos, false);
+ if (drv->bdrv_load_vmstate) {
+ ret = drv->bdrv_save_vmstate(bs, qiov, pos);
+ } else if (bs->file) {
+ ret = bdrv_co_writev_vmstate(bs->file->bs, qiov, pos);
+ }
+
+ bdrv_dec_in_flight(bs);
+
+ return ret;
}
-int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
+int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
int64_t pos, int size)
{
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
- int ret;
-
- ret = bdrv_readv_vmstate(bs, &qiov, pos);
- if (ret < 0) {
- return ret;
- }
+ int ret = bdrv_writev_vmstate(bs, &qiov, pos);
- return size;
+ return ret < 0 ? ret : size;
}
-int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
+int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
+ int64_t pos, int size)
{
- return bdrv_rw_vmstate(bs, qiov, pos, true);
+ QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
+ int ret = bdrv_readv_vmstate(bs, &qiov, pos);
+
+ return ret < 0 ? ret : size;
}
/**************************************************************/
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 0/5] coroutines: generate wrapper code
2020-05-25 10:07 [PATCH v4 0/5] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2020-05-25 10:08 ` [PATCH v4 5/5] block/io: refactor save/load vmstate Vladimir Sementsov-Ogievskiy
@ 2020-05-25 13:14 ` no-reply
2020-05-25 13:48 ` Vladimir Sementsov-Ogievskiy
5 siblings, 1 reply; 17+ messages in thread
From: no-reply @ 2020-05-25 13:14 UTC (permalink / raw)
To: vsementsov
Cc: kwolf, fam, vsementsov, ehabkost, qemu-block, qemu-devel, mreitz,
stefanha, crosa, den
Patchew URL: https://patchew.org/QEMU/20200525100801.13859-1-vsementsov@virtuozzo.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
block/vhdx-log.o: In function `vhdx_log_write_and_flush':
/tmp/qemu-test/src/block/vhdx-log.c:1049: undefined reference to `bdrv_flush'
/tmp/qemu-test/src/block/vhdx-log.c:1061: undefined reference to `bdrv_flush'
collect2: error: ld returned 1 exit status
make: *** [qemu-nbd] Error 1
make: *** Waiting for unfinished jobs....
LINK vhost-user-input
block.o: In function `bdrv_invalidate_cache_all':
---
block/vhdx-log.o: In function `vhdx_log_write_and_flush':
/tmp/qemu-test/src/block/vhdx-log.c:1049: undefined reference to `bdrv_flush'
/tmp/qemu-test/src/block/vhdx-log.c:1061: undefined reference to `bdrv_flush'
collect2: error: ld returned 1 exit status
make: *** [qemu-storage-daemon] Error 1
block.o: In function `bdrv_invalidate_cache_all':
/tmp/qemu-test/src/block.c:5697: undefined reference to `bdrv_invalidate_cache'
block.o: In function `bdrv_close':
---
block/vhdx-log.o: In function `vhdx_log_write_and_flush':
/tmp/qemu-test/src/block/vhdx-log.c:1049: undefined reference to `bdrv_flush'
/tmp/qemu-test/src/block/vhdx-log.c:1061: undefined reference to `bdrv_flush'
collect2: error: ld returned 1 exit status
make: *** [qemu-io] Error 1
GEN x86_64-softmmu/hmp-commands.h
GEN x86_64-softmmu/hmp-commands-info.h
GEN x86_64-softmmu/config-devices.h
---
../block/vhdx-log.o: In function `vhdx_log_write_and_flush':
/tmp/qemu-test/src/block/vhdx-log.c:1049: undefined reference to `bdrv_flush'
/tmp/qemu-test/src/block/vhdx-log.c:1061: undefined reference to `bdrv_flush'
collect2: error: ld returned 1 exit status
make[1]: *** [qemu-system-x86_64] Error 1
make: *** [x86_64-softmmu/all] Error 2
../blockdev.o: In function `external_snapshot_prepare':
/tmp/qemu-test/src/blockdev.c:1480: undefined reference to `bdrv_flush'
../block.o: In function `bdrv_invalidate_cache_all':
---
../block/vhdx-log.o: In function `vhdx_log_write_and_flush':
/tmp/qemu-test/src/block/vhdx-log.c:1049: undefined reference to `bdrv_flush'
/tmp/qemu-test/src/block/vhdx-log.c:1061: undefined reference to `bdrv_flush'
collect2: error: ld returned 1 exit status
make[1]: *** [qemu-system-aarch64] Error 1
make: *** [aarch64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=cb38533df12a483595ddf084eb0a9493', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ga_jl87d/src/docker-src.2020-05-25-09.11.03.18391:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=cb38533df12a483595ddf084eb0a9493
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ga_jl87d/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 3m21.247s
user 0m8.473s
The full log is available at
http://patchew.org/logs/20200525100801.13859-1-vsementsov@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 0/5] coroutines: generate wrapper code
2020-05-25 13:14 ` [PATCH v4 0/5] coroutines: generate wrapper code no-reply
@ 2020-05-25 13:48 ` Vladimir Sementsov-Ogievskiy
2020-05-26 20:42 ` Eric Blake
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-25 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, fam, ehabkost, qemu-block, mreitz, stefanha, crosa, den
25.05.2020 16:14, no-reply@patchew.org wrote:
> Patchew URL:https://patchew.org/QEMU/20200525100801.13859-1-vsementsov@virtuozzo.com/
>
>
>
> Hi,
>
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
>
> block/vhdx-log.o: In function `vhdx_log_write_and_flush':
> /tmp/qemu-test/src/block/vhdx-log.c:1049: undefined reference to `bdrv_flush'
> /tmp/qemu-test/src/block/vhdx-log.c:1061: undefined reference to `bdrv_flush'
> collect2: error: ld returned 1 exit status
> make: *** [qemu-nbd] Error 1
Hmm. Who can help?
I assume, that this is because I've added block/block-gen.o into ./Makefile.objs, and not into block/Makefile.objs. I'll try it with next resend.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/5] block/io: refactor coroutine wrappers
2020-05-25 10:07 ` [PATCH v4 1/5] block/io: refactor coroutine wrappers Vladimir Sementsov-Ogievskiy
@ 2020-05-26 19:24 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-26 19:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
On 5/25/20 5:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> Most of our coroutine wrappers already follow this convention:
>
> We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as
> the core function, and a wrapper 'bdrv_<something>(<same argument
> list>)' which does a polling loop.
>
> The only outsiders are the bdrv_prwv_co and
> bdrv_common_block_status_above wrappers. Let's refactor them to behave
> as the others, it simplifies further conversion of coroutine wrappers.
It might be worth mentioning that a later patch in the series will then
further reduce the indirection present here. But R-b still stands.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> block/io.c | 61 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 33 insertions(+), 28 deletions(-)
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/5] block: declare some coroutine functions in block/coroutines.h
2020-05-25 10:07 ` [PATCH v4 2/5] block: declare some coroutine functions in block/coroutines.h Vladimir Sementsov-Ogievskiy
@ 2020-05-26 19:50 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-26 19:50 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
On 5/25/20 5:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to keep coroutine-wrappers code (structure-packing
> parameters, BDRV_POLL wrapper functions) in a separate auto-generated
s/a //
> files. So, we'll need a header with declaration of original _co_
> functions, for those which are static now. As well, we'll need
> declarations for wrapper functions. Do these declarations now, as a
> preparation step.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 0/5] coroutines: generate wrapper code
2020-05-25 13:48 ` Vladimir Sementsov-Ogievskiy
@ 2020-05-26 20:42 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-26 20:42 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: kwolf, fam, ehabkost, qemu-block, mreitz, stefanha, crosa, den
On 5/25/20 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.05.2020 16:14, no-reply@patchew.org wrote:
>> Patchew
>> URL:https://patchew.org/QEMU/20200525100801.13859-1-vsementsov@virtuozzo.com/
>>
>>
>>
>>
>> Hi,
>>
>> This series failed the docker-quick@centos7 build test. Please find
>> the testing commands and
>> their output below. If you have Docker installed, you can probably
>> reproduce it
>> locally.
>>
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>> make docker-image-centos7 V=1 NETWORK=1
>> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
>> === TEST SCRIPT END ===
>>
>> block/vhdx-log.o: In function `vhdx_log_write_and_flush':
>> /tmp/qemu-test/src/block/vhdx-log.c:1049: undefined reference to
>> `bdrv_flush'
>> /tmp/qemu-test/src/block/vhdx-log.c:1061: undefined reference to
>> `bdrv_flush'
>> collect2: error: ld returned 1 exit status
>> make: *** [qemu-nbd] Error 1
>
> Hmm. Who can help?
>
> I assume, that this is because I've added block/block-gen.o into
> ./Makefile.objs, and not into block/Makefile.objs. I'll try it with next
> resend.
Are you doing in-tree or VPATH builds? When I tried a VPATH build, I got:
$ make -C build block/block-gen.c V=1
make: Entering directory '/home/eblake/qemu/build'
...
cat include/block/block.h block/coroutines.h |
/home/eblake/qemu/scripts/coroutine-wrapper.py > block/block-gen.c
cat: include/block/block.h: No such file or directory
cat: block/coroutines.h: No such file or directory
make: 'block/block-gen.c' is up to date.
make: Leaving directory '/home/eblake/qemu/build'
and a resulting block/block-gen.c that declares nothing but 3 #includes.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] fixup! block: generate coroutine-wrapper code
2020-05-25 10:07 ` [PATCH v4 3/5] block: generate coroutine-wrapper code Vladimir Sementsov-Ogievskiy
@ 2020-05-26 20:51 ` Eric Blake
2020-05-26 21:30 ` [PATCH v4 3/5] " Eric Blake
1 sibling, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-26 20:51 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, vsementsov, ehabkost, qemu-block, mreitz, stefanha, crosa, den
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Fix Makefile usage for VPATH builds
Signed-off-by: Eric Blake <eblake@redhat.com>
---
This fixup lets me build locally with my VPATH build; it probably also
explains why patchew and other CLI tools (which use VPATH) were
failing.
Makefile | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index ec15b8ea8900..d194cf067ba7 100644
--- a/Makefile
+++ b/Makefile
@@ -179,7 +179,9 @@ generated-files-y += .git-submodule-status
COROUTINE_HEADERS = include/block/block.h block/coroutines.h
block/block-gen.c: $(COROUTINE_HEADERS) $(SRC_PATH)/scripts/coroutine-wrapper.py
- $(call quiet-command, cat $(COROUTINE_HEADERS) | $(SRC_PATH)/scripts/coroutine-wrapper.py > $@,"GEN","$(TARGET_DIR)$@")
+ $(call quiet-command, \
+ cat $(addprefix $(SRC_PATH)/,$(COROUTINE_HEADERS)) | \
+ $(SRC_PATH)/scripts/coroutine-wrapper.py > $@,"GEN","$(TARGET_DIR)$@")
trace-group-name = $(shell dirname $1 | sed -e 's/[^a-zA-Z0-9]/_/g')
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/5] block: generate coroutine-wrapper code
2020-05-25 10:07 ` [PATCH v4 3/5] block: generate coroutine-wrapper code Vladimir Sementsov-Ogievskiy
2020-05-26 20:51 ` [PATCH] fixup! " Eric Blake
@ 2020-05-26 21:30 ` Eric Blake
2020-05-27 11:24 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 17+ messages in thread
From: Eric Blake @ 2020-05-26 21:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
On 5/25/20 5:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> We have a very frequent pattern of creating coroutine from function
> with several arguments:
>
> - create structure to pack parameters
> - create _entry function to call original function taking parameters
> from struct
> - do different magic to handle completion: set ret to NOT_DONE or
> EINPROGRESS, use separate bool for void functions
> - fill the struct and create coroutine from _entry function and this
> struct as a parameter
> - do coroutine enter and BDRV_POLL_WHILE loop
>
> Let's reduce code duplication. Here:
>
> Functional part (BDRV_POLL_WHILE loop, aio_wait_kick()) moved to
> (non-generated) block/block-gen.h
>
> Mechanical part (arguments packing, different kind of needed wrappers)
> are generated from template by scripts/coroutine-wrapper.py to
> resulting file block/block-gen.c
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> @@ -175,6 +177,10 @@ generated-files-y += $(TRACE_SOURCES)
> generated-files-y += $(BUILD_DIR)/trace-events-all
> generated-files-y += .git-submodule-status
>
> +COROUTINE_HEADERS = include/block/block.h block/coroutines.h
> +block/block-gen.c: $(COROUTINE_HEADERS) $(SRC_PATH)/scripts/coroutine-wrapper.py
> + $(call quiet-command, cat $(COROUTINE_HEADERS) | $(SRC_PATH)/scripts/coroutine-wrapper.py > $@,"GEN","$(TARGET_DIR)$@")
> +
Not VPATH-friendly; I posted a proposed fixup! separately.
> trace-group-name = $(shell dirname $1 | sed -e 's/[^a-zA-Z0-9]/_/g')
>
> tracetool-y = $(SRC_PATH)/scripts/tracetool.py
> diff --git a/Makefile.objs b/Makefile.objs
> index 99774cfd25..8cb20f94c3 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -14,7 +14,7 @@ chardev-obj-y = chardev/
> authz-obj-y = authz/
>
> block-obj-y = block/ block/monitor/ nbd/ scsi/
> -block-obj-y += block.o blockjob.o job.o
> +block-obj-y += block.o blockjob.o job.o block/block-gen.o
It may be cleaner to add this in block/Makefile.objs rather than in
top-level Makefile.objs. In fact, rearranging your mail a bit...
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 3635b6b4c1..05e4d033c1 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -45,6 +45,7 @@ block-obj-y += crypto.o
> block-obj-y += aio_task.o
> block-obj-y += backup-top.o
> block-obj-y += filter-compress.o
> +block-obj-y += block-gen.o
> common-obj-y += monitor/
>
> block-obj-y += stream.o
...you did just that. Dropping the change to top-level Makefile.objs
seems to make no difference to a correct build.
> +++ b/block/block-gen.h
> @@ -0,0 +1,55 @@
> +/*
> + * Block layer I/O functions
Is this still the best one-line summary for this file? Especially since...
> +
> +/* This function is called at the end of generated coroutine entries. */
> +static inline void bdrv_poll_co__on_exit(void)
> +{
> + aio_wait_kick();
> +}
> +
> +/* Base structure for argument packing structures */
> +typedef struct BdrvPollCo {
> + BlockDriverState *bs;
> + bool in_progress;
> + int ret;
> + Coroutine *co; /* Keep pointer here for debugging */
> +} BdrvPollCo;
> +
> +static inline int bdrv_poll_co(BdrvPollCo *s)
> +{
> + assert(!qemu_in_coroutine());
> +
> + bdrv_coroutine_enter(s->bs, s->co);
> + BDRV_POLL_WHILE(s->bs, s->in_progress);
> +
> + return s->ret;
> +}
This part looks fine.
> +++ b/include/block/generated-co-wrapper.h
> @@ -0,0 +1,35 @@
> +/*
> + * Block layer I/O functions
...you repeat it here?
> +/*
> + * generated_co_wrapper
> + * Function specifier, which does nothing but marking functions to be
> + * generated by scripts/coroutine-wrapper.py
> + */
> +#define generated_co_wrapper
> +
> +#endif /* BLOCK_GENERATED_CO_WRAPPER_H */
Not sure if a separate header was needed for this, but I guess it
doesn't hurt. I might have just used block_int.h.
> diff --git a/block.c b/block.c
> index 7f06e82880..c1132ab323 100644
> --- a/block.c
> +++ b/block.c
> @@ -4640,43 +4640,6 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
> return bs->drv->bdrv_co_check(bs, res, fix);
> }
>
> -typedef struct CheckCo {
> - BlockDriverState *bs;
> - BdrvCheckResult *res;
> - BdrvCheckMode fix;
> - int ret;
> -} CheckCo;
This patch is doing two things - introducing a new generator script that
scans the code for generated_co_wrapper tags, _and_ adds the tags in as
many places as possible. It makes for a big patch. Better might have
been to introduce the script and the concept of a tag in one patch but
not actually tag any new functions (so the generated file is basically
empty, but you prove the build works and can audit the script without
being bogged down by the mechanical changes), then do a separate patch
with adding the tags and deleting the code now covered by the generator
(which will be mostly mechanical).
> +++ b/scripts/coroutine-wrapper.py
> @@ -0,0 +1,168 @@
> +#!/usr/bin/env python3
My python review skills are weak, so you'll probably want another
reviewer here (although I _can_ state that I checked the generated
block/block-gen.c file and it makes sense).
> +import re
> +from typing import List, Iterator
> +
> +header = '''/*
> + * File is generated by scripts/coroutine-wrapper.py
> + */
It's worth also generating a short copyright blurb into the generated file.
> +
> +#include "qemu/osdep.h"
> +#include "block/coroutines.h"
> +#include "block/block-gen.h"'''
> +
> +template = """
Why ''' above and """ here? While python accepts both forms,
consistency is desirable. I have no idea what pylint thinks of your
code, though, so I may be completely overlooking idiomatic styles.
> +
> +/*
> + * Wrappers for $name$
> + */
> +
> +typedef struct $struct_name$ {
> + BdrvPollCo poll_state;
> + $fields$
> +} $struct_name$;
> +
> +static void coroutine_fn $name$_entry(void *opaque)
> +{
> + $struct_name$ *s = opaque;
> +
> + $assign_ret$$name$($args_from_s$);
Creates a long line. Doesn't matter since this is generated code, so I
don't know if it's worth trying to coerce python into generating
something that is wrapped nicely.
> +
> + s->poll_state.in_progress = false;
> +
> + bdrv_poll_co__on_exit();
That function only calls aio_wait_kick(). Why did you not want to call
that directly here, instead of going through such a one-line wrapper
function?
> +}
> +
> +$ret_type$ $wrapper_name$($args_def$)
> +{
> + if (qemu_in_coroutine()) {
> + $do_return$$name$($arg_names$);
Would it be any simpler to teach bdrv_co_invalidate_cache() to return a
value, so that you can eliminate the need for $do_return$ and just
hard-code 'return ' here? The .bdrv_co_invalidate_cache driver callback
can still return void, and just have the wrapper in block.c return 0.
But if you want to do that, it would be a separate prerequisite patch.
> + } else {
> + $struct_name$ s = {
> + .poll_state.bs = $bs$,
> + .poll_state.in_progress = true,
> +
> + $initializers$
> + };
> +
> + s.poll_state.co = qemu_coroutine_create($name$_entry, &s);
> +
> + $do_return$bdrv_poll_co(&s.poll_state);
Another spot impacted by that one outlier.
> + }
> +}"""
> +
> +# We want to use python string.format() formatter, which uses curly brackets
> +# as separators. But it's not comfortable with C. So, we used dollars instead,
> +# and now is the time to escape curly brackets and convert dollars.
> +template = template.replace('{', '{{').replace('}', '}}')
> +template = re.sub(r'\$(\w+)\$', r'{\1}', template)
I'll leave it to others to decide if your approach has a more idiomatic
python solution. I'm used to the qapi generator using code like this in
script/qapi/visit.py:
def gen_visit_members_decl(name):
return mcgen('''
void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error
**errp);
''',
c_name=c_name(name))
which has the same end goal of inserting named tags into a format
string, but which uses %(tag) for tags rather than your novel approach
of $tag$ with post-processing.
> +
> +
> +class ParamDecl:
> + param_re = re.compile(r'(?P<decl>'
> + r'(?P<type>.*[ *])'
> + r'(?P<name>[a-z][a-z0-9_]*)'
> + r')')
> +
> + def __init__(self, param_decl: str) -> None:
> + m = self.param_re.match(param_decl.strip())
> + self.decl = m.group('decl')
> + self.type = m.group('type')
> + self.name = m.group('name')
> +
> +
> +class FuncDecl:
> + def __init__(self, return_type: str, name: str, args: str) -> None:
> + self.return_type = return_type.strip()
> + self.name = name.strip()
> + self.args: List[ParamDecl] = []
> + self.args = [ParamDecl(arg) for arg in args.split(',')]
> +
> + def get_args_decl(self) -> str:
> + return ', '.join(arg.decl for arg in self.args)
> +
> + def get_arg_names(self) -> str:
> + return ', '.join(arg.name for arg in self.args)
> +
> + def gen_struct_fields(self) -> str:
> + return '\n '.join(f'{arg.decl};' for arg in self.args)
> +
> + def gen_struct_initializers(self, indent: int) -> str:
> + sep = '\n' + ' ' * indent
> + return sep.join(f'.{a.name} = {a.name},' for a in self.args)
> +
> +
> +# Match wrappers declaration, with generated_co_wrapper mark
s/declaration, with/declared with a/
> +func_decl_re = re.compile(r'^(?P<return_type>(int|void))'
> + r'\s*generated_co_wrapper\s*'
> + r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
> + r'\((?P<args>[^)]*)\);$', re.MULTILINE)
> +
> +
> +def func_decl_iter(text: str) -> Iterator:
> + for m in func_decl_re.finditer(text):
> + yield FuncDecl(return_type=m.group('return_type'),
> + name=m.group('wrapper_name'),
> + args=m.group('args'))
> +
> +
> +def struct_name(func_name: str) -> str:
> + """some_function_name -> SomeFunctionName"""
> + words = func_name.split('_')
> + words = [w[0].upper() + w[1:] for w in words]
> + return ''.join(words)
> +
> +
> +def make_wrapper(func: FuncDecl) -> str:
> + assert func.name.startswith('bdrv_')
> + co_name = 'bdrv_co_' + func.name[5:]
> +
> + has_ret = func.return_type != 'void'
> +
> + params = {
> + 'name': co_name,
> + 'do_return': 'return ' if has_ret else '',
> + 'assign_ret': 's->poll_state.ret = ' if has_ret else '',
See my comments above about possibly normalizing the one outlier of
bdrv_co_invalidate_cache first.
> + 'struct_name': struct_name(co_name),
> + 'wrapper_name': func.name,
> + 'ret_type': func.return_type,
> + 'args_def': func.get_args_decl(),
> + 'arg_names': func.get_arg_names(),
> + 'fields': func.gen_struct_fields(),
> + 'initializers': func.gen_struct_initializers(12),
12 looks like a magic number.
> + 'args_from_s': ', '.join(f's->{a.name}' for a in func.args),
> + }
> +
> + if func.args[0].type == 'BlockDriverState *':
> + params['bs'] = 'bs'
> + else:
> + assert func.args[0].type == 'BdrvChild *'
> + params['bs'] = 'child->bs'
> +
> + return template.format(**params)
> +
> +
> +if __name__ == '__main__':
> + import sys
> +
> + print(header)
> + for func in func_decl_iter(sys.stdin.read()):
> + print(make_wrapper(func))
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/5] block: drop bdrv_prwv
2020-05-25 10:08 ` [PATCH v4 4/5] block: drop bdrv_prwv Vladimir Sementsov-Ogievskiy
@ 2020-05-26 21:34 ` Eric Blake
2020-05-27 11:35 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2020-05-26 21:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
On 5/25/20 5:08 AM, Vladimir Sementsov-Ogievskiy wrote:
> Now, when we are not more paying extra code for coroutine wrappers,
> there no more sence in extra indirection layer: bdrv_prwv(). Let's drop
> it and instead genereate pure bdrv_preadv() and bdrv_pwritev().
Typos and grammar; I suggest:
Now that we are not maintaining boilerplate code for coroutine wrappers,
there is no more sense in keeping the extra indirection layer of
bdrv_prwv(). Let's drop it and instead generate pure bdrv_preadv() and
bdrv_pwritev().
>
> Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on
> success, auto generated functions will instead return zero, as their
> _co_ prototype. Still, it's simple to make the conversion safe: the
> only external user of bdrv_pwritev() is test-bdrv-drain, and it is
> comfortable enough with bdrv_co_pwritev() instead. So prototypes are
> moved to local block/coroutines.h. Next, the only internal use is
> bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on
> success.
Does returning bytes on success buy us anything useful? We don't allow
partial success, so blindly returning 0 on success is no less useful.
True, we'd have to audit callers to make sure we aren't doing an
inadvertent semantic change.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/coroutines.h | 10 ++++-----
> include/block/block.h | 2 --
> block/io.c | 49 ++++++++---------------------------------
> tests/test-bdrv-drain.c | 2 +-
> 4 files changed, 15 insertions(+), 48 deletions(-)
>
At any rate, I think this patch is reasonable.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 5/5] block/io: refactor save/load vmstate
2020-05-25 10:08 ` [PATCH v4 5/5] block/io: refactor save/load vmstate Vladimir Sementsov-Ogievskiy
@ 2020-05-26 21:36 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-05-26 21:36 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
On 5/25/20 5:08 AM, Vladimir Sementsov-Ogievskiy wrote:
> Like for read/write in a previous commit, drop extra indirection layer,
> generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/coroutines.h | 10 +++----
> include/block/block.h | 6 ++--
> block/io.c | 67 ++++++++++++++++++++++---------------------
> 3 files changed, 42 insertions(+), 41 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/5] block: generate coroutine-wrapper code
2020-05-26 21:30 ` [PATCH v4 3/5] " Eric Blake
@ 2020-05-27 11:24 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-27 11:24 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: kwolf, fam, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
27.05.2020 00:30, Eric Blake wrote:
> On 5/25/20 5:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> We have a very frequent pattern of creating coroutine from function
>> with several arguments:
>>
>> - create structure to pack parameters
>> - create _entry function to call original function taking parameters
>> from struct
>> - do different magic to handle completion: set ret to NOT_DONE or
>> EINPROGRESS, use separate bool for void functions
>> - fill the struct and create coroutine from _entry function and this
>> struct as a parameter
>> - do coroutine enter and BDRV_POLL_WHILE loop
>>
>> Let's reduce code duplication. Here:
>>
>> Functional part (BDRV_POLL_WHILE loop, aio_wait_kick()) moved to
>> (non-generated) block/block-gen.h
>>
>> Mechanical part (arguments packing, different kind of needed wrappers)
>> are generated from template by scripts/coroutine-wrapper.py to
>> resulting file block/block-gen.c
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>
>> @@ -175,6 +177,10 @@ generated-files-y += $(TRACE_SOURCES)
>> generated-files-y += $(BUILD_DIR)/trace-events-all
>> generated-files-y += .git-submodule-status
>> +COROUTINE_HEADERS = include/block/block.h block/coroutines.h
>> +block/block-gen.c: $(COROUTINE_HEADERS) $(SRC_PATH)/scripts/coroutine-wrapper.py
>> + $(call quiet-command, cat $(COROUTINE_HEADERS) | $(SRC_PATH)/scripts/coroutine-wrapper.py > $@,"GEN","$(TARGET_DIR)$@")
>> +
>
> Not VPATH-friendly; I posted a proposed fixup! separately.
Thanks!
>
>> trace-group-name = $(shell dirname $1 | sed -e 's/[^a-zA-Z0-9]/_/g')
>> tracetool-y = $(SRC_PATH)/scripts/tracetool.py
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 99774cfd25..8cb20f94c3 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -14,7 +14,7 @@ chardev-obj-y = chardev/
>> authz-obj-y = authz/
>> block-obj-y = block/ block/monitor/ nbd/ scsi/
>> -block-obj-y += block.o blockjob.o job.o
>> +block-obj-y += block.o blockjob.o job.o block/block-gen.o
>
> It may be cleaner to add this in block/Makefile.objs rather than in top-level Makefile.objs. In fact, rearranging your mail a bit...
>
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 3635b6b4c1..05e4d033c1 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -45,6 +45,7 @@ block-obj-y += crypto.o
> > block-obj-y += aio_task.o
> > block-obj-y += backup-top.o
> > block-obj-y += filter-compress.o
> > +block-obj-y += block-gen.o
> > common-obj-y += monitor/
> >
> > block-obj-y += stream.o
>
> ...you did just that. Dropping the change to top-level Makefile.objs seems to make no difference to a correct build.
Oops, yes.
>
>> +++ b/block/block-gen.h
>> @@ -0,0 +1,55 @@
>> +/*
>> + * Block layer I/O functions
>
> Is this still the best one-line summary for this file? Especially since...
Yes, it's outdated. Maybe
Block coroutine wrapping core, used by auto-generated block/block-gen.c
>
>> +
>> +/* This function is called at the end of generated coroutine entries. */
>> +static inline void bdrv_poll_co__on_exit(void)
>> +{
>> + aio_wait_kick();
>> +}
>> +
>> +/* Base structure for argument packing structures */
>> +typedef struct BdrvPollCo {
>> + BlockDriverState *bs;
>> + bool in_progress;
>> + int ret;
>> + Coroutine *co; /* Keep pointer here for debugging */
>> +} BdrvPollCo;
>> +
>> +static inline int bdrv_poll_co(BdrvPollCo *s)
>> +{
>> + assert(!qemu_in_coroutine());
>> +
>> + bdrv_coroutine_enter(s->bs, s->co);
>> + BDRV_POLL_WHILE(s->bs, s->in_progress);
>> +
>> + return s->ret;
>> +}
>
> This part looks fine.
>
>
>> +++ b/include/block/generated-co-wrapper.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * Block layer I/O functions
>
> ...you repeat it here?
>
>> +/*
>> + * generated_co_wrapper
>> + * Function specifier, which does nothing but marking functions to be
>> + * generated by scripts/coroutine-wrapper.py
>> + */
>> +#define generated_co_wrapper
>> +
>> +#endif /* BLOCK_GENERATED_CO_WRAPPER_H */
>
> Not sure if a separate header was needed for this, but I guess it doesn't hurt. I might have just used block_int.h.
Than we'll have to include block_int.h in block.h.. Is it OK? This is the reason why I decided to keep it separate.
>
>> diff --git a/block.c b/block.c
>> index 7f06e82880..c1132ab323 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4640,43 +4640,6 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
>> return bs->drv->bdrv_co_check(bs, res, fix);
>> }
>> -typedef struct CheckCo {
>> - BlockDriverState *bs;
>> - BdrvCheckResult *res;
>> - BdrvCheckMode fix;
>> - int ret;
>> -} CheckCo;
>
> This patch is doing two things - introducing a new generator script that scans the code for generated_co_wrapper tags, _and_ adds the tags in as many places as possible. It makes for a big patch. Better might have been to introduce the script and the concept of a tag in one patch but not actually tag any new functions (so the generated file is basically empty, but you prove the build works and can audit the script without being bogged down by the mechanical changes), then do a separate patch with adding the tags and deleting the code now covered by the generator (which will be mostly mechanical).
No problem, will do.
>
>> +++ b/scripts/coroutine-wrapper.py
>> @@ -0,0 +1,168 @@
>> +#!/usr/bin/env python3
>
> My python review skills are weak, so you'll probably want another reviewer here (although I _can_ state that I checked the generated block/block-gen.c file and it makes sense).
>
>
>> +import re
>> +from typing import List, Iterator
>> +
>> +header = '''/*
>> + * File is generated by scripts/coroutine-wrapper.py
>> + */
>
> It's worth also generating a short copyright blurb into the generated file.
>
>> +
>> +#include "qemu/osdep.h"
>> +#include "block/coroutines.h"
>> +#include "block/block-gen.h"'''
>> +
>> +template = """
>
> Why ''' above and """ here? While python accepts both forms, consistency is desirable. I have no idea what pylint thinks of your code, though, so I may be completely overlooking idiomatic styles.
''' is used just to not interfer with " around include files.. So I tend to generally use """ for multiline strings, but the above is and exception just for internal " characters.
>
>> +
>> +/*
>> + * Wrappers for $name$
>> + */
>> +
>> +typedef struct $struct_name$ {
>> + BdrvPollCo poll_state;
>> + $fields$
>> +} $struct_name$;
>> +
>> +static void coroutine_fn $name$_entry(void *opaque)
>> +{
>> + $struct_name$ *s = opaque;
>> +
>> + $assign_ret$$name$($args_from_s$);
>
> Creates a long line. Doesn't matter since this is generated code, so I don't know if it's worth trying to coerce python into generating something that is wrapped nicely.
>
>> +
>> + s->poll_state.in_progress = false;
>> +
>> + bdrv_poll_co__on_exit();
>
> That function only calls aio_wait_kick(). Why did you not want to call that directly here, instead of going through such a one-line wrapper function?
To keep polling logic out of generated code, in block/block-gen.h, like bdrv_poll_co. Does it worth doing - I'm not sure. Up to maintainers.
>
>> +}
>> +
>> +$ret_type$ $wrapper_name$($args_def$)
>> +{
>> + if (qemu_in_coroutine()) {
>> + $do_return$$name$($arg_names$);
>
> Would it be any simpler to teach bdrv_co_invalidate_cache() to return a value, so that you can eliminate the need for $do_return$ and just hard-code 'return ' here? The .bdrv_co_invalidate_cache driver callback can still return void, and just have the wrapper in block.c return 0. But if you want to do that, it would be a separate prerequisite patch.
Hmm. I thought, that it's not a good reason to add a always-success return value.. But stop! bdrv_invalidate_cache is a void functions with errp argument, so it makes sense to add a return value anyway. Will do.
>
>> + } else {
>> + $struct_name$ s = {
>> + .poll_state.bs = $bs$,
>> + .poll_state.in_progress = true,
>> +
>> + $initializers$
>> + };
>> +
>> + s.poll_state.co = qemu_coroutine_create($name$_entry, &s);
>> +
>> + $do_return$bdrv_poll_co(&s.poll_state);
>
> Another spot impacted by that one outlier.
>
>> + }
>> +}"""
>> +
>> +# We want to use python string.format() formatter, which uses curly brackets
>> +# as separators. But it's not comfortable with C. So, we used dollars instead,
>> +# and now is the time to escape curly brackets and convert dollars.
>> +template = template.replace('{', '{{').replace('}', '}}')
>> +template = re.sub(r'\$(\w+)\$', r'{\1}', template)
>
> I'll leave it to others to decide if your approach has a more idiomatic python solution. I'm used to the qapi generator using code like this in script/qapi/visit.py:
>
> def gen_visit_members_decl(name):
> return mcgen('''
>
> void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
> ''',
> c_name=c_name(name))
>
> which has the same end goal of inserting named tags into a format string, but which uses %(tag) for tags rather than your novel approach of $tag$ with post-processing.
Note, that actually not %(tag) but %(tag)s
%(name)s notation comes from % interpolation operator.. As I understand, in modern python using .format(), f-strings and templates is preferable.
But I agree, that introducing new hand-made notation is not a great idea. I think, I'll move to string.Template or f-strings.
>
>> +
>> +
>> +class ParamDecl:
>> + param_re = re.compile(r'(?P<decl>'
>> + r'(?P<type>.*[ *])'
>> + r'(?P<name>[a-z][a-z0-9_]*)'
>> + r')')
>> +
>> + def __init__(self, param_decl: str) -> None:
>> + m = self.param_re.match(param_decl.strip())
>> + self.decl = m.group('decl')
>> + self.type = m.group('type')
>> + self.name = m.group('name')
>> +
>> +
>> +class FuncDecl:
>> + def __init__(self, return_type: str, name: str, args: str) -> None:
>> + self.return_type = return_type.strip()
>> + self.name = name.strip()
>> + self.args: List[ParamDecl] = []
>> + self.args = [ParamDecl(arg) for arg in args.split(',')]
>> +
>> + def get_args_decl(self) -> str:
>> + return ', '.join(arg.decl for arg in self.args)
>> +
>> + def get_arg_names(self) -> str:
>> + return ', '.join(arg.name for arg in self.args)
>> +
>> + def gen_struct_fields(self) -> str:
>> + return '\n '.join(f'{arg.decl};' for arg in self.args)
>> +
>> + def gen_struct_initializers(self, indent: int) -> str:
>> + sep = '\n' + ' ' * indent
>> + return sep.join(f'.{a.name} = {a.name},' for a in self.args)
>> +
>> +
>> +# Match wrappers declaration, with generated_co_wrapper mark
>
> s/declaration, with/declared with a/
>
>> +func_decl_re = re.compile(r'^(?P<return_type>(int|void))'
>> + r'\s*generated_co_wrapper\s*'
>> + r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
>> + r'\((?P<args>[^)]*)\);$', re.MULTILINE)
>> +
>> +
>> +def func_decl_iter(text: str) -> Iterator:
>> + for m in func_decl_re.finditer(text):
>> + yield FuncDecl(return_type=m.group('return_type'),
>> + name=m.group('wrapper_name'),
>> + args=m.group('args'))
>> +
>> +
>> +def struct_name(func_name: str) -> str:
>> + """some_function_name -> SomeFunctionName"""
>> + words = func_name.split('_')
>> + words = [w[0].upper() + w[1:] for w in words]
>> + return ''.join(words)
>> +
>> +
>> +def make_wrapper(func: FuncDecl) -> str:
>> + assert func.name.startswith('bdrv_')
>> + co_name = 'bdrv_co_' + func.name[5:]
>> +
>> + has_ret = func.return_type != 'void'
>> +
>> + params = {
>> + 'name': co_name,
>> + 'do_return': 'return ' if has_ret else '',
>> + 'assign_ret': 's->poll_state.ret = ' if has_ret else '',
>
> See my comments above about possibly normalizing the one outlier of bdrv_co_invalidate_cache first.
>
>> + 'struct_name': struct_name(co_name),
>> + 'wrapper_name': func.name,
>> + 'ret_type': func.return_type,
>> + 'args_def': func.get_args_decl(),
>> + 'arg_names': func.get_arg_names(),
>> + 'fields': func.gen_struct_fields(),
>> + 'initializers': func.gen_struct_initializers(12),
>
> 12 looks like a magic number.
>
>> + 'args_from_s': ', '.join(f's->{a.name}' for a in func.args),
>> + }
>> +
>> + if func.args[0].type == 'BlockDriverState *':
>> + params['bs'] = 'bs'
>> + else:
>> + assert func.args[0].type == 'BdrvChild *'
>> + params['bs'] = 'child->bs'
>> +
>> + return template.format(**params)
>> +
>> +
>> +if __name__ == '__main__':
>> + import sys
>> +
>> + print(header)
>> + for func in func_decl_iter(sys.stdin.read()):
>> + print(make_wrapper(func))
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/5] block: drop bdrv_prwv
2020-05-26 21:34 ` Eric Blake
@ 2020-05-27 11:35 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-05-27 11:35 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: kwolf, fam, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
27.05.2020 00:34, Eric Blake wrote:
> On 5/25/20 5:08 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Now, when we are not more paying extra code for coroutine wrappers,
>> there no more sence in extra indirection layer: bdrv_prwv(). Let's drop
>> it and instead genereate pure bdrv_preadv() and bdrv_pwritev().
>
> Typos and grammar; I suggest:
>
> Now that we are not maintaining boilerplate code for coroutine wrappers, there is no more sense in keeping the extra indirection layer of bdrv_prwv(). Let's drop it and instead generate pure bdrv_preadv() and bdrv_pwritev().
>
>>
>> Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on
>> success, auto generated functions will instead return zero, as their
>> _co_ prototype. Still, it's simple to make the conversion safe: the
>> only external user of bdrv_pwritev() is test-bdrv-drain, and it is
>> comfortable enough with bdrv_co_pwritev() instead. So prototypes are
>> moved to local block/coroutines.h. Next, the only internal use is
>> bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on
>> success.
>
> Does returning bytes on success buy us anything useful? We don't allow partial success, so blindly returning 0 on success is no less useful. True, we'd have to audit callers to make sure we aren't doing an inadvertent semantic change.
Not so simple.. Seems we have 151 calls in 23 files:
# git grep -l '\(bdrv_pread\|bdrv_pwrite\)\>' '*.[hc]' | wc -l
23
# git grep '\(bdrv_pread\|bdrv_pwrite\)\>' '*.[hc]' | wc -l
151
Amyway, let it be another series. And such series should probably change most of calls to _co_ variants.
>
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/coroutines.h | 10 ++++-----
>> include/block/block.h | 2 --
>> block/io.c | 49 ++++++++---------------------------------
>> tests/test-bdrv-drain.c | 2 +-
>> 4 files changed, 15 insertions(+), 48 deletions(-)
>>
>
> At any rate, I think this patch is reasonable.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-27 11:36 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-25 10:07 [PATCH v4 0/5] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
2020-05-25 10:07 ` [PATCH v4 1/5] block/io: refactor coroutine wrappers Vladimir Sementsov-Ogievskiy
2020-05-26 19:24 ` Eric Blake
2020-05-25 10:07 ` [PATCH v4 2/5] block: declare some coroutine functions in block/coroutines.h Vladimir Sementsov-Ogievskiy
2020-05-26 19:50 ` Eric Blake
2020-05-25 10:07 ` [PATCH v4 3/5] block: generate coroutine-wrapper code Vladimir Sementsov-Ogievskiy
2020-05-26 20:51 ` [PATCH] fixup! " Eric Blake
2020-05-26 21:30 ` [PATCH v4 3/5] " Eric Blake
2020-05-27 11:24 ` Vladimir Sementsov-Ogievskiy
2020-05-25 10:08 ` [PATCH v4 4/5] block: drop bdrv_prwv Vladimir Sementsov-Ogievskiy
2020-05-26 21:34 ` Eric Blake
2020-05-27 11:35 ` Vladimir Sementsov-Ogievskiy
2020-05-25 10:08 ` [PATCH v4 5/5] block/io: refactor save/load vmstate Vladimir Sementsov-Ogievskiy
2020-05-26 21:36 ` Eric Blake
2020-05-25 13:14 ` [PATCH v4 0/5] coroutines: generate wrapper code no-reply
2020-05-25 13:48 ` Vladimir Sementsov-Ogievskiy
2020-05-26 20:42 ` Eric Blake
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).