Re: [Qemu-devel] [PATCH for-2.11 2/3] block: Add errp to bdrv_all_goto_snapshot()

[Vladimir Sementsov-Ogievskiy]

20.11.2017 17:50, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   include/block/snapshot.h |  3 ++-
>   block/snapshot.c         | 11 ++++++-----
>   migration/savevm.c       |  6 +++---
>   3 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index aeb80405e8..9407799941 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -84,7 +84,8 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
>   bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
>   int bdrv_all_delete_snapshot(const char *name, BlockDriverState 
> **first_bsd_bs,
>                                Error **err);
> -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
> +                           Error **errp);
>   int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
>   int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>                                BlockDriverState *vm_state_bs,
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 9c941e638d..13ec3b1c8c 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -467,9 +467,10 @@ fail:
>   }
>   
>   
> -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
> +                           Error **errp)
>   {
> -    int err = 0;
> +    int ret = 0;
>       BlockDriverState *bs;
>       BdrvNextIterator it;
>   
> @@ -478,10 +479,10 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>   
>           aio_context_acquire(ctx);
>           if (bdrv_can_snapshot(bs)) {
> -            err = bdrv_snapshot_goto(bs, name, NULL);
> +            ret = bdrv_snapshot_goto(bs, name, errp);
>           }
>           aio_context_release(ctx);
> -        if (err < 0) {
> +        if (ret < 0) {
>               bdrv_next_cleanup(&it);
>               goto fail;
>           }
> @@ -489,7 +490,7 @@ int bdrv_all_goto_snapshot(const char *name, 
> BlockDriverState **first_bad_bs)
>   
>   fail:
>       *first_bad_bs = bs;
> -    return err;
> +    return ret;
>   }
>   
>   int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4a88228614..192f2d82cd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2346,10 +2346,10 @@ int load_snapshot(const char *name, Error **errp)
>       /* Flush all IO requests so they don't interfere with the new state.  */
>       bdrv_drain_all_begin();
>   
> -    ret = bdrv_all_goto_snapshot(name, &bs);
> +    ret = bdrv_all_goto_snapshot(name, &bs, errp);
>       if (ret < 0) {
> -        error_setg(errp, "Error %d while activating snapshot '%s' on '%s'",
> -                     ret, name, bdrv_get_device_name(bs));
> +        error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
> +                      name, bdrv_get_device_name(bs));

possible resulting msg: "Could not load snapshot 'snap0' on 'dev0': 
Failed to load snapshot". a bit weird. with this fixed or not:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

>           goto err_drain;
>       }
>   


--
Best regards,
Vladimir