Re: [Qemu-devel] [PATCH V7 05/14] block: add snapshot info query function bdrv_query_snapshot_infolist()

[Wenchao Xia]

于 2013-2-27 22:26, Markus Armbruster 写道:
> Wenchao Xia <address@hidden> writes:
> 
>>    This patch add function bdrv_query_snapshot_infolist(), which will
> 
> adds
> 
>> return snapshot info of an image in qmp object format. The implementation
>> code are based on the code moved from qemu-img.c with modification to fit 
>> more
> 
> implementation is based
> 
>> for qmp based block layer API.
>>    To help filter out snapshot info not needed, a call back function is
>> added in bdrv_query_snapshot_infolist().
>>    bdrv_can_read_snapshot() should be called before call this function,
>> to avoid got *errp set unexpectedly.
> 
> "avoid getting"
> 
> Not sure about "should".  Couldn't a caller safely call this with a
> non-snapshot bs argument, and simply deal with the error?
> 
  yes, but in some case this error should be avoided. For eg, in
bdrv_query_image_info(), if a image is not inserted, or it can't
take snapshot, other info should also be returned without error,
so if bdrv_can_read_snapshot() is called before, this kind of
error is avoided, and caller can check if there is other
error. I guess it should be documented as "in some case,
bdrv_can_read_snapshot() should be called."

> Long lines, and unusual paragraph formatting.  Here's how we usually do
> this:
> 
> This patch adds function bdrv_query_snapshot_infolist(), which will
> return snapshot info of an image in qmp object format. The
> implementation is based on the code moved from qemu-img.c with
> modification to fit more for qmp based block layer API.
> 
> To help filter out snapshot info not needed, a call back function is
> added in bdrv_query_snapshot_infolist().
> 
> bdrv_can_read_snapshot() should be called before call this function, to
> avoid getting *errp set unexpectedly.
> 
  Thanks, going to use this.

