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);
>
