Re: [Qemu-devel] [PATCH 04/11] qemu-img: move image retrieving function to block layer

[Eric Blake]

On 12/29/2012 01:45 AM, Wenchao Xia wrote:
>   This patch moves collect_image_info() and collect_snapshot()
> to general block layer and encapsulate them as bdrv_query_image_info()
> and bdrv_query_snapshot_infolist(), as mirror function to brdv_query_info().
> The called function in qemu-img.c is switched to bdrv_query_image_info().
> To help filter out snapshot info not needed, a call back function is
> added in bdrv_query_snapshot_infolist().
> 
> Signed-off-by: Wenchao Xia <address@hidden>
> ---
>  block.c               |  118 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |    9 ++++
>  qemu-img.c            |   88 +-----------------------------------
>  3 files changed, 129 insertions(+), 86 deletions(-)
> 

> +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, *head = NULL;
> +    sn_count = bdrv_snapshot_list(bs, &sn_tab);
> +    if (sn_count < 0) {
> +        if (errp != NULL) {
> +            error_setg(errp, "bdrv_snapshot_list: error %d\n", sn_count);
> +        }
> +        return NULL;
> +    }
> +
> +    for (i = 0; i < sn_count; i++) {
> +        if ((filter != NULL) && (filter(&sn_tab[i], opaque) != 0)) {

My brief glance through qemu sources makes me think you
over-parenthesized this, and that:

if (filter && filter(&sn_tab[i], opaque) != 0) {

would be sufficient.


> +    bdrv_get_backing_filename(bs, backing_filename, 
> sizeof(backing_filename));
> +    if (backing_filename[0] != '\0') {
> +        info->backing_filename = g_strdup(backing_filename);
> +        info->has_backing_filename = true;
> +        bdrv_get_full_backing_filename(bs, backing_filename2,
> +                                       sizeof(backing_filename2));
> +
> +        if (strcmp(backing_filename, backing_filename2) != 0) {
> +            info->full_backing_filename =
> +                        g_strdup(backing_filename2);

I know it's just code motion, but why the line wrap here?

> +++ b/include/block/block.h
> @@ -316,8 +316,17 @@ 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 openned, use g_free to free returned value. */

s/openned/opened/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature