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

[Wenchao Xia]

于 2013-4-10 23:19, 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?
> 
  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;
>>           }
> 


-- 
Best Regards

Wenchao Xia