| Message ID | 20200915164411.20590-8-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | coroutines: generate wrapper code | expand |
On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote: > Like for read/write in a previous commit, drop extra indirection layer, > generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > block/coroutines.h | 10 +++---- > include/block/block.h | 6 ++-- > block/io.c | 67 ++++++++++++++++++++++--------------------- > 3 files changed, 42 insertions(+), 41 deletions(-) > > diff --git a/block/coroutines.h b/block/coroutines.h > int coroutine_fn > -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, > - bool is_read) > +bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) > { > BlockDriver *drv = bs->drv; > int ret = -ENOTSUP; > > + if (!drv) { > + return -ENOMEDIUM; > + } > + > bdrv_inc_in_flight(bs); > > - if (!drv) { > - ret = -ENOMEDIUM; > - } else if (drv->bdrv_load_vmstate) { > - if (is_read) { > - ret = drv->bdrv_load_vmstate(bs, qiov, pos); > - } else { > - ret = drv->bdrv_save_vmstate(bs, qiov, pos); > - } > + if (drv->bdrv_load_vmstate) { > + ret = drv->bdrv_load_vmstate(bs, qiov, pos); This one makes sense; > } else if (bs->file) { > - ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read); > + ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos); > } > > bdrv_dec_in_flight(bs); > + > return ret; > } > > -int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, > - int64_t pos, int size) > +int coroutine_fn > +bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) > { > - QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); > - int ret; > + BlockDriver *drv = bs->drv; > + int ret = -ENOTSUP; > > - ret = bdrv_writev_vmstate(bs, &qiov, pos); > - if (ret < 0) { > - return ret; > + if (!drv) { > + return -ENOMEDIUM; > } > > - return size; > -} > + bdrv_inc_in_flight(bs); > > -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) > -{ > - return bdrv_rw_vmstate(bs, qiov, pos, false); > + if (drv->bdrv_load_vmstate) { > + ret = drv->bdrv_save_vmstate(bs, qiov, pos); but this one looks awkward. It represents the pre-patch logic, but it would be nicer to check for bdrv_save_vmstate. With that tweak, my R-b still stands. I had an interesting time applying this patch due to merge conflicts with the new bdrv_primary_bs() changes that landed in the meantime.
23.09.2020 23:10, Eric Blake wrote: > On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote: >> Like for read/write in a previous commit, drop extra indirection layer, >> generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate(). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> block/coroutines.h | 10 +++---- >> include/block/block.h | 6 ++-- >> block/io.c | 67 ++++++++++++++++++++++--------------------- >> 3 files changed, 42 insertions(+), 41 deletions(-) >> >> diff --git a/block/coroutines.h b/block/coroutines.h > >> int coroutine_fn >> -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, >> - bool is_read) >> +bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) >> { >> BlockDriver *drv = bs->drv; >> int ret = -ENOTSUP; >> + if (!drv) { >> + return -ENOMEDIUM; >> + } >> + >> bdrv_inc_in_flight(bs); >> - if (!drv) { >> - ret = -ENOMEDIUM; >> - } else if (drv->bdrv_load_vmstate) { >> - if (is_read) { >> - ret = drv->bdrv_load_vmstate(bs, qiov, pos); >> - } else { >> - ret = drv->bdrv_save_vmstate(bs, qiov, pos); >> - } >> + if (drv->bdrv_load_vmstate) { >> + ret = drv->bdrv_load_vmstate(bs, qiov, pos); > > This one makes sense; > >> } else if (bs->file) { >> - ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read); >> + ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos); >> } >> bdrv_dec_in_flight(bs); >> + >> return ret; >> } >> -int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, >> - int64_t pos, int size) >> +int coroutine_fn >> +bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) >> { >> - QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); >> - int ret; >> + BlockDriver *drv = bs->drv; >> + int ret = -ENOTSUP; >> - ret = bdrv_writev_vmstate(bs, &qiov, pos); >> - if (ret < 0) { >> - return ret; >> + if (!drv) { >> + return -ENOMEDIUM; >> } >> - return size; >> -} >> + bdrv_inc_in_flight(bs); >> -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) >> -{ >> - return bdrv_rw_vmstate(bs, qiov, pos, false); >> + if (drv->bdrv_load_vmstate) { >> + ret = drv->bdrv_save_vmstate(bs, qiov, pos); > > but this one looks awkward. It represents the pre-patch logic, but it would be nicer to check for bdrv_save_vmstate. With that tweak, my R-b still stands. Agree. > > I had an interesting time applying this patch due to merge conflicts with the new bdrv_primary_bs() changes that landed in the meantime. > Thanks a lot! To clarify: did you finally staged the series to send a pull request? Or Stefan should do it? Should I make a v9?
On Tue, Sep 15, 2020 at 07:44:11PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Like for read/write in a previous commit, drop extra indirection layer, > generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > block/coroutines.h | 10 +++---- > include/block/block.h | 6 ++-- > block/io.c | 67 ++++++++++++++++++++++--------------------- > 3 files changed, 42 insertions(+), 41 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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 b8b4c177de..6cd789724b 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 68d7d9cf80..84f82bc069 100644 --- a/block/io.c +++ b/block/io.c @@ -2491,66 +2491,67 @@ int bdrv_is_allocated_above(BlockDriverState *top, } int coroutine_fn -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, - bool is_read) +bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { BlockDriver *drv = bs->drv; int ret = -ENOTSUP; + if (!drv) { + return -ENOMEDIUM; + } + bdrv_inc_in_flight(bs); - if (!drv) { - ret = -ENOMEDIUM; - } else if (drv->bdrv_load_vmstate) { - if (is_read) { - ret = drv->bdrv_load_vmstate(bs, qiov, pos); - } else { - ret = drv->bdrv_save_vmstate(bs, qiov, pos); - } + if (drv->bdrv_load_vmstate) { + ret = drv->bdrv_load_vmstate(bs, qiov, pos); } else if (bs->file) { - ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read); + ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos); } bdrv_dec_in_flight(bs); + return ret; } -int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, - int64_t pos, int size) +int coroutine_fn +bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { - QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); - int ret; + BlockDriver *drv = bs->drv; + int ret = -ENOTSUP; - ret = bdrv_writev_vmstate(bs, &qiov, pos); - if (ret < 0) { - return ret; + if (!drv) { + return -ENOMEDIUM; } - return size; -} + bdrv_inc_in_flight(bs); -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) -{ - return bdrv_rw_vmstate(bs, qiov, pos, false); + if (drv->bdrv_load_vmstate) { + ret = drv->bdrv_save_vmstate(bs, qiov, pos); + } else if (bs->file) { + ret = bdrv_co_writev_vmstate(bs->file->bs, qiov, pos); + } + + bdrv_dec_in_flight(bs); + + return ret; } -int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, +int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, int64_t pos, int size) { QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); - int ret; - - ret = bdrv_readv_vmstate(bs, &qiov, pos); - if (ret < 0) { - return ret; - } + int ret = bdrv_writev_vmstate(bs, &qiov, pos); - return size; + return ret < 0 ? ret : size; } -int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) +int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, + int64_t pos, int size) { - return bdrv_rw_vmstate(bs, qiov, pos, true); + QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size); + int ret = bdrv_readv_vmstate(bs, &qiov, pos); + + return ret < 0 ? ret : size; } /**************************************************************/