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

[Max Reitz]

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?

Max