>> Signed-off-by: Wenchao Xia <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> ---
>>   block.c               |   60 
>> ++++++++++++++++++++++++++++++++++++++----------
>>   include/block/block.h |    8 +++++-
>>   qemu-img.c            |    8 +++++-
>>   3 files changed, 61 insertions(+), 15 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 368b37c..8d0145a 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2813,29 +2813,62 @@ int coroutine_fn 
>> bdrv_co_is_allocated_above(BlockDriverState *top,
>>       return 0;
>>   }
>>   
>> -void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
>> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
>> +                                               SnapshotFilterFunc filter,
>> +                                               void *opaque,
>> +                                               Error **errp)
>>   {
>>       int i, sn_count;
>>       QEMUSnapshotInfo *sn_tab = NULL;
>> -    SnapshotInfoList *info_list, *cur_item = NULL;
>> +    SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
>> +    SnapshotInfo *info;
>> +
>>       sn_count = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (sn_count < 0) {
>> +        /* Now bdrv_snapshot_list() use negative value to tip error, so a 
>> check
>> +         * of it was done here. In future errp can be set by that function
>> +         * itself, by changing the call back functions in c files in 
>> ./block.
>> +         */
> 
> I doubt we need this comment.
> 
  I am OK to remove it.

>> +        const char *dev = bdrv_get_device_name(bs);
>> +        switch (sn_count) {
>> +        case -ENOMEDIUM:
>> +            error_setg(errp, "Device '%s' is not inserted.", dev);
> 
> We generally don't end error messages with a period.  Please remove it
> from this and other messages.
> 
  OK.

>> +            break;
>> +        case -ENOTSUP:
>> +            error_setg(errp,
>> +                       "Device '%s' does not support internal snapshot.",
> 
> internal snapshots
> 
OK.

>> +                       dev);
>> +            break;
>> +        default:
>> +            error_setg(errp,
>> +                       "Device '%s' got '%d' for bdrv_snapshot_list(), "
>> +                       "message '%s'.",
>> +                       dev, sn_count, strerror(-sn_count));
> 
>                 error_setg(errp, "Can't list snapshots of device '%s': %s",
>                            dev, sterror(-sn_count));
>
OK.

>> +            break;
>> +        }
>> +        return NULL;
>> +    }
>>   
>>       for (i = 0; i < sn_count; i++) {
>> -        info->has_snapshots = true;
>> -        info_list = g_new0(SnapshotInfoList, 1);
>> +        if (filter && filter(&sn_tab[i], opaque) != 0) {
>> +            continue;
>> +        }
> 
> Is the filter feature really worth it?  If yes, should it be added in a
> separate patch?
> 
  I feel filter make logic more clearer later which can be used later
to filter out inconsistent snapshots. I am OK to form a seperate patch.

>>   
>> -        info_list->value                = g_new0(SnapshotInfo, 1);
>> -        info_list->value->id            = g_strdup(sn_tab[i].id_str);
>> -        info_list->value->name          = g_strdup(sn_tab[i].name);
>> -        info_list->value->vm_state_size = sn_tab[i].vm_state_size;
>> -        info_list->value->date_sec      = sn_tab[i].date_sec;
>> -        info_list->value->date_nsec     = sn_tab[i].date_nsec;
>> -        info_list->value->vm_clock_sec  = sn_tab[i].vm_clock_nsec / 
>> 1000000000;
>> -        info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 
>> 1000000000;
>> +        info = g_new0(SnapshotInfo, 1);
>> +        info->id            = g_strdup(sn_tab[i].id_str);
>> +        info->name          = g_strdup(sn_tab[i].name);
>> +        info->vm_state_size = sn_tab[i].vm_state_size;
>> +        info->date_sec      = sn_tab[i].date_sec;
>> +        info->date_nsec     = sn_tab[i].date_nsec;
>> +        info->vm_clock_sec  = sn_tab[i].vm_clock_nsec / 1000000000;
>> +        info->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
>> +
>> +        info_list = g_new0(SnapshotInfoList, 1);
>> +        info_list->value = info;
>>   
>>           /* XXX: waiting for the qapi to support qemu-queue.h types */
>>           if (!cur_item) {
>> -            info->snapshots = cur_item = info_list;
>> +            head = cur_item = info_list;
>>           } else {
>>               cur_item->next = info_list;
>>               cur_item = info_list;
>> @@ -2844,6 +2877,7 @@ void collect_snapshots(BlockDriverState *bs , 
>> ImageInfo *info)
>>       }
>>   
>>       g_free(sn_tab);
>> +    return head;
>>   }
>>   
>>   void collect_image_info(BlockDriverState *bs,
>> diff --git a/include/block/block.h b/include/block/block.h
>> index e6d915c..51a14f2 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -319,6 +319,13 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
>>                                  char *filename, int filename_size);
>>   void bdrv_get_full_backing_filename(BlockDriverState *bs,
>>                                       char *dest, size_t sz);
>> +
>> +typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque);
>> +/* assume bs is already opened, use qapi_free_* to free returned value. */
> 
> Pretty much all functions assume bs is open, hardly worth a comment.
> 
  OK, will remove.

>> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
>> +                                               SnapshotFilterFunc filter,
>> +                                               void *opaque,
>> +                                               Error **errp);
>>   BlockInfo *bdrv_query_info(BlockDriverState *s);
>>   BlockStats *bdrv_query_stats(const BlockDriverState *bs);
>>   bool bdrv_can_read_snapshot(BlockDriverState *bs);
>> @@ -450,7 +457,6 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const 
>> char *event,
>>   int bdrv_debug_resume(BlockDriverState *bs, const char *tag);
>>   bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag);
>>   
>> -void collect_snapshots(BlockDriverState *bs , ImageInfo *info);
>>   void collect_image_info(BlockDriverState *bs,
>>                           ImageInfo *info,
>>                           const char *filename);
>> diff --git a/qemu-img.c b/qemu-img.c
>> index b650d17..1034cc5 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1280,7 +1280,13 @@ static ImageInfoList *collect_image_info_list(const 
>> char *filename,
>>   
>>           info = g_new0(ImageInfo, 1);
>>           collect_image_info(bs, info, filename);
>> -        collect_snapshots(bs, info);
>> +        if (bdrv_can_read_snapshot(bs)) {
>> +            info->snapshots = bdrv_query_snapshot_infolist(bs, NULL,
>> +                                                           NULL, NULL);
>> +            if (info->snapshots) {
>> +                info->has_snapshots = true;
>> +            }
>> +        }
>>   
>>           elem = g_new0(ImageInfoList, 1);
>>           elem->value = info;
> 
> Silently ignores errors, but the old code does so, too.
> 
  OK, error will be checked.

> QAPI's has_FOO is pointless when FOO is a pointer.  Not your fault.
> 


-- 
Best Regards

Wenchao Xia