Am 26.04.2021 um 19:18 hat Vladimir Sementsov-Ogievskiy geschrieben: > 26.04.2021 19:26, Kevin Wolf wrote: > > Am 17.03.2021 um 15:35 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > > > --- > > > block.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 76 insertions(+), 2 deletions(-) > > > > > > diff --git a/block.c b/block.c > > > index 11f7ad0818..2fca1f2ad5 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -2929,12 +2929,19 @@ static void bdrv_replace_child(BdrvChild *child, > > > BlockDriverState *new_bs) > > > } > > > } > > > +static void bdrv_child_free(void *opaque) > > > +{ > > > + BdrvChild *c = opaque; > > > + > > > + g_free(c->name); > > > + g_free(c); > > > +} > > > + > > > static void bdrv_remove_empty_child(BdrvChild *child) > > > { > > > assert(!child->bs); > > > QLIST_SAFE_REMOVE(child, next); > > > - g_free(child->name); > > > - g_free(child); > > > + bdrv_child_free(child); > > > } > > > typedef struct BdrvAttachChildCommonState { > > > @@ -4956,6 +4963,73 @@ static bool should_update_child(BdrvChild *c, > > > BlockDriverState *to) > > > return ret; > > > } > > > +typedef struct BdrvRemoveFilterOrCowChild { > > > + BdrvChild *child; > > > + bool is_backing; > > > +} BdrvRemoveFilterOrCowChild; > > > + > > > +/* this doesn't restore original child bs, only the child itself */ > > > > Hm, this comment tells me that it's intentional, but why is it correct? > > that's because bdrv_remove_filter_or_cow_child_abort() aborts only > part of bdrv_remove_filter_or_cow_child().
I see that it aborts only part of it, but why? Normally, a function getting a Transaction object suggests to me that on failure, all changes the function made will be reverted, not just parts of the changes. > Look: bdrv_remove_filter_or_cow_child() firstly do > bdrv_replace_child_safe(child, NULL, tran);, so bs would be restored > by .abort() of bdrv_replace_child_safe() action. Ah! So the reason is not that we don't want to restore child->bs, but that bdrv_replace_child_safe() is already transactionable and will automatically do this. > So, improved comment may look like: > > This doesn't restore original child bs, only the child itself. The bs > would be restored by .abort() bdrv_replace_child_safe() subation of > bdrv_remove_filter_or_cow_child() action. "subation" is a typo for "subaction"? Maybe something like this: We don't have to restore child->bs here to undo bdrv_replace_child() because that function is already transactionable and will do so in its own .abort() callback. > Probably it would be more correct to rename > > BdrvRemoveFilterOrCowChild -> BdrvRemoveFilterOrCowChildNoBs > bdrv_remove_filter_or_cow_child_abort -> > bdrv_remove_filter_or_cow_child_no_bs_abort > bdrv_remove_filter_or_cow_child_commit -> > bdrv_remove_filter_or_cow_child_no_bs_commit > > and assert on .abort() and .commit() that s->child->bs is NULL. I wouldn't bother with that. It was just that the comment confused me because it seemed to suggest that we actually don't want to restore child->bs, when its real intention was to say that it's already restored somewhere else. Kevin