Re: [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list()

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

>   This patch adds a parameter to tell whether return valid snapshots
> for whole VM only.
>
> Signed-off-by: Wenchao Xia <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> Reviewed-by: Kevin Wolf <address@hidden>
> ---
>  block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
>  include/block/qapi.h |    1 +
>  qemu-img.c           |    3 ++-
>  3 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 03369a5..19d4d93 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -23,14 +23,47 @@
>   */
>  
>  #include "block/qapi.h"
> +#include "block/snapshot.h"
>  #include "block/block_int.h"
>  
>  /*
> + * check whether the snapshot is valid for whole vm.
> + *
> + * @sn: snapshot info to be checked.
> + * @bs: where @sn was found.
> + *
> + * return true if the snapshot is consistent for the VM.
> + */
> +static bool snapshot_valid_for_vm(const QEMUSnapshotInfo *sn,
> +                                  BlockDriverState *bs)
> +{
> +    BlockDriverState *bs1 = NULL;
> +    QEMUSnapshotInfo s, *sn_info = &s;
> +    int ret;
> +
> +    /* Check logic is connected with load_vmstate():
> +       Only check the devices that can snapshot, other devices that can't
> +       take snapshot, for example, readonly ones, will be ignored in
> +       load_vmstate(). */
> +    while ((bs1 = bdrv_next(bs1))) {

Unlike load_vmstate(), you don't do

           if (!bdrv_is_inserted(bs1) || bdrv_is_read_only(bs1)) {
               continue;
           }

Why?

> +        if (bs1 != bs && bdrv_can_snapshot(bs1)) {
> +            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
> +            if (ret < 0) {
> +                return false;
> +            }
> +        }
> +    }
> +    return true;
> +}
> +
> +/*
>   * return 0 on success, @p_list will be set only on success, and caller need 
> to
> - * check *p_list on success.
> + * check *p_list on success. If @vm_snapshot is true, limit the results to
> + * snapshots valid for the whole VM.
>   */
>  int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>                                    SnapshotInfoList **p_list,
> +                                  bool vm_snapshot,
>                                    Error **errp)
>  {
>      int i, sn_count;
> @@ -59,7 +92,9 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>      }
>  
>      for (i = 0; i < sn_count; i++) {
> -
> +        if (vm_snapshot && !snapshot_valid_for_vm(&sn_tab[i], bs)) {
> +            continue;
> +        }
>          info = g_new0(SnapshotInfo, 1);
>          info->id            = g_strdup(sn_tab[i].id_str);
>          info->name          = g_strdup(sn_tab[i].name);
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index 91dc41b..fe66053 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -30,6 +30,7 @@
>  
>  int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>                                    SnapshotInfoList **p_list,
> +                                  bool vm_snapshot,
>                                    Error **errp);
>  void bdrv_collect_image_info(BlockDriverState *bs,
>                               ImageInfo *info,
> diff --git a/qemu-img.c b/qemu-img.c
> index 51043ef..261c277 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1735,7 +1735,8 @@ static ImageInfoList *collect_image_info_list(const 
> char *filename,
>  
>          info = g_new0(ImageInfo, 1);
>          bdrv_collect_image_info(bs, info, filename);
> -        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
> +        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
> +                                           false, NULL) &&
>              info->snapshots) {
>              info->has_snapshots = true;
>          }