From patchwork Wed May 25 17:39:26 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 9135959 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 3AC2A6075C for ; Wed, 25 May 2016 17:45:19 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3007B280BB for ; Wed, 25 May 2016 17:45:19 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 24EAF2829C; Wed, 25 May 2016 17:45:19 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 2B170280BB for ; Wed, 25 May 2016 17:45:17 +0000 (UTC) Received: from localhost ([::1]:34307 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5crr-0001Pe-OM for patchwork-qemu-devel@patchwork.kernel.org; Wed, 25 May 2016 13:45:15 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49294) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5cnI-0005e0-Uk for qemu-devel@nongnu.org; Wed, 25 May 2016 13:40:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5cnD-0004Pj-LB for qemu-devel@nongnu.org; Wed, 25 May 2016 13:40:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46672) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5cmz-0004Hn-LU; Wed, 25 May 2016 13:40:13 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2719C50F5C; Wed, 25 May 2016 17:40:13 +0000 (UTC) Received: from noname.redhat.com (ovpn-116-86.ams2.redhat.com [10.36.116.86]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u4PHeAtJ024245; Wed, 25 May 2016 13:40:12 -0400 From: Kevin Wolf To: qemu-block@nongnu.org Date: Wed, 25 May 2016 19:39:26 +0200 Message-Id: <1464197996-4581-2-git-send-email-kwolf@redhat.com> In-Reply-To: <1464197996-4581-1-git-send-email-kwolf@redhat.com> References: <1464197996-4581-1-git-send-email-kwolf@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 25 May 2016 17:40:13 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL 01/31] block: Fix bdrv_next() memory leak X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, qemu-devel@nongnu.org Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP The bdrv_next() users all leaked the BdrvNextIterator after completing the iteration. Simply changing bdrv_next() to free the iterator before returning NULL at the end of list doesn't work because some callers exit the loop before looking at all BDSes. This patch moves the BdrvNextIterator from the heap to the stack of the caller and switches to a bdrv_first()/bdrv_next() interface for initialising the iterator. Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng --- block.c | 18 ++++++++--------- block/block-backend.c | 41 +++++++++++++++++--------------------- block/io.c | 10 ++++------ block/snapshot.c | 55 ++++++++++++++++++++++++++++++++++++++------------- blockdev.c | 4 ++-- include/block/block.h | 15 ++++++++++++-- migration/block.c | 4 ++-- monitor.c | 4 ++-- qmp.c | 5 ++--- 9 files changed, 92 insertions(+), 64 deletions(-) diff --git a/block.c b/block.c index 1205ef8..d287771 100644 --- a/block.c +++ b/block.c @@ -3195,9 +3195,9 @@ void bdrv_invalidate_cache_all(Error **errp) { BlockDriverState *bs; Error *local_err = NULL; - BdrvNextIterator *it = NULL; + BdrvNextIterator it; - while ((it = bdrv_next(it, &bs)) != NULL) { + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { AioContext *aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); @@ -3239,11 +3239,11 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs, int bdrv_inactivate_all(void) { BlockDriverState *bs = NULL; - BdrvNextIterator *it = NULL; + BdrvNextIterator it; int ret = 0; int pass; - while ((it = bdrv_next(it, &bs)) != NULL) { + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { aio_context_acquire(bdrv_get_aio_context(bs)); } @@ -3252,8 +3252,7 @@ int bdrv_inactivate_all(void) * the second pass sets the BDRV_O_INACTIVE flag so that no further write * is allowed. */ for (pass = 0; pass < 2; pass++) { - it = NULL; - while ((it = bdrv_next(it, &bs)) != NULL) { + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { ret = bdrv_inactivate_recurse(bs, pass); if (ret < 0) { goto out; @@ -3262,8 +3261,7 @@ int bdrv_inactivate_all(void) } out: - it = NULL; - while ((it = bdrv_next(it, &bs)) != NULL) { + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { aio_context_release(bdrv_get_aio_context(bs)); } @@ -3753,10 +3751,10 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, bool bdrv_is_first_non_filter(BlockDriverState *candidate) { BlockDriverState *bs; - BdrvNextIterator *it = NULL; + BdrvNextIterator it; /* walk down the bs forest recursively */ - while ((it = bdrv_next(it, &bs)) != NULL) { + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { bool perm; /* try to recurse in this top level bs */ diff --git a/block/block-backend.c b/block/block-backend.c index 6928d61..9f306f8 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -286,25 +286,11 @@ BlockBackend *blk_next(BlockBackend *blk) : QTAILQ_FIRST(&monitor_block_backends); } -struct BdrvNextIterator { - enum { - BDRV_NEXT_BACKEND_ROOTS, - BDRV_NEXT_MONITOR_OWNED, - } phase; - BlockBackend *blk; - BlockDriverState *bs; -}; - /* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by * the monitor or attached to a BlockBackend */ -BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs) +BlockDriverState *bdrv_next(BdrvNextIterator *it) { - if (!it) { - it = g_new(BdrvNextIterator, 1); - *it = (BdrvNextIterator) { - .phase = BDRV_NEXT_BACKEND_ROOTS, - }; - } + BlockDriverState *bs; /* First, return all root nodes of BlockBackends. In order to avoid * returning a BDS twice when multiple BBs refer to it, we only return it @@ -312,11 +298,11 @@ BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs) if (it->phase == BDRV_NEXT_BACKEND_ROOTS) { do { it->blk = blk_all_next(it->blk); - *bs = it->blk ? blk_bs(it->blk) : NULL; - } while (it->blk && (*bs == NULL || bdrv_first_blk(*bs) != it->blk)); + bs = it->blk ? blk_bs(it->blk) : NULL; + } while (it->blk && (bs == NULL || bdrv_first_blk(bs) != it->blk)); - if (*bs) { - return it; + if (bs) { + return bs; } it->phase = BDRV_NEXT_MONITOR_OWNED; } @@ -326,10 +312,19 @@ BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs) * by the above block already */ do { it->bs = bdrv_next_monitor_owned(it->bs); - *bs = it->bs; - } while (*bs && bdrv_has_blk(*bs)); + bs = it->bs; + } while (bs && bdrv_has_blk(bs)); + + return bs; +} + +BlockDriverState *bdrv_first(BdrvNextIterator *it) +{ + *it = (BdrvNextIterator) { + .phase = BDRV_NEXT_BACKEND_ROOTS, + }; - return *bs ? it : NULL; + return bdrv_next(it); } /* diff --git a/block/io.c b/block/io.c index 60a6bd8..2a28d63 100644 --- a/block/io.c +++ b/block/io.c @@ -271,10 +271,10 @@ void bdrv_drain_all(void) /* Always run first iteration so any pending completion BHs run */ bool busy = true; BlockDriverState *bs; - BdrvNextIterator *it = NULL; + BdrvNextIterator it; GSList *aio_ctxs = NULL, *ctx; - 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); @@ -302,10 +302,9 @@ void bdrv_drain_all(void) for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) { AioContext *aio_context = ctx->data; - it = NULL; aio_context_acquire(aio_context); - while ((it = bdrv_next(it, &bs))) { + for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { if (aio_context == bdrv_get_aio_context(bs)) { if (bdrv_requests_pending(bs)) { busy = true; @@ -318,8 +317,7 @@ void bdrv_drain_all(void) } } - it = NULL; - 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/block/snapshot.c b/block/snapshot.c index 3917ec5..6e6e34f 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -374,9 +374,9 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs) { bool ok = true; BlockDriverState *bs; - BdrvNextIterator *it = NULL; + BdrvNextIterator it; - while (ok && (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); @@ -384,8 +384,12 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs) ok = bdrv_can_snapshot(bs); } aio_context_release(ctx); + if (!ok) { + goto fail; + } } +fail: *first_bad_bs = bs; return ok; } @@ -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; + } } 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);