Re: [Qemu-devel] [PULL 01/31] block: Fix bdrv_next() memory leak

[Paolo Bonzini <pbonzini@redhat.com>]

On 25/05/2016 19:39, Kevin Wolf wrote:
> @@ -395,20 +399,27 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
>  {
>      int ret = 0;
>      BlockDriverState *bs;
> -    BdrvNextIterator *it = NULL;
> +    BdrvNextIterator it;
>      QEMUSnapshotInfo sn1, *snapshot = &sn1;
>  
> -    while (ret == 0 && (it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
>  
>          aio_context_acquire(ctx);
>          if (bdrv_can_snapshot(bs) &&
>                  bdrv_snapshot_find(bs, snapshot, name) >= 0) {
>              ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
> +            if (ret < 0) {
> +                goto fail;
> +            }

This is redundant with the check below (and the check below is right;
this one is wrong).

Thanks,

Paolo

>          }
>          aio_context_release(ctx);
> +        if (ret < 0) {
> +            goto fail;
> +        }
>      }
>  
> +fail:
>      *first_bad_bs = bs;
>      return ret;
>  }
> @@ -418,9 +429,9 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>  {
>      int err = 0;
>      BlockDriverState *bs;
> -    BdrvNextIterator *it = NULL;
> +    BdrvNextIterator it;
>  
> -    while (err == 0 && (it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
>  
>          aio_context_acquire(ctx);
> @@ -428,8 +439,12 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>              err = bdrv_snapshot_goto(bs, name);
>          }
>          aio_context_release(ctx);
> +        if (err < 0) {
> +            goto fail;
> +        }
>      }
>  
> +fail:
>      *first_bad_bs = bs;
>      return err;
>  }
> @@ -439,9 +454,9 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
>      QEMUSnapshotInfo sn;
>      int err = 0;
>      BlockDriverState *bs;
> -    BdrvNextIterator *it = NULL;
> +    BdrvNextIterator it;
>  
> -    while (err == 0 && (it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
>  
>          aio_context_acquire(ctx);
> @@ -449,8 +464,12 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
>              err = bdrv_snapshot_find(bs, &sn, name);
>          }
>          aio_context_release(ctx);
> +        if (err < 0) {
> +            goto fail;
> +        }
>      }
>  
> +fail:
>      *first_bad_bs = bs;
>      return err;
>  }
> @@ -462,9 +481,9 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>  {
>      int err = 0;
>      BlockDriverState *bs;
> -    BdrvNextIterator *it = NULL;
> +    BdrvNextIterator it;
>  
> -    while (err == 0 && (it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
>  
>          aio_context_acquire(ctx);
> @@ -476,24 +495,32 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>              err = bdrv_snapshot_create(bs, sn);
>          }
>          aio_context_release(ctx);
> +        if (err < 0) {
> +            goto fail;
> +        }
>      }
>  
> +fail:
>      *first_bad_bs = bs;
>      return err;
>  }
>  
>  BlockDriverState *bdrv_all_find_vmstate_bs(void)
>  {
> -    bool not_found = true;
>      BlockDriverState *bs;
> -    BdrvNextIterator *it = NULL;
> +    BdrvNextIterator it;
>  
> -    while (not_found && (it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        bool found;
>  
>          aio_context_acquire(ctx);
> -        not_found = !bdrv_can_snapshot(bs);
> +        found = bdrv_can_snapshot(bs);
>          aio_context_release(ctx);
> +
> +        if (found) {
> +            break;
> +        }
>      }
>      return bs;
>  }
> diff --git a/blockdev.c b/blockdev.c
> index 40e4e6f..0e37e09 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4164,9 +4164,9 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>  {
>      BlockJobInfoList *head = NULL, **p_next = &head;
>      BlockDriverState *bs;
> -    BdrvNextIterator *it = NULL;
> +    BdrvNextIterator it;
>  
> -    while ((it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *aio_context = bdrv_get_aio_context(bs);
>  
>          aio_context_acquire(aio_context);
> diff --git a/include/block/block.h b/include/block/block.h
> index a8c15e3..770ca26 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -17,7 +17,6 @@ typedef struct BlockJob BlockJob;
>  typedef struct BdrvChild BdrvChild;
>  typedef struct BdrvChildRole BdrvChildRole;
>  typedef struct BlockJobTxn BlockJobTxn;
> -typedef struct BdrvNextIterator BdrvNextIterator;
>  
>  typedef struct BlockDriverInfo {
>      /* in bytes, 0 if irrelevant */
> @@ -402,7 +401,19 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
>                                   Error **errp);
>  bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
>  BlockDriverState *bdrv_next_node(BlockDriverState *bs);
> -BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs);
> +
> +typedef struct BdrvNextIterator {
> +    enum {
> +        BDRV_NEXT_BACKEND_ROOTS,
> +        BDRV_NEXT_MONITOR_OWNED,
> +    } phase;
> +    BlockBackend *blk;
> +    BlockDriverState *bs;
> +} BdrvNextIterator;
> +
> +BlockDriverState *bdrv_first(BdrvNextIterator *it);
> +BlockDriverState *bdrv_next(BdrvNextIterator *it);
> +
>  BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
>  int bdrv_is_encrypted(BlockDriverState *bs);
>  int bdrv_key_required(BlockDriverState *bs);
> diff --git a/migration/block.c b/migration/block.c
> index a7a76a0..e0628d1 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -383,7 +383,7 @@ static void init_blk_migration(QEMUFile *f)
>      BlockDriverState *bs;
>      BlkMigDevState *bmds;
>      int64_t sectors;
> -    BdrvNextIterator *it = NULL;
> +    BdrvNextIterator it;
>  
>      block_mig_state.submitted = 0;
>      block_mig_state.read_done = 0;
> @@ -394,7 +394,7 @@ static void init_blk_migration(QEMUFile *f)
>      block_mig_state.zero_blocks = migrate_zero_blocks();
>  
>  
> -    while ((it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          if (bdrv_is_read_only(bs)) {
>              continue;
>          }
> diff --git a/monitor.c b/monitor.c
> index 6a32b9b..404d594 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3432,12 +3432,12 @@ static void vm_completion(ReadLineState *rs, const char *str)
>  {
>      size_t len;
>      BlockDriverState *bs;
> -    BdrvNextIterator *it = NULL;
> +    BdrvNextIterator it;
>  
>      len = strlen(str);
>      readline_set_completion_index(rs, len);
>  
> -    while ((it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          SnapshotInfoList *snapshots, *snapshot;
>          AioContext *ctx = bdrv_get_aio_context(bs);
>          bool ok = false;
> diff --git a/qmp.c b/qmp.c
> index 8f8ae3a..3165f87 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -181,7 +181,7 @@ void qmp_cont(Error **errp)
>      Error *local_err = NULL;
>      BlockBackend *blk;
>      BlockDriverState *bs;
> -    BdrvNextIterator *it;
> +    BdrvNextIterator it;
>  
>      /* if there is a dump in background, we should wait until the dump
>       * finished */
> @@ -201,8 +201,7 @@ void qmp_cont(Error **errp)
>          blk_iostatus_reset(blk);
>      }
>  
> -    it = NULL;
> -    while ((it = bdrv_next(it, &bs))) {
> +    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          bdrv_add_key(bs, NULL, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>