[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 1/4] block: Expect graph changes in bdrv_parent_
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-block] [PATCH 1/4] block: Expect graph changes in bdrv_parent_drained_begin/end |
Date: |
Wed, 29 Nov 2017 13:44:08 +0000 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Wed, Nov 29, 2017 at 11:25:10AM +0100, Paolo Bonzini wrote:
> From: Kevin Wolf <address@hidden>
>
> The .drained_begin/end callbacks can (directly or indirectly via
> aio_poll()) cause block nodes to be removed or the current BdrvChild to
> point to a different child node.
>
> Use QLIST_FOREACH_SAFE() to make sure we don't access invalid
> BlockDriverStates or accidentally continue iterating the parents of the
> new child node instead of the node we actually came from.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> Tested-by: Jeff Cody <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Reviewed-by: Jeff Cody <address@hidden>
> ---
> block/io.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 4fdf93a014..6773926fc1 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -42,9 +42,9 @@ static int coroutine_fn
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>
> void bdrv_parent_drained_begin(BlockDriverState *bs)
> {
> - BdrvChild *c;
> + BdrvChild *c, *next;
>
> - QLIST_FOREACH(c, &bs->parents, next_parent) {
> + QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
> if (c->role->drained_begin) {
> c->role->drained_begin(c);
> }
> @@ -53,9 +53,9 @@ void bdrv_parent_drained_begin(BlockDriverState *bs)
>
> void bdrv_parent_drained_end(BlockDriverState *bs)
> {
> - BdrvChild *c;
> + BdrvChild *c, *next;
>
> - QLIST_FOREACH(c, &bs->parents, next_parent) {
> + QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
> if (c->role->drained_end) {
> c->role->drained_end(c);
> }
Hmm...not sure if this patch is enough.
Thinking about this more, if this sub-graph changes then
.drained_begin() callbacks are not necessarily paired with
.drained_end() callbacks.
In other words, all of the following are possible:
1. Only .drained_begin() is called
2. .drained_begin() is called, then .drained_end()
3. Only .drained_end() is called
It makes me wonder if we need to either:
Take references and ensure .drained_end() gets called if and only if
.drained_begin() was also called.
OR
Document that .drained_begin/end() calls may not be paired and audit
existing code to check that this is safe.
Stefan
signature.asc
Description: PGP signature