[V11,06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list()

This patch adds a parameter to tell whether return valid snapshots
for whole VM only.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
 include/block/qapi.h |    1 +
 qemu-img.c           |    3 ++-
 3 files changed, 40 insertions(+), 3 deletions(-)

Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

>   This patch adds a parameter to tell whether return valid snapshots
> for whole VM only.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  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;
>          }

于 2013-4-10 23:19, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
>>    This patch adds a parameter to tell whether return valid snapshots
>> for whole VM only.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   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?
> 
  It is copied from do_info_snapshots() and forgot to check it in
load_vmstate(), I think that is not correct, will use the exactly same
logic in load_vmstate() in next version.

>> +        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;
>>           }
>