Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben: > bdrv_append is not very good for inserting filters: it does extra > permission update as part of bdrv_set_backing_hd(). During this update > filter may conflict with other parents of top_bs. > > Instead, let's first do all graph modifications and after it update > permissions.
This sounds like it fixes a bug. If so, should we have a test like for the other cases fixed by this series? > Note: bdrv_append() is still only works for backing-child based > filters. It's something to improve later. > > It simplifies the fact that bdrv_append() used to append new nodes, > without backing child. Let's add an assertion. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > block.c | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/block.c b/block.c > index 02da1a90bc..7094922509 100644 > --- a/block.c > +++ b/block.c > @@ -4998,22 +4998,28 @@ int bdrv_replace_node(BlockDriverState *from, > BlockDriverState *to, > int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, > Error **errp) > { > - Error *local_err = NULL; > + int ret; > + GSList *tran = NULL; > > - bdrv_set_backing_hd(bs_new, bs_top, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return -EPERM; > + assert(!bs_new->backing); > + > + ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing", > + &child_of_bds, bdrv_backing_role(bs_new), > + &bs_new->backing, &tran, errp); > + if (ret < 0) { > + goto out; > } I don't think changing bs->backing without bdrv_set_backing_hd() is correct at the moment. We lose a few things: 1. The bdrv_is_backing_chain_frozen() check 2. Updating backing_hd->inherits_from if necessary 3. bdrv_refresh_limits() If I'm not missing anything, all of these are needed in the context of bdrv_append(). > - bdrv_replace_node(bs_top, bs_new, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - bdrv_set_backing_hd(bs_new, NULL, &error_abort); > - return -EPERM; > + ret = bdrv_replace_node_noperm(bs_top, bs_new, true, &tran, errp); > + if (ret < 0) { > + goto out; > } > > - return 0; > + ret = bdrv_refresh_perms(bs_new, errp); > +out: > + tran_finalize(tran, ret); > + > + return ret; > } Kevin