Re: [PATCH 01/21] block: introduce bdrv_replace_child_bs()

[Vladimir Sementsov-Ogievskiy]

19.05.2021 14:11, Max Reitz wrote:
> On 19.05.21 12:12, Vladimir Sementsov-Ogievskiy wrote:
> > 17.05.2021 15:09, Max Reitz wrote:
> > > On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
> > > > Add function to transactionally replace bs inside BdrvChild.
> > > >
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > > ---
> > > >   include/block/block.h |  2 ++
> > > >   block.c               | 36 ++++++++++++++++++++++++++++++++++++
> > > >   2 files changed, 38 insertions(+)
> > >
> > > As you may guess, I know little about the rewritten replacing functions, so 
> > > this is kind of difficult to review for me.  However, nothing looks out of 
> > > place, and the function looks sufficiently similar to 
> > > bdrv_replace_node_common() to make me happy.
> > >
> > > > diff --git a/include/block/block.h b/include/block/block.h
> > > > index 82185965ff..f9d5fcb108 100644
> > > > --- a/include/block/block.h
> > > > +++ b/include/block/block.h
> > > > @@ -361,6 +361,8 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState 
> > > > *bs_top,
> > > >                   Error **errp);
> > > >   int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> > > >                         Error **errp);
> > > > +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
> > > > +                          Error **errp);
> > > >   BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
> > > >                                      int flags, Error **errp);
> > > >   int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
> > > > diff --git a/block.c b/block.c
> > > > index 9ad725d205..755fa53d85 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -4961,6 +4961,42 @@ out:
> > > >       return ret;
> > > >   }
> > > > +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
> > > > +                          Error **errp)
> > > > +{
> > > > +    int ret;
> > > > +    Transaction *tran = tran_new();
> > > > +    g_autoptr(GHashTable) found = NULL;
> > > > +    g_autoptr(GSList) refresh_list = NULL;
> > > > +    BlockDriverState *old_bs = child->bs;
> > > > +
> > > > +    if (old_bs) {
> > >
> > > Hm.  Can child->bs be ever NULL?
> >
> > Seems it can. For example we have hmp_drive_del command, that removes bs from 
> > blk :(
> >
> > qmp eject and blockdev-remove-medium seems do it too.
>
> blk_remove_bs() doesn’t eject the BDS from the BdrvChild blk->root, though, it 
> drops blk->root altogether.  Doesn’t it?

A hm, yes. What I say is that we can have empty blk. But probably we should 
never have BdrvChild with NULL bs. I'll check it


--
Best regards,
Vladimir