| Message ID | 20211111120829.81329-6-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Attempt on fixing 030-reported errors | expand |
11.11.2021 15:08, Hanna Reitz wrote: > bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially > set it to NULL. That is dangerous, because BDS parents generally assume > that their children's .bs pointer is never NULL. We therefore want to > let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer > to NULL, too. > > This patch lays the foundation for it by passing a BdrvChild ** pointer > to bdrv_replace_child_noperm() so that it can later use it to NULL the > BdrvChild pointer immediately after setting BdrvChild.bs to NULL. > > (We will still need to undertake some intermediate steps, though.) > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> Series already applied, but I still feel myself responsible to track how transactions changed:) Don't bother with applying my r-b marks into applied series. > --- > block.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/block.c b/block.c > index c7d5aa5254..d668156eca 100644 > --- a/block.c > +++ b/block.c > @@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, > static bool bdrv_recurse_has_child(BlockDriverState *bs, > BlockDriverState *child); > > -static void bdrv_replace_child_noperm(BdrvChild *child, > +static void bdrv_replace_child_noperm(BdrvChild **child, > BlockDriverState *new_bs); > static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, > BdrvChild *child, > @@ -2270,7 +2270,7 @@ static void bdrv_replace_child_abort(void *opaque) > BlockDriverState *new_bs = s->child->bs; > > /* old_bs reference is transparently moved from @s to @s->child */ > - bdrv_replace_child_noperm(s->child, s->old_bs); > + bdrv_replace_child_noperm(&s->child, s->old_bs); - no sense / no harm in clearing the pointer, as it's a field in transaction state struct, and should not be used after abort - hard to say do we really need clearing some another pointer, upper level should care about it > bdrv_unref(new_bs); > } > > @@ -2300,7 +2300,7 @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs, > if (new_bs) { > bdrv_ref(new_bs); > } > - bdrv_replace_child_noperm(child, new_bs); > + bdrv_replace_child_noperm(&child, new_bs); - no sence / no harm, as it's a local variable, which is not used anymore - most probably we have some another pointer that should be cleared, but it's not available here.. To make it available, bdrv_replace_child_tran() should get BdrvChild **.. maybe later patch will do it > /* old_bs reference is transparently moved from @child to @s */ > } > > @@ -2672,9 +2672,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) > return permissions[qapi_perm]; > } > > -static void bdrv_replace_child_noperm(BdrvChild *child, > +static void bdrv_replace_child_noperm(BdrvChild **childp, > BlockDriverState *new_bs) > { > + BdrvChild *child = *childp; No real logic change for now, OK > BlockDriverState *old_bs = child->bs; > int new_bs_quiesce_counter; > int drain_saldo; > @@ -2767,7 +2768,7 @@ static void bdrv_attach_child_common_abort(void *opaque) > BdrvChild *child = *s->child; > BlockDriverState *bs = child->bs; > > - bdrv_replace_child_noperm(child, NULL); > + bdrv_replace_child_noperm(s->child, NULL); More interesting. Currently bdrv_replace_child_tran() clear the pointer as last action, so later we can remove this last "*s->child = NULL" as bdrv_replace_child_noperm() will do it. No harm: in the the function we use local variable, initialized as *s->child. > > if (bdrv_get_aio_context(bs) != s->old_child_ctx) { > bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); > @@ -2867,7 +2868,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, > } > > bdrv_ref(child_bs); > - bdrv_replace_child_noperm(new_child, child_bs); > + bdrv_replace_child_noperm(&new_child, child_bs); Here child_bs must not be NULL, otherwise bdrv_ref() crashes. So, nothing would be cleared. > > *child = new_child; > > @@ -2922,12 +2923,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, > return 0; > } > > -static void bdrv_detach_child(BdrvChild *child) > +static void bdrv_detach_child(BdrvChild **childp) > { > - BlockDriverState *old_bs = child->bs; > + BlockDriverState *old_bs = (*childp)->bs; > > - bdrv_replace_child_noperm(child, NULL); > - bdrv_child_free(child); > + bdrv_replace_child_noperm(childp, NULL); And here for sure we'll clear the pointer > + bdrv_child_free(*childp); This obviously should be changed in further patches > > if (old_bs) { > /* > @@ -3033,7 +3034,7 @@ void bdrv_root_unref_child(BdrvChild *child) > BlockDriverState *child_bs; > > child_bs = child->bs; > - bdrv_detach_child(child); > + bdrv_detach_child(&child); - no sence / no harm, as it's a local variable, which is not used anymore > bdrv_unref(child_bs); > } > > Patch is correct: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/block.c b/block.c index c7d5aa5254..d668156eca 100644 --- a/block.c +++ b/block.c @@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, static bool bdrv_recurse_has_child(BlockDriverState *bs, BlockDriverState *child); -static void bdrv_replace_child_noperm(BdrvChild *child, +static void bdrv_replace_child_noperm(BdrvChild **child, BlockDriverState *new_bs); static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, BdrvChild *child, @@ -2270,7 +2270,7 @@ static void bdrv_replace_child_abort(void *opaque) BlockDriverState *new_bs = s->child->bs; /* old_bs reference is transparently moved from @s to @s->child */ - bdrv_replace_child_noperm(s->child, s->old_bs); + bdrv_replace_child_noperm(&s->child, s->old_bs); bdrv_unref(new_bs); } @@ -2300,7 +2300,7 @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs, if (new_bs) { bdrv_ref(new_bs); } - bdrv_replace_child_noperm(child, new_bs); + bdrv_replace_child_noperm(&child, new_bs); /* old_bs reference is transparently moved from @child to @s */ } @@ -2672,9 +2672,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) return permissions[qapi_perm]; } -static void bdrv_replace_child_noperm(BdrvChild *child, +static void bdrv_replace_child_noperm(BdrvChild **childp, BlockDriverState *new_bs) { + BdrvChild *child = *childp; BlockDriverState *old_bs = child->bs; int new_bs_quiesce_counter; int drain_saldo; @@ -2767,7 +2768,7 @@ static void bdrv_attach_child_common_abort(void *opaque) BdrvChild *child = *s->child; BlockDriverState *bs = child->bs; - bdrv_replace_child_noperm(child, NULL); + bdrv_replace_child_noperm(s->child, NULL); if (bdrv_get_aio_context(bs) != s->old_child_ctx) { bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); @@ -2867,7 +2868,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, } bdrv_ref(child_bs); - bdrv_replace_child_noperm(new_child, child_bs); + bdrv_replace_child_noperm(&new_child, child_bs); *child = new_child; @@ -2922,12 +2923,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, return 0; } -static void bdrv_detach_child(BdrvChild *child) +static void bdrv_detach_child(BdrvChild **childp) { - BlockDriverState *old_bs = child->bs; + BlockDriverState *old_bs = (*childp)->bs; - bdrv_replace_child_noperm(child, NULL); - bdrv_child_free(child); + bdrv_replace_child_noperm(childp, NULL); + bdrv_child_free(*childp); if (old_bs) { /* @@ -3033,7 +3034,7 @@ void bdrv_root_unref_child(BdrvChild *child) BlockDriverState *child_bs; child_bs = child->bs; - bdrv_detach_child(child); + bdrv_detach_child(&child); bdrv_unref(child_bs); }
bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially set it to NULL. That is dangerous, because BDS parents generally assume that their children's .bs pointer is never NULL. We therefore want to let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer to NULL, too. This patch lays the foundation for it by passing a BdrvChild ** pointer to bdrv_replace_child_noperm() so that it can later use it to NULL the BdrvChild pointer immediately after setting BdrvChild.bs to NULL. (We will still need to undertake some intermediate steps, though.) Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)