Re: [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list()

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-3-9 6:55, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    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>
>> ---
>>   block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
>>   include/block/qapi.h |    1 +
>>   qemu-img.c           |    3 ++-
>>   3 files changed, 40 insertions(+), 3 deletions(-)
>>
>
>> + * @sn: snapshot info need to be checked.
>
> s/need //
>
   OK.

>> + * return 0 if valid, it means load_vmstate() for it could succeed.
>
> Reads awkwardly; how about:
>
> it means load_vmstate() could load that snapshot.
>
   I forgot it may not have vmstate() but only only block snapshot,
It should be:
    return 0 if valid, it means the snapshot is consistent for the VM.

>> + */
>> +static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
>> +{
>> +    BlockDriverState *bs1 = NULL;
>> +    QEMUSnapshotInfo s, *sn_info = &s;
>> +    int ret = 0;
>> +
>> +    /* 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))) {
>> +        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
>> +            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
>> +            if (ret < 0) {
>> +                ret = -1;
>
> This function only returns 0 or -1...
>
   OK, it should be bool.

>> +/* return 0 on success, and @p_list will be set only on success. If
>> +   @vm_snapshot is true, only the snapshot valid for whole vm will be got. */
>
> grammar
>
> If @vm_snapshot is true, limit the results to snapshots valid for the
> whole vm.
>
   Looks better, thanks.

>> -
>> +        if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {
>
> ...yet you are only ever calling it in a boolean context.  Would it be
> smarter to have the function return bool (true for valid vm snapshot,
> false if the image snapshot is not usable as a vm snapshot)?
>
> Also, it might be nicer to name it snapshot_valid_for_vm, as in:
>
> if (vm_snapshot && !snapshot_valid_for_vm(...)) {
>      continue;
> }
   OK.

>


-- 
Best Regards

Wenchao Xia