Re: [PATCH v2 23/36] block: adapt bdrv_append() for inserting filters

[Vladimir Sementsov-Ogievskiy]

04.02.2021 12:05, Kevin Wolf wrote:
> Am 04.02.2021 um 09:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 04.02.2021 00:33, Kevin Wolf wrote:
> > > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > >    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().
> >
> > I decided that bdrv_append() is only for appending new nodes, so
> > frozen and inherts_from checks are not needed. And I've added
> > assert(!bs_new->backing)...
> >
> > Checking this now:
> >
> > - appending filters is obvious
> > - bdrv_append_temp_snapshot() creates new qcow2 node based on tmp
> >    file, don't see any backing initialization (and it would be rather
> >    strange)
>
> Yes, the internal uses are obviously unproblematic for the frozen check.
>
> > - external_snapshot_prepare() do check if
> >    (bdrv_cow_child(state->new_bs)) {  error-out }
>
> Ok, the only thing bdrv_set_backing_hd() can and must check is whether
> the link to the old backing file was frozen, and we know that we don't
> have an old backing file. Makes sense.
>
> Same thing for inherits_from, we only do this if the the new backing
> file (i.e. the old active layer for bdrv_append) was already in the
> backing chain of the new node.
>
> > So everything is OK. I should describe it in commit message and add a
> > comment to bdrv_append.
>
> What about bdrv_refresh_limits()? The node gains a new backing file, so
> I think the limits could change.
>
> Ideally, bdrv_child_cb_attach/detach() would take care of this, but at
> the moment they don't.

when answering I thought that it is called at the end of a function. But I both 
forget to write it in the answer and was wrong :) As it's actually 
bdrv_refresh_perms().  I'll add call of bdrv_refresh_limits()


--
Best regards,
Vladimir