Re: [PATCH v8 7/7] block/io: refactor save/load vmstate

[Eric Blake]

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.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org