* [PATCH v9 0/7] coroutines: generate wrapper code
@ 2020-09-24 18:54 Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 1/7] block: return error-code from bdrv_invalidate_cache Vladimir Sementsov-Ogievskiy
` (10 more replies)
0 siblings, 11 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-24 18:54 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake
Hi all!
The aim of the series is to reduce code-duplication and writing
parameters structure-packing by hand around coroutine function wrappers.
Benefits:
- no code duplication
- less indirection
v9: Thanks to Eric, I used commit message tweaks and rebase-conflict solving from his git.
01: add Philippe's, Stefan's r-bs
02: - add Philippe's, Stefan's r-bs
- commit message tweaks stolen from Eric's git :)
03: add Philippe's, Stefan's r-bs
04: - wording/grammar by Eric (partly, stolen from repo)
- ref new file in docs/devel/index.rst
- use 644 rights and recommended header for python script
- call gen_header() once
- rename gen_wrappers_file to gen_wrappers
05: add Stefan's r-b
06: add Philippe's, Stefan's r-bs
07: Stefan's r-b
Vladimir Sementsov-Ogievskiy (7):
block: return error-code from bdrv_invalidate_cache
block/io: refactor coroutine wrappers
block: declare some coroutine functions in block/coroutines.h
scripts: add block-coroutine-wrapper.py
block: generate coroutine-wrapper code
block: drop bdrv_prwv
block/io: refactor save/load vmstate
docs/devel/block-coroutine-wrapper.rst | 54 ++++
docs/devel/index.rst | 1 +
block/block-gen.h | 49 ++++
block/coroutines.h | 65 +++++
include/block/block.h | 34 ++-
block.c | 97 +------
block/io.c | 337 ++++---------------------
tests/test-bdrv-drain.c | 2 +-
block/meson.build | 8 +
scripts/block-coroutine-wrapper.py | 188 ++++++++++++++
10 files changed, 454 insertions(+), 381 deletions(-)
create mode 100644 docs/devel/block-coroutine-wrapper.rst
create mode 100644 block/block-gen.h
create mode 100644 block/coroutines.h
create mode 100644 scripts/block-coroutine-wrapper.py
--
2.21.3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v9 1/7] block: return error-code from bdrv_invalidate_cache
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
@ 2020-09-24 18:54 ` Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 2/7] block/io: refactor coroutine wrappers Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-24 18:54 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake, Philippe Mathieu-Daudé
This is the only coroutine wrapper from block.c and block/io.c which
doesn't return a value, so let's convert it to the common behavior, to
simplify moving to generated coroutine wrappers in a further commit.
Also, bdrv_invalidate_cache is a void function, returning error only
through **errp parameter, which is considered to be bad practice, as
it forces callers to define and propagate local_err variable, so
conversion is good anyway.
This patch leaves the conversion of .bdrv_co_invalidate_cache() driver
callbacks and bdrv_invalidate_cache_all() for another day.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/block/block.h | 2 +-
block.c | 32 ++++++++++++++++++--------------
2 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 981ab5b314..81d591dd4c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -460,7 +460,7 @@ 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);
+int bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
void bdrv_invalidate_cache_all(Error **errp);
int bdrv_inactivate_all(void);
diff --git a/block.c b/block.c
index 11ab55f80b..47b3845e14 100644
--- a/block.c
+++ b/block.c
@@ -5781,8 +5781,8 @@ void bdrv_init_with_whitelist(void)
bdrv_init();
}
-static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
- Error **errp)
+static int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
+ Error **errp)
{
BdrvChild *child, *parent;
uint64_t perm, shared_perm;
@@ -5791,14 +5791,14 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
BdrvDirtyBitmap *bm;
if (!bs->drv) {
- return;
+ return -ENOMEDIUM;
}
QLIST_FOREACH(child, &bs->children, next) {
bdrv_co_invalidate_cache(child->bs, &local_err);
if (local_err) {
error_propagate(errp, local_err);
- return;
+ return -EINVAL;
}
}
@@ -5821,7 +5821,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
if (ret < 0) {
bs->open_flags |= BDRV_O_INACTIVE;
- return;
+ return ret;
}
bdrv_set_perm(bs, perm, shared_perm);
@@ -5830,7 +5830,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
if (local_err) {
bs->open_flags |= BDRV_O_INACTIVE;
error_propagate(errp, local_err);
- return;
+ return -EINVAL;
}
}
@@ -5842,7 +5842,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
if (ret < 0) {
bs->open_flags |= BDRV_O_INACTIVE;
error_setg_errno(errp, -ret, "Could not refresh total sector count");
- return;
+ return ret;
}
}
@@ -5852,27 +5852,30 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
if (local_err) {
bs->open_flags |= BDRV_O_INACTIVE;
error_propagate(errp, local_err);
- return;
+ return -EINVAL;
}
}
}
+
+ return 0;
}
typedef struct InvalidateCacheCo {
BlockDriverState *bs;
Error **errp;
bool done;
+ int ret;
} InvalidateCacheCo;
static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque)
{
InvalidateCacheCo *ico = opaque;
- bdrv_co_invalidate_cache(ico->bs, ico->errp);
+ ico->ret = bdrv_co_invalidate_cache(ico->bs, ico->errp);
ico->done = true;
aio_wait_kick();
}
-void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
+int bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
{
Coroutine *co;
InvalidateCacheCo ico = {
@@ -5889,22 +5892,23 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
bdrv_coroutine_enter(bs, co);
BDRV_POLL_WHILE(bs, !ico.done);
}
+
+ return ico.ret;
}
void bdrv_invalidate_cache_all(Error **errp)
{
BlockDriverState *bs;
- Error *local_err = NULL;
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *aio_context = bdrv_get_aio_context(bs);
+ int ret;
aio_context_acquire(aio_context);
- bdrv_invalidate_cache(bs, &local_err);
+ ret = bdrv_invalidate_cache(bs, errp);
aio_context_release(aio_context);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (ret < 0) {
bdrv_next_cleanup(&it);
return;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v9 2/7] block/io: refactor coroutine wrappers
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 1/7] block: return error-code from bdrv_invalidate_cache Vladimir Sementsov-Ogievskiy
@ 2020-09-24 18:54 ` Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 3/7] block: declare some coroutine functions in block/coroutines.h Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-24 18:54 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake, Philippe Mathieu-Daudé
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 parameter packing and calls bdrv_run_co().
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.
This patch adds an indirection layer, but it will be compensated by
a further commit, which will drop bdrv_co_prwv together with the
is_write logic, to keep the read and write paths separate.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/io.c | 60 +++++++++++++++++++++++++++++-------------------------
1 file changed, 32 insertions(+), 28 deletions(-)
diff --git a/block/io.c b/block/io.c
index a2389bb38c..24a7de3463 100644
--- a/block/io.c
+++ b/block/io.c
@@ -933,27 +933,31 @@ 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 int coroutine_fn bdrv_rw_co_entry(void *opaque)
{
RwCo *rwco = opaque;
- if (!rwco->is_write) {
- return bdrv_co_preadv(rwco->child, rwco->offset,
- rwco->qiov->size, rwco->qiov,
- rwco->flags);
- } else {
- return bdrv_co_pwritev(rwco->child, rwco->offset,
- rwco->qiov->size, rwco->qiov,
- rwco->flags);
- }
+ return bdrv_co_prwv(rwco->child, rwco->offset, rwco->qiov,
+ rwco->is_write, rwco->flags);
}
/*
* 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)
{
RwCo rwco = {
.child = child,
@@ -971,8 +975,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);
}
/*
@@ -1021,7 +1024,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;
}
@@ -1045,7 +1048,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;
}
@@ -2449,14 +2452,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;
@@ -2494,10 +2498,10 @@ static int coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
{
BdrvCoBlockStatusData *data = opaque;
- return bdrv_co_block_status_above(data->bs, data->base,
- data->want_zero,
- data->offset, data->bytes,
- data->pnum, data->map, data->file);
+ return bdrv_co_common_block_status_above(data->bs, data->base,
+ data->want_zero,
+ data->offset, data->bytes,
+ data->pnum, data->map, data->file);
}
/*
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v9 3/7] block: declare some coroutine functions in block/coroutines.h
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 1/7] block: return error-code from bdrv_invalidate_cache Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 2/7] block/io: refactor coroutine wrappers Vladimir Sementsov-Ogievskiy
@ 2020-09-24 18:54 ` Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-24 18:54 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake, Philippe Mathieu-Daudé
We are going to keep coroutine-wrappers code (structure-packing
parameters, BDRV_POLL wrapper functions) in 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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..9ce1730a09
--- /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);
+int 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 47b3845e14..6e2bfb93d8 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>
@@ -4676,8 +4677,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;
@@ -5781,8 +5782,7 @@ void bdrv_init_with_whitelist(void)
bdrv_init();
}
-static int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
- Error **errp)
+int 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 24a7de3463..897139a8b2 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"
@@ -933,9 +934,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);
@@ -955,9 +956,9 @@ static int 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)
{
RwCo rwco = {
.child = child,
@@ -2452,7 +2453,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,
@@ -2509,12 +2510,12 @@ static int 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)
{
BdrvCoBlockStatusData data = {
.bs = bs,
@@ -2630,7 +2631,7 @@ typedef struct BdrvVmstateCo {
bool is_read;
} BdrvVmstateCo;
-static int coroutine_fn
+int coroutine_fn
bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read)
{
@@ -2663,9 +2664,8 @@ static int coroutine_fn bdrv_co_rw_vmstate_entry(void *opaque)
return bdrv_co_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);
}
-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)
{
BdrvVmstateCo data = {
.bs = bs,
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2020-09-24 18:54 ` [PATCH v9 3/7] block: declare some coroutine functions in block/coroutines.h Vladimir Sementsov-Ogievskiy
@ 2020-09-24 18:54 ` Vladimir Sementsov-Ogievskiy
2020-09-24 19:10 ` Eric Blake
2020-09-25 8:04 ` Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 5/7] block: generate coroutine-wrapper code Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
10 siblings, 2 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-24 18:54 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake
We have a very frequent pattern of creating a coroutine from a function
with several arguments:
- create a 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 or use separate bool field
- fill the struct and create coroutine from _entry function with this
struct as a parameter
- do coroutine enter and BDRV_POLL_WHILE loop
Let's reduce code duplication by generating coroutine wrappers.
This patch adds scripts/block-coroutine-wrapper.py together with some
friends, which will generate functions with declared prototypes marked
by the 'generated_co_wrapper' specifier.
The usage of new code generation is as follows:
1. define the coroutine function somewhere
int coroutine_fn bdrv_co_NAME(...) {...}
2. declare in some header file
int generated_co_wrapper bdrv_NAME(...);
with same list of parameters (generated_co_wrapper is
defined in "include/block/block.h").
3. Make sure the block_gen_c delaration in block/meson.build
mentions the file with your marker function.
Still, no function is now marked, this work is for the following
commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
docs/devel/block-coroutine-wrapper.rst | 54 +++++++
docs/devel/index.rst | 1 +
block/block-gen.h | 49 +++++++
include/block/block.h | 10 ++
block/meson.build | 8 ++
scripts/block-coroutine-wrapper.py | 188 +++++++++++++++++++++++++
6 files changed, 310 insertions(+)
create mode 100644 docs/devel/block-coroutine-wrapper.rst
create mode 100644 block/block-gen.h
create mode 100644 scripts/block-coroutine-wrapper.py
diff --git a/docs/devel/block-coroutine-wrapper.rst b/docs/devel/block-coroutine-wrapper.rst
new file mode 100644
index 0000000000..d09fff2cc5
--- /dev/null
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -0,0 +1,54 @@
+=======================
+block-coroutine-wrapper
+=======================
+
+A lot of functions in QEMU block layer (see ``block/*``) can only be
+called in coroutine context. Such functions are normally marked by the
+coroutine_fn specifier. Still, sometimes we need to call them from
+non-coroutine context; for this we need to start a coroutine, run the
+needed function from it and wait for coroutine finish in
+BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
+void* argument. So for each coroutine_fn function which needs a
+non-coroutine interface, we should define a structure to pack the
+parameters, define a separate function to unpack the parameters and
+call the original function and finally define a new interface function
+with same list of arguments as original one, which will pack the
+parameters into a struct, create a coroutine, run it and wait in
+BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
+so we have a script to generate them.
+
+Usage
+=====
+
+Assume we have defined the ``coroutine_fn`` function
+``bdrv_co_foo(<some args>)`` and need a non-coroutine interface for it,
+called ``bdrv_foo(<same args>)``. In this case the script can help. To
+trigger the generation:
+
+1. You need ``bdrv_foo`` declaration somewhere (for example, in
+ ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
+ like this:
+
+.. code-block:: c
+
+ int generated_co_wrapper bdrv_foo(<some args>);
+
+2. You need to feed this declaration to block-coroutine-wrapper script.
+ For this, add the .h (or .c) file with the declaration to the
+ ``input: files(...)`` list of ``block_gen_c`` target declaration in
+ ``block/meson.build``
+
+You are done. During the build, coroutine wrappers will be generated in
+``<BUILD_DIR>/block/block-gen.c``.
+
+Links
+=====
+
+1. The script location is ``scripts/block-coroutine-wrapper.py``.
+
+2. Generic place for private ``generated_co_wrapper`` declarations is
+ ``block/coroutines.h``, for public declarations:
+ ``include/block/block.h``
+
+3. The core API of generated coroutine wrappers is placed in
+ (not generated) ``block/block-gen.h``
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 04773ce076..cb0abe1e69 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -31,3 +31,4 @@ Contents:
reset
s390-dasd-ipl
clocks
+ block-coroutine-wrapper
diff --git a/block/block-gen.h b/block/block-gen.h
new file mode 100644
index 0000000000..f80cf4897d
--- /dev/null
+++ b/block/block-gen.h
@@ -0,0 +1,49 @@
+/*
+ * Block coroutine wrapping core, used by auto-generated block/block-gen.c
+ *
+ * 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"
+
+/* 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/include/block/block.h b/include/block/block.h
index 81d591dd4c..0f0ddc51b4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -10,6 +10,16 @@
#include "block/blockjob.h"
#include "qemu/hbitmap.h"
+/*
+ * generated_co_wrapper
+ *
+ * Function specifier, which does nothing but mark functions to be
+ * generated by scripts/block-coroutine-wrapper.py
+ *
+ * Read more in docs/devel/block-coroutine-wrapper.rst
+ */
+#define generated_co_wrapper
+
/* block.c */
typedef struct BlockDriver BlockDriver;
typedef struct BdrvChild BdrvChild;
diff --git a/block/meson.build b/block/meson.build
index a3e56b7cd1..88ad73583a 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -107,6 +107,14 @@ module_block_h = custom_target('module_block.h',
command: [module_block_py, '@OUTPUT0@', modsrc])
block_ss.add(module_block_h)
+wrapper_py = find_program('../scripts/block-coroutine-wrapper.py')
+block_gen_c = custom_target('block-gen.c',
+ output: 'block-gen.c',
+ input: files('../include/block/block.h',
+ 'coroutines.h'),
+ command: [wrapper_py, '@OUTPUT@', '@INPUT@'])
+block_ss.add(block_gen_c)
+
block_ss.add(files('stream.c'))
softmmu_ss.add(files('qapi-sysemu.c'))
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
new file mode 100644
index 0000000000..505e070660
--- /dev/null
+++ b/scripts/block-coroutine-wrapper.py
@@ -0,0 +1,188 @@
+#! /usr/bin/env python3
+"""Generate coroutine wrappers for block subsystem.
+
+The program parses one or several concatenated c files from stdin,
+searches for functions with the 'generated_co_wrapper' specifier
+and generates corresponding wrappers on stdout.
+
+Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
+
+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 sys
+import re
+import subprocess
+import json
+from typing import Iterator
+
+
+def prettify(code: str) -> str:
+ """Prettify code using clang-format if available"""
+
+ try:
+ style = json.dumps({
+ 'IndentWidth': 4,
+ 'BraceWrapping': {'AfterFunction': True},
+ 'BreakBeforeBraces': 'Custom',
+ 'SortIncludes': False,
+ 'MaxEmptyLinesToKeep': 2,
+ })
+ p = subprocess.run(['clang-format', f'-style={style}'], check=True,
+ encoding='utf-8', input=code,
+ stdout=subprocess.PIPE)
+ return p.stdout
+ except FileNotFoundError:
+ return code
+
+
+def gen_header():
+ copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
+ copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
+ copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
+ return f"""\
+/*
+ * File is generated by scripts/block-coroutine-wrapper.py
+ *
+{copyright}
+ */
+
+#include "qemu/osdep.h"
+#include "block/coroutines.h"
+#include "block/block-gen.h"
+#include "block/block_int.h"\
+"""
+
+
+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())
+ if m is None:
+ raise ValueError(f'Wrong parameter declaration: "{param_decl}"')
+ 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 = [ParamDecl(arg.strip()) for arg in args.split(',')]
+
+ def gen_list(self, format: str) -> str:
+ return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
+
+ def gen_block(self, format: str) -> str:
+ return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
+
+
+# Match wrappers declared with a generated_co_wrapper mark
+func_decl_re = re.compile(r'^int\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='int',
+ name=m.group('wrapper_name'),
+ args=m.group('args'))
+
+
+def snake_to_camel(func_name: str) -> str:
+ """
+ Convert underscore names like 'some_function_name' to camel-case like
+ 'SomeFunctionName'
+ """
+ words = func_name.split('_')
+ words = [w[0].upper() + w[1:] for w in words]
+ return ''.join(words)
+
+
+def gen_wrapper(func: FuncDecl) -> str:
+ assert func.name.startswith('bdrv_')
+ assert not func.name.startswith('bdrv_co_')
+ assert func.return_type == 'int'
+ assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *']
+
+ name = 'bdrv_co_' + func.name[5:]
+ bs = 'bs' if func.args[0].type == 'BlockDriverState *' else 'child->bs'
+ struct_name = snake_to_camel(name)
+
+ return f"""\
+/*
+ * Wrappers for {name}
+ */
+
+typedef struct {struct_name} {{
+ BdrvPollCo poll_state;
+{ func.gen_block(' {decl};') }
+}} {struct_name};
+
+static void coroutine_fn {name}_entry(void *opaque)
+{{
+ {struct_name} *s = opaque;
+
+ s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
+ s->poll_state.in_progress = false;
+
+ aio_wait_kick();
+}}
+
+int {func.name}({ func.gen_list('{decl}') })
+{{
+ if (qemu_in_coroutine()) {{
+ return {name}({ func.gen_list('{name}') });
+ }} else {{
+ {struct_name} s = {{
+ .poll_state.bs = {bs},
+ .poll_state.in_progress = true,
+
+{ func.gen_block(' .{name} = {name},') }
+ }};
+
+ s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
+
+ return bdrv_poll_co(&s.poll_state);
+ }}
+}}"""
+
+
+def gen_wrappers(input_code: str) -> str:
+ res = ''
+ for func in func_decl_iter(input_code):
+ res += '\n\n\n'
+ res += gen_wrapper(func)
+
+ return prettify(res) # prettify to wrap long lines
+
+
+if __name__ == '__main__':
+ if len(sys.argv) < 3:
+ exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
+
+ with open(sys.argv[1], 'w') as f_out:
+ f_out.write(gen_header())
+ for fname in sys.argv[2:]:
+ with open(fname) as f_in:
+ f_out.write(gen_wrappers(f_in.read()))
+ f_out.write('\n')
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v9 5/7] block: generate coroutine-wrapper code
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2020-09-24 18:54 ` [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py Vladimir Sementsov-Ogievskiy
@ 2020-09-24 18:54 ` Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 6/7] block: drop bdrv_prwv Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-24 18:54 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake
Use code generation implemented in previous commit to generated
coroutine wrappers in block.c and block/io.c
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/coroutines.h | 6 +-
include/block/block.h | 16 ++--
block.c | 73 ---------------
block/io.c | 212 ------------------------------------------
4 files changed, 13 insertions(+), 294 deletions(-)
diff --git a/block/coroutines.h b/block/coroutines.h
index 9ce1730a09..c62b3a2697 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -34,7 +34,7 @@ int 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 +47,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 +60,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 0f0ddc51b4..f2d85f2cf1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -403,8 +403,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);
@@ -446,7 +447,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
@@ -470,12 +472,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 */
-int bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
+int 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);
@@ -490,7 +493,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/block.c b/block.c
index 6e2bfb93d8..de056c695a 100644
--- a/block.c
+++ b/block.c
@@ -4691,43 +4691,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
@@ -5860,42 +5823,6 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
return 0;
}
-typedef struct InvalidateCacheCo {
- BlockDriverState *bs;
- Error **errp;
- bool done;
- int ret;
-} InvalidateCacheCo;
-
-static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque)
-{
- InvalidateCacheCo *ico = opaque;
- ico->ret = bdrv_co_invalidate_cache(ico->bs, ico->errp);
- ico->done = true;
- aio_wait_kick();
-}
-
-int 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);
- }
-
- return ico.ret;
-}
-
void bdrv_invalidate_cache_all(Error **errp)
{
BlockDriverState *bs;
diff --git a/block/io.c b/block/io.c
index 897139a8b2..c1360ba57d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -890,50 +890,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
return 0;
}
-typedef int coroutine_fn BdrvRequestEntry(void *opaque);
-typedef struct BdrvRunCo {
- BdrvRequestEntry *entry;
- void *opaque;
- int ret;
- bool done;
- Coroutine *co; /* Coroutine, running bdrv_run_co_entry, for debugging */
-} BdrvRunCo;
-
-static void coroutine_fn bdrv_run_co_entry(void *opaque)
-{
- BdrvRunCo *arg = opaque;
-
- arg->ret = arg->entry(arg->opaque);
- arg->done = true;
- aio_wait_kick();
-}
-
-static int bdrv_run_co(BlockDriverState *bs, BdrvRequestEntry *entry,
- void *opaque)
-{
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- return entry(opaque);
- } else {
- BdrvRunCo s = { .entry = entry, .opaque = opaque };
-
- s.co = qemu_coroutine_create(bdrv_run_co_entry, &s);
- bdrv_coroutine_enter(bs, s.co);
-
- BDRV_POLL_WHILE(bs, !s.done);
-
- return s.ret;
- }
-}
-
-typedef struct RwCo {
- BdrvChild *child;
- int64_t offset;
- QEMUIOVector *qiov;
- bool is_write;
- BdrvRequestFlags flags;
-} RwCo;
-
int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
QEMUIOVector *qiov, bool is_write,
BdrvRequestFlags flags)
@@ -945,32 +901,6 @@ int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
}
}
-static int coroutine_fn bdrv_rw_co_entry(void *opaque)
-{
- RwCo *rwco = opaque;
-
- return bdrv_co_prwv(rwco->child, rwco->offset, rwco->qiov,
- rwco->is_write, rwco->flags);
-}
-
-/*
- * Process a vectored synchronous request using coroutines
- */
-int bdrv_prwv(BdrvChild *child, int64_t offset,
- QEMUIOVector *qiov, bool is_write,
- BdrvRequestFlags flags)
-{
- RwCo rwco = {
- .child = child,
- .offset = offset,
- .qiov = qiov,
- .is_write = is_write,
- .flags = flags,
- };
-
- return bdrv_run_co(child->bs, bdrv_rw_co_entry, &rwco);
-}
-
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags)
{
@@ -2247,18 +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;
-} BdrvCoBlockStatusData;
-
/*
* Returns the allocation status of the specified sectors.
* Drivers not implementing the functionality are assumed to not support
@@ -2494,43 +2412,6 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
return ret;
}
-/* Coroutine wrapper for bdrv_block_status_above() */
-static int coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
-{
- BdrvCoBlockStatusData *data = opaque;
-
- return bdrv_co_common_block_status_above(data->bs, data->base,
- data->want_zero,
- data->offset, data->bytes,
- data->pnum, data->map, data->file);
-}
-
-/*
- * 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)
-{
- BdrvCoBlockStatusData data = {
- .bs = bs,
- .base = base,
- .want_zero = want_zero,
- .offset = offset,
- .bytes = bytes,
- .pnum = pnum,
- .map = map,
- .file = file,
- };
-
- return bdrv_run_co(bs, bdrv_block_status_above_co_entry, &data);
-}
-
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriverState **file)
@@ -2624,13 +2505,6 @@ int bdrv_is_allocated_above(BlockDriverState *top,
return 0;
}
-typedef struct BdrvVmstateCo {
- BlockDriverState *bs;
- QEMUIOVector *qiov;
- int64_t pos;
- bool is_read;
-} BdrvVmstateCo;
-
int coroutine_fn
bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read)
@@ -2657,26 +2531,6 @@ bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
return ret;
}
-static int coroutine_fn bdrv_co_rw_vmstate_entry(void *opaque)
-{
- BdrvVmstateCo *co = opaque;
-
- return bdrv_co_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);
-}
-
-int bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
- bool is_read)
-{
- BdrvVmstateCo data = {
- .bs = bs,
- .qiov = qiov,
- .pos = pos,
- .is_read = is_read,
- };
-
- return bdrv_run_co(bs, bdrv_co_rw_vmstate_entry, &data);
-}
-
int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
int64_t pos, int size)
{
@@ -2752,11 +2606,6 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb)
/**************************************************************/
/* Coroutine block device emulation */
-static int coroutine_fn bdrv_flush_co_entry(void *opaque)
-{
- return bdrv_co_flush(opaque);
-}
-
int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
{
BdrvChild *primary_child = bdrv_primary_child(bs);
@@ -2880,24 +2729,6 @@ early_exit:
return ret;
}
-int bdrv_flush(BlockDriverState *bs)
-{
- return bdrv_run_co(bs, bdrv_flush_co_entry, bs);
-}
-
-typedef struct DiscardCo {
- BdrvChild *child;
- int64_t offset;
- int64_t bytes;
-} DiscardCo;
-
-static int coroutine_fn bdrv_pdiscard_co_entry(void *opaque)
-{
- DiscardCo *rwco = opaque;
-
- return bdrv_co_pdiscard(rwco->child, rwco->offset, rwco->bytes);
-}
-
int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
int64_t bytes)
{
@@ -3012,17 +2843,6 @@ out:
return ret;
}
-int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes)
-{
- DiscardCo rwco = {
- .child = child,
- .offset = offset,
- .bytes = bytes,
- };
-
- return bdrv_run_co(child->bs, bdrv_pdiscard_co_entry, &rwco);
-}
-
int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
{
BlockDriver *drv = bs->drv;
@@ -3424,35 +3244,3 @@ out:
return ret;
}
-
-typedef struct TruncateCo {
- BdrvChild *child;
- int64_t offset;
- bool exact;
- PreallocMode prealloc;
- BdrvRequestFlags flags;
- Error **errp;
-} TruncateCo;
-
-static int coroutine_fn bdrv_truncate_co_entry(void *opaque)
-{
- TruncateCo *tco = opaque;
-
- return bdrv_co_truncate(tco->child, tco->offset, tco->exact,
- tco->prealloc, tco->flags, tco->errp);
-}
-
-int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
- PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
-{
- TruncateCo tco = {
- .child = child,
- .offset = offset,
- .exact = exact,
- .prealloc = prealloc,
- .flags = flags,
- .errp = errp,
- };
-
- return bdrv_run_co(child->bs, bdrv_truncate_co_entry, &tco);
-}
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v9 6/7] block: drop bdrv_prwv
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2020-09-24 18:54 ` [PATCH v9 5/7] block: generate coroutine-wrapper code Vladimir Sementsov-Ogievskiy
@ 2020-09-24 18:54 ` Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 7/7] block/io: refactor save/load vmstate Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-24 18:54 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake, Philippe Mathieu-Daudé
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.
Of course, it would be great to convert bdrv_pread() and bdrv_pwrite()
to return 0 on success. But this requires audit (and probably
conversion) of all their users, let's leave it for another day
refactoring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.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 c62b3a2697..6c63a819c9 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -31,12 +31,12 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
BdrvCheckResult *res, BdrvCheckMode fix);
int 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 f2d85f2cf1..eef4cceaf0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -383,9 +383,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 c1360ba57d..cd5b689473 100644
--- a/block/io.c
+++ b/block/io.c
@@ -890,23 +890,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);
}
/*
@@ -950,41 +938,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:
@@ -995,13 +961,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.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v9 7/7] block/io: refactor save/load vmstate
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2020-09-24 18:54 ` [PATCH v9 6/7] block: drop bdrv_prwv Vladimir Sementsov-Ogievskiy
@ 2020-09-24 18:54 ` Vladimir Sementsov-Ogievskiy
2020-09-24 19:00 ` [PATCH v9 0/7] coroutines: generate wrapper code Eric Blake
` (3 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-24 18:54 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake
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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/coroutines.h | 10 +++----
include/block/block.h | 6 ++--
block/io.c | 68 ++++++++++++++++++++++---------------------
3 files changed, 43 insertions(+), 41 deletions(-)
diff --git a/block/coroutines.h b/block/coroutines.h
index 6c63a819c9..f69179f5ef 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -57,11 +57,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 eef4cceaf0..8b87df69a1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -572,8 +572,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 cd5b689473..449b99b92c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2475,67 +2475,69 @@ 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;
BlockDriverState *child_bs = bdrv_primary_bs(bs);
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 (child_bs) {
- ret = bdrv_co_rw_vmstate(child_bs, qiov, pos, is_read);
+ ret = bdrv_co_readv_vmstate(child_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;
+ BlockDriverState *child_bs = bdrv_primary_bs(bs);
+ 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_save_vmstate) {
+ ret = drv->bdrv_save_vmstate(bs, qiov, pos);
+ } else if (child_bs) {
+ ret = bdrv_co_writev_vmstate(child_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.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v9 0/7] coroutines: generate wrapper code
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2020-09-24 18:54 ` [PATCH v9 7/7] block/io: refactor save/load vmstate Vladimir Sementsov-Ogievskiy
@ 2020-09-24 19:00 ` Eric Blake
2020-09-24 20:32 ` no-reply
` (2 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-09-24 19:00 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
On 9/24/20 1:54 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> The aim of the series is to reduce code-duplication and writing
> parameters structure-packing by hand around coroutine function wrappers.
>
> Benefits:
> - no code duplication
> - less indirection
>
> v9: Thanks to Eric, I used commit message tweaks and rebase-conflict solving from his git.
> 01: add Philippe's, Stefan's r-bs
> 02: - add Philippe's, Stefan's r-bs
> - commit message tweaks stolen from Eric's git :)
LOL: "stolen" makes it sound like a crime was committed ;) I find it to
be one of the joys of open source when my ideas show up in someone
else's work when properly attributed, as you did here. [Maybe the
recent push towards a conscious language initiative has let me hone in
on something that turned out humorous to me, but might not be as obvious
to someone less fluent in English idioms]
At any rate, I'm glad my rebase efforts helped.
> 03: add Philippe's, Stefan's r-bs
> 04: - wording/grammar by Eric (partly, stolen from repo)
> - ref new file in docs/devel/index.rst
> - use 644 rights and recommended header for python script
> - call gen_header() once
> - rename gen_wrappers_file to gen_wrappers
> 05: add Stefan's r-b
> 06: add Philippe's, Stefan's r-bs
> 07: Stefan's r-b
>
--
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 v9 4/7] scripts: add block-coroutine-wrapper.py
2020-09-24 18:54 ` [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py Vladimir Sementsov-Ogievskiy
@ 2020-09-24 19:10 ` Eric Blake
2020-09-25 8:04 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-09-24 19:10 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
On 9/24/20 1:54 PM, Vladimir Sementsov-Ogievskiy wrote:
> We have a very frequent pattern of creating a coroutine from a function
> with several arguments:
>
> - create a 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 or use separate bool field
> - fill the struct and create coroutine from _entry function with this
> struct as a parameter
> - do coroutine enter and BDRV_POLL_WHILE loop
>
> Let's reduce code duplication by generating coroutine wrappers.
>
> This patch adds scripts/block-coroutine-wrapper.py together with some
> friends, which will generate functions with declared prototypes marked
> by the 'generated_co_wrapper' specifier.
>
> The usage of new code generation is as follows:
>
> 1. define the coroutine function somewhere
>
> int coroutine_fn bdrv_co_NAME(...) {...}
>
> 2. declare in some header file
>
> int generated_co_wrapper bdrv_NAME(...);
>
> with same list of parameters (generated_co_wrapper is
> defined in "include/block/block.h").
>
> 3. Make sure the block_gen_c delaration in block/meson.build
declaration
> mentions the file with your marker function.
>
> Still, no function is now marked, this work is for the following
> commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> +++ b/docs/devel/block-coroutine-wrapper.rst
> @@ -0,0 +1,54 @@
> +=======================
> +block-coroutine-wrapper
> +=======================
> +
> +A lot of functions in QEMU block layer (see ``block/*``) can only be
> +called in coroutine context. Such functions are normally marked by the
> +coroutine_fn specifier. Still, sometimes we need to call them from
> +non-coroutine context; for this we need to start a coroutine, run the
> +needed function from it and wait for coroutine finish in
to finish in a
> +BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
> +void* argument. So for each coroutine_fn function which needs a
> +non-coroutine interface, we should define a structure to pack the
> +parameters, define a separate function to unpack the parameters and
> +call the original function and finally define a new interface function
> +with same list of arguments as original one, which will pack the
> +parameters into a struct, create a coroutine, run it and wait in
> +BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
> +so we have a script to generate them.
> +
> +++ b/scripts/block-coroutine-wrapper.py
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 v9 0/7] coroutines: generate wrapper code
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2020-09-24 19:00 ` [PATCH v9 0/7] coroutines: generate wrapper code Eric Blake
@ 2020-09-24 20:32 ` no-reply
2020-09-25 8:04 ` Vladimir Sementsov-Ogievskiy
2020-09-25 8:32 ` [PATCH 0.5/7] include/block/block.h: drop non-ascii quotation mark Vladimir Sementsov-Ogievskiy
2020-09-25 16:02 ` [PATCH v9 0/7] coroutines: generate wrapper code Stefan Hajnoczi
10 siblings, 1 reply; 17+ messages in thread
From: no-reply @ 2020-09-24 20:32 UTC (permalink / raw)
To: vsementsov
Cc: fam, kwolf, vsementsov, ehabkost, qemu-block, qemu-devel, mreitz,
stefanha, crosa, den
Patchew URL: https://patchew.org/QEMU/20200924185414.28642-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 ===
C linker for the host machine: cc ld.bfd 2.27-43
Host machine cpu family: x86_64
Host machine cpu: x86_64
../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
Program sh found: YES
Program python3 found: YES (/usr/bin/python3)
Configuring ninjatool using configuration
---
return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128)
Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp-uncond.c.inc'.
make: *** [block/block-gen.c.stamp] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 709, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=ea922a0a6ce34dfc90f59bf1b059b9d5', '-u', '1001', '--security-opt', 'seccomp=unconfined', '-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-rctf_xsm/src/docker-src.2020-09-24-16.29.53.14429:/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=ea922a0a6ce34dfc90f59bf1b059b9d5
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-rctf_xsm/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 3m4.047s
user 0m21.648s
The full log is available at
http://patchew.org/logs/20200924185414.28642-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 v9 0/7] coroutines: generate wrapper code
2020-09-24 20:32 ` no-reply
@ 2020-09-25 8:04 ` Vladimir Sementsov-Ogievskiy
2020-09-25 12:57 ` Eric Blake
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-25 8:04 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
eblake
24.09.2020 23:32, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200924185414.28642-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 ===
>
> C linker for the host machine: cc ld.bfd 2.27-43
> Host machine cpu family: x86_64
> Host machine cpu: x86_64
> ../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
> Program sh found: YES
> Program python3 found: YES (/usr/bin/python3)
> Configuring ninjatool using configuration
> ---
> return codecs.ascii_decode(input, self.errors)[0]
> UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128)
> Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp-uncond.c.inc'.
> make: *** [block/block-gen.c.stamp] Error 1
> make: *** Waiting for unfinished jobs....
> Traceback (most recent call last):
> File "./tests/docker/docker.py", line 709, in <module>
> ---
> raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=ea922a0a6ce34dfc90f59bf1b059b9d5', '-u', '1001', '--security-opt', 'seccomp=unconfined', '-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-rctf_xsm/src/docker-src.2020-09-24-16.29.53.14429:/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=ea922a0a6ce34dfc90f59bf1b059b9d5
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-rctf_xsm/src'
> make: *** [docker-run-test-quick@centos7] Error 2
>
> real 3m4.047s
> user 0m21.648s
>
>
> The full log is available at
> http://patchew.org/logs/20200924185414.28642-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
>
Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp.c.inc'.
Traceback (most recent call last):
File "/tmp/qemu-test/src/block/../scripts/block-coroutine-wrapper.py", line 187, in <module>
f_out.write(gen_wrappers(f_in.read()))
File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode
return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 11406: ordinal not in range(128)
Interesting:
[root@kvm up-coroutine-wrapper]# grep --color='auto' -P -n '[^\x00-\x7F]' include/block/block.h
307: * Child from which to read all data that isn’t allocated in the
^
The file really contains one non-ascii symbol. I think it worth a separate patch. Still, it shouldn't break build process. On my system it works as is, probably unicode is default for me.
Aha, from "open" specification:
if encoding is not specified the encoding used is platform dependent: locale.getpreferredencoding(False) is called to get the current locale encoding.
Is it ok, that utf-8 is not default on test system?
So, possible solutions are:
1. Enforce utf-8 io in scripts/block-coroutine-wrapper.py (patch 4)
2. Drop non-ascii quotation mark from block.h
3. Fix the test system default to be utf-8
Do we want them all?
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py
2020-09-24 18:54 ` [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py Vladimir Sementsov-Ogievskiy
2020-09-24 19:10 ` Eric Blake
@ 2020-09-25 8:04 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-25 8:04 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
eblake
24.09.2020 21:54, Vladimir Sementsov-Ogievskiy wrote:
> We have a very frequent pattern of creating a coroutine from a function
> with several arguments:
>
> - create a 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 or use separate bool field
> - fill the struct and create coroutine from _entry function with this
> struct as a parameter
> - do coroutine enter and BDRV_POLL_WHILE loop
>
> Let's reduce code duplication by generating coroutine wrappers.
>
> This patch adds scripts/block-coroutine-wrapper.py together with some
> friends, which will generate functions with declared prototypes marked
> by the 'generated_co_wrapper' specifier.
>
> The usage of new code generation is as follows:
>
> 1. define the coroutine function somewhere
>
> int coroutine_fn bdrv_co_NAME(...) {...}
>
> 2. declare in some header file
>
> int generated_co_wrapper bdrv_NAME(...);
>
> with same list of parameters (generated_co_wrapper is
> defined in "include/block/block.h").
>
> 3. Make sure the block_gen_c delaration in block/meson.build
> mentions the file with your marker function.
>
> Still, no function is now marked, this work is for the following
> commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> docs/devel/block-coroutine-wrapper.rst | 54 +++++++
> docs/devel/index.rst | 1 +
> block/block-gen.h | 49 +++++++
> include/block/block.h | 10 ++
> block/meson.build | 8 ++
> scripts/block-coroutine-wrapper.py | 188 +++++++++++++++++++++++++
> 6 files changed, 310 insertions(+)
> create mode 100644 docs/devel/block-coroutine-wrapper.rst
> create mode 100644 block/block-gen.h
> create mode 100644 scripts/block-coroutine-wrapper.py
>
> diff --git a/docs/devel/block-coroutine-wrapper.rst b/docs/devel/block-coroutine-wrapper.rst
> new file mode 100644
> index 0000000000..d09fff2cc5
> --- /dev/null
> +++ b/docs/devel/block-coroutine-wrapper.rst
> @@ -0,0 +1,54 @@
> +=======================
> +block-coroutine-wrapper
> +=======================
> +
> +A lot of functions in QEMU block layer (see ``block/*``) can only be
> +called in coroutine context. Such functions are normally marked by the
> +coroutine_fn specifier. Still, sometimes we need to call them from
> +non-coroutine context; for this we need to start a coroutine, run the
> +needed function from it and wait for coroutine finish in
> +BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
> +void* argument. So for each coroutine_fn function which needs a
> +non-coroutine interface, we should define a structure to pack the
> +parameters, define a separate function to unpack the parameters and
> +call the original function and finally define a new interface function
> +with same list of arguments as original one, which will pack the
> +parameters into a struct, create a coroutine, run it and wait in
> +BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
> +so we have a script to generate them.
> +
> +Usage
> +=====
> +
> +Assume we have defined the ``coroutine_fn`` function
> +``bdrv_co_foo(<some args>)`` and need a non-coroutine interface for it,
> +called ``bdrv_foo(<same args>)``. In this case the script can help. To
> +trigger the generation:
> +
> +1. You need ``bdrv_foo`` declaration somewhere (for example, in
> + ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
> + like this:
> +
> +.. code-block:: c
> +
> + int generated_co_wrapper bdrv_foo(<some args>);
> +
> +2. You need to feed this declaration to block-coroutine-wrapper script.
> + For this, add the .h (or .c) file with the declaration to the
> + ``input: files(...)`` list of ``block_gen_c`` target declaration in
> + ``block/meson.build``
> +
> +You are done. During the build, coroutine wrappers will be generated in
> +``<BUILD_DIR>/block/block-gen.c``.
> +
> +Links
> +=====
> +
> +1. The script location is ``scripts/block-coroutine-wrapper.py``.
> +
> +2. Generic place for private ``generated_co_wrapper`` declarations is
> + ``block/coroutines.h``, for public declarations:
> + ``include/block/block.h``
> +
> +3. The core API of generated coroutine wrappers is placed in
> + (not generated) ``block/block-gen.h``
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index 04773ce076..cb0abe1e69 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -31,3 +31,4 @@ Contents:
> reset
> s390-dasd-ipl
> clocks
> + block-coroutine-wrapper
> diff --git a/block/block-gen.h b/block/block-gen.h
> new file mode 100644
> index 0000000000..f80cf4897d
> --- /dev/null
> +++ b/block/block-gen.h
> @@ -0,0 +1,49 @@
> +/*
> + * Block coroutine wrapping core, used by auto-generated block/block-gen.c
> + *
> + * 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"
> +
> +/* 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/include/block/block.h b/include/block/block.h
> index 81d591dd4c..0f0ddc51b4 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -10,6 +10,16 @@
> #include "block/blockjob.h"
> #include "qemu/hbitmap.h"
>
> +/*
> + * generated_co_wrapper
> + *
> + * Function specifier, which does nothing but mark functions to be
> + * generated by scripts/block-coroutine-wrapper.py
> + *
> + * Read more in docs/devel/block-coroutine-wrapper.rst
> + */
> +#define generated_co_wrapper
> +
> /* block.c */
> typedef struct BlockDriver BlockDriver;
> typedef struct BdrvChild BdrvChild;
> diff --git a/block/meson.build b/block/meson.build
> index a3e56b7cd1..88ad73583a 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -107,6 +107,14 @@ module_block_h = custom_target('module_block.h',
> command: [module_block_py, '@OUTPUT0@', modsrc])
> block_ss.add(module_block_h)
>
> +wrapper_py = find_program('../scripts/block-coroutine-wrapper.py')
> +block_gen_c = custom_target('block-gen.c',
> + output: 'block-gen.c',
> + input: files('../include/block/block.h',
> + 'coroutines.h'),
> + command: [wrapper_py, '@OUTPUT@', '@INPUT@'])
> +block_ss.add(block_gen_c)
> +
> block_ss.add(files('stream.c'))
>
> softmmu_ss.add(files('qapi-sysemu.c'))
> diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
> new file mode 100644
> index 0000000000..505e070660
> --- /dev/null
> +++ b/scripts/block-coroutine-wrapper.py
> @@ -0,0 +1,188 @@
> +#! /usr/bin/env python3
> +"""Generate coroutine wrappers for block subsystem.
> +
> +The program parses one or several concatenated c files from stdin,
> +searches for functions with the 'generated_co_wrapper' specifier
> +and generates corresponding wrappers on stdout.
> +
> +Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
> +
> +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 sys
> +import re
> +import subprocess
> +import json
> +from typing import Iterator
> +
> +
> +def prettify(code: str) -> str:
> + """Prettify code using clang-format if available"""
> +
> + try:
> + style = json.dumps({
> + 'IndentWidth': 4,
> + 'BraceWrapping': {'AfterFunction': True},
> + 'BreakBeforeBraces': 'Custom',
> + 'SortIncludes': False,
> + 'MaxEmptyLinesToKeep': 2,
> + })
> + p = subprocess.run(['clang-format', f'-style={style}'], check=True,
> + encoding='utf-8', input=code,
> + stdout=subprocess.PIPE)
> + return p.stdout
> + except FileNotFoundError:
> + return code
> +
> +
> +def gen_header():
> + copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
> + copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
> + copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
> + return f"""\
> +/*
> + * File is generated by scripts/block-coroutine-wrapper.py
> + *
> +{copyright}
> + */
> +
> +#include "qemu/osdep.h"
> +#include "block/coroutines.h"
> +#include "block/block-gen.h"
> +#include "block/block_int.h"\
> +"""
> +
> +
> +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())
> + if m is None:
> + raise ValueError(f'Wrong parameter declaration: "{param_decl}"')
> + 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 = [ParamDecl(arg.strip()) for arg in args.split(',')]
> +
> + def gen_list(self, format: str) -> str:
> + return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
> +
> + def gen_block(self, format: str) -> str:
> + return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
> +
> +
> +# Match wrappers declared with a generated_co_wrapper mark
> +func_decl_re = re.compile(r'^int\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='int',
> + name=m.group('wrapper_name'),
> + args=m.group('args'))
> +
> +
> +def snake_to_camel(func_name: str) -> str:
> + """
> + Convert underscore names like 'some_function_name' to camel-case like
> + 'SomeFunctionName'
> + """
> + words = func_name.split('_')
> + words = [w[0].upper() + w[1:] for w in words]
> + return ''.join(words)
> +
> +
> +def gen_wrapper(func: FuncDecl) -> str:
> + assert func.name.startswith('bdrv_')
> + assert not func.name.startswith('bdrv_co_')
> + assert func.return_type == 'int'
> + assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *']
> +
> + name = 'bdrv_co_' + func.name[5:]
> + bs = 'bs' if func.args[0].type == 'BlockDriverState *' else 'child->bs'
> + struct_name = snake_to_camel(name)
> +
> + return f"""\
> +/*
> + * Wrappers for {name}
> + */
> +
> +typedef struct {struct_name} {{
> + BdrvPollCo poll_state;
> +{ func.gen_block(' {decl};') }
> +}} {struct_name};
> +
> +static void coroutine_fn {name}_entry(void *opaque)
> +{{
> + {struct_name} *s = opaque;
> +
> + s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
> + s->poll_state.in_progress = false;
> +
> + aio_wait_kick();
> +}}
> +
> +int {func.name}({ func.gen_list('{decl}') })
> +{{
> + if (qemu_in_coroutine()) {{
> + return {name}({ func.gen_list('{name}') });
> + }} else {{
> + {struct_name} s = {{
> + .poll_state.bs = {bs},
> + .poll_state.in_progress = true,
> +
> +{ func.gen_block(' .{name} = {name},') }
> + }};
> +
> + s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
> +
> + return bdrv_poll_co(&s.poll_state);
> + }}
> +}}"""
> +
> +
> +def gen_wrappers(input_code: str) -> str:
> + res = ''
> + for func in func_decl_iter(input_code):
> + res += '\n\n\n'
> + res += gen_wrapper(func)
> +
> + return prettify(res) # prettify to wrap long lines
> +
> +
> +if __name__ == '__main__':
> + if len(sys.argv) < 3:
> + exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
> +
> + with open(sys.argv[1], 'w') as f_out:
> + f_out.write(gen_header())
> + for fname in sys.argv[2:]:
> + with open(fname) as f_in:
> + f_out.write(gen_wrappers(f_in.read()))
> + f_out.write('\n')
>
probably we want encoding='utf-8' for both open calls.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0.5/7] include/block/block.h: drop non-ascii quotation mark
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2020-09-24 20:32 ` no-reply
@ 2020-09-25 8:32 ` Vladimir Sementsov-Ogievskiy
2020-09-25 12:58 ` Eric Blake
2020-09-25 16:02 ` [PATCH v9 0/7] coroutines: generate wrapper code Stefan Hajnoczi
10 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-25 8:32 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, crosa, ehabkost, fam, stefanha, mreitz, kwolf, den,
vsementsov, eblake
This is the only non-ascii character in the file and it doesn't really
needed here. Let's use normal "'" symbol for consistency with the rest
11 occurrences of "'" in the file.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/block.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/block/block.h b/include/block/block.h
index 8b87df69a1..ce2ac39299 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -304,7 +304,7 @@ enum BdrvChildRoleBits {
BDRV_CHILD_FILTERED = (1 << 2),
/*
- * Child from which to read all data that isn’t allocated in the
+ * Child from which to read all data that isn't allocated in the
* parent (i.e., the backing child); such data is copied to the
* parent through COW (and optionally COR).
* This field is mutually exclusive with DATA, METADATA, and
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v9 0/7] coroutines: generate wrapper code
2020-09-25 8:04 ` Vladimir Sementsov-Ogievskiy
@ 2020-09-25 12:57 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-09-25 12:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: fam, kwolf, ehabkost, qemu-block, mreitz, stefanha, crosa, den
On 9/25/20 3:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> 24.09.2020 23:32, no-reply@patchew.org wrote:
>> Patchew URL:
>> https://patchew.org/QEMU/20200924185414.28642-1-vsementsov@virtuozzo.com/
>>
>> Program python3 found: YES (/usr/bin/python3)
>> Configuring ninjatool using configuration
>> ---
>> return codecs.ascii_decode(input, self.errors)[0]
>> UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position
>> 11406: ordinal not in range(128)
> Generating 'libqemu-aarch64-softmmu.fa.p/decode-vfp.c.inc'.
> Traceback (most recent call last):
> File
> "/tmp/qemu-test/src/block/../scripts/block-coroutine-wrapper.py", line
> 187, in <module>
> f_out.write(gen_wrappers(f_in.read()))
> File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode
> return codecs.ascii_decode(input, self.errors)[0]
> UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position
> 11406: ordinal not in range(128)
>
>
> Interesting:
>
> [root@kvm up-coroutine-wrapper]# grep --color='auto' -P -n
> '[^\x00-\x7F]' include/block/block.h
> 307: * Child from which to read all data that isn’t allocated in the
> ^
>
> The file really contains one non-ascii symbol. I think it worth a
> separate patch. Still, it shouldn't break build process. On my system it
> works as is, probably unicode is default for me.
Python 3 has had an interesting history when it comes to 8-bit cleanness
by default. Which means we DO have to be explicit about utf8.
>
> Aha, from "open" specification:
>
> if encoding is not specified the encoding used is platform
> dependent: locale.getpreferredencoding(False) is called to get the
> current locale encoding.
>
>
>
> Is it ok, that utf-8 is not default on test system?
It's intentional.
>
> So, possible solutions are:
>
> 1. Enforce utf-8 io in scripts/block-coroutine-wrapper.py (patch 4)
Yes, we should do that regardless (we do it in our other python scripts).
> 2. Drop non-ascii quotation mark from block.h
Yes, we should do that as well (it's only in a comment, but it is
inconsistent).
> 3. Fix the test system default to be utf-8
No. That one we want to keep where it is, because it helps us flush out
these sorts of issues.
>
> Do we want them all?
>
--
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 0.5/7] include/block/block.h: drop non-ascii quotation mark
2020-09-25 8:32 ` [PATCH 0.5/7] include/block/block.h: drop non-ascii quotation mark Vladimir Sementsov-Ogievskiy
@ 2020-09-25 12:58 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2020-09-25 12:58 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: fam, kwolf, ehabkost, qemu-devel, mreitz, stefanha, crosa, den
On 9/25/20 3:32 AM, Vladimir Sementsov-Ogievskiy wrote:
> This is the only non-ascii character in the file and it doesn't really
> needed here. Let's use normal "'" symbol for consistency with the rest
> 11 occurrences of "'" in the file.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> include/block/block.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 8b87df69a1..ce2ac39299 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -304,7 +304,7 @@ enum BdrvChildRoleBits {
> BDRV_CHILD_FILTERED = (1 << 2),
>
> /*
> - * Child from which to read all data that isn’t allocated in the
> + * Child from which to read all data that isn't allocated in the
> * parent (i.e., the backing child); such data is copied to the
> * parent through COW (and optionally COR).
> * This field is mutually exclusive with DATA, METADATA, and
>
--
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 v9 0/7] coroutines: generate wrapper code
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2020-09-25 8:32 ` [PATCH 0.5/7] include/block/block.h: drop non-ascii quotation mark Vladimir Sementsov-Ogievskiy
@ 2020-09-25 16:02 ` Stefan Hajnoczi
10 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2020-09-25 16:02 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: fam, kwolf, ehabkost, qemu-block, qemu-devel, mreitz, crosa, den
[-- Attachment #1: Type: text/plain, Size: 550 bytes --]
On Thu, Sep 24, 2020 at 09:54:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> The aim of the series is to reduce code-duplication and writing
> parameters structure-packing by hand around coroutine function wrappers.
>
> Benefits:
> - no code duplication
> - less indirection
>
> v9: Thanks to Eric, I used commit message tweaks and rebase-conflict solving from his git.
Thanks, applied to my block tree with the encoding='utf-8' and
spelling/grammar fixes:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-09-25 16:25 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-24 18:54 [PATCH v9 0/7] coroutines: generate wrapper code Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 1/7] block: return error-code from bdrv_invalidate_cache Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 2/7] block/io: refactor coroutine wrappers Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 3/7] block: declare some coroutine functions in block/coroutines.h Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py Vladimir Sementsov-Ogievskiy
2020-09-24 19:10 ` Eric Blake
2020-09-25 8:04 ` Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 5/7] block: generate coroutine-wrapper code Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 6/7] block: drop bdrv_prwv Vladimir Sementsov-Ogievskiy
2020-09-24 18:54 ` [PATCH v9 7/7] block/io: refactor save/load vmstate Vladimir Sementsov-Ogievskiy
2020-09-24 19:00 ` [PATCH v9 0/7] coroutines: generate wrapper code Eric Blake
2020-09-24 20:32 ` no-reply
2020-09-25 8:04 ` Vladimir Sementsov-Ogievskiy
2020-09-25 12:57 ` Eric Blake
2020-09-25 8:32 ` [PATCH 0.5/7] include/block/block.h: drop non-ascii quotation mark Vladimir Sementsov-Ogievskiy
2020-09-25 12:58 ` Eric Blake
2020-09-25 16:02 ` [PATCH v9 0/7] coroutines: generate wrapper code Stefan Hajnoczi
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).