Re: [Qemu-devel] [PATCH V5 04/13] block: add snapshot info query function bdrv_query_snapshot_infolist()

[Wenchao Xia]

于 2013-1-29 20:48, Kevin Wolf 写道:
> Am 24.01.2013 03:57, schrieb Wenchao Xia:
> >    This patch add function bdrv_query_snapshot_infolist(), which will
> > return snapshot info of an image in qmp object format. The implementation
> > code are mostly copied 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 got *errp set unexpectedly.
> >
> > Signed-off-by: Wenchao Xia <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > ---
> >   block.c               |   60 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   include/block/block.h |    7 +++++
> >   2 files changed, 67 insertions(+), 0 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 934bb3f..7cdb6c6 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2842,6 +2842,66 @@ int coroutine_fn 
> > bdrv_co_is_allocated_above(BlockDriverState *top,
> >       return 0;
> >   }
> >
> > +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
> > +                                               SnapshotFilterFunc filter,
>
> This filter things looks overengineered to me, and no patch in this
> series really uses it. Do we really need it?
  It is used later, which filter out the internal ones not exist
in every block device. I think this provide a clearer way, than
original code in savevm.c which use two "for" to get the correct
snapshots.

> > +                                               void *opaque,
> > +                                               Error **errp)
> > +{
> > +    int i, sn_count;
> > +    QEMUSnapshotInfo *sn_tab = NULL;
> > +    SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
> > +
> > +    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.
> > +         */
> > +        const char *dev = bdrv_get_device_name(bs);
> > +        if (sn_count == -ENOMEDIUM) {
> > +            error_setg(errp, "Device '%s' is not inserted.", dev);
> > +        } else if (sn_count == -ENOTSUP) {
> > +            error_setg(errp,
> > +                       "Device '%s' does not support internal snapshot.",
> > +                       dev);
> > +        } else {
> > +            error_setg(errp,
> > +                       "Device '%s' got %d for bdrv_snapshot_list().",
> > +                       dev, sn_count);
>
> strerror(-sn_count) would make the error message more useful.
>
> You could also consider using switch instead of an if/else if chain.
 OK.

> > +        }
> > +        return NULL;
> > +    }
> > +
> > +    for (i = 0; i < sn_count; i++) {
> > +        if (filter && filter(&sn_tab[i], opaque) != 0) {
> > +            continue;
> > +        }
> > +
> > +        info_list = g_new0(SnapshotInfoList, 1);
> > +
> > +        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;
>
> Did I already mention somewhere that I'm a fan of compound literals?
> Looks so much nicer than repeating info_list->value-> in each line. ;-)
  Do you mean value=info_list->value and then value->***=***?


> Kevin


--
Best Regards

Wenchao Xia