Re: [Qemu-block] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()

[Max Reitz]

On 01.07.2015 21:41, Alberto Garcia wrote:
> On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz <address@hidden> wrote:
>
> > > @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> > > BlockDriverState *backing_hd)
> > >            bs->backing_blocker = NULL;
> > >            goto out;
> > >        }
> > > +
> > > +    bdrv_attach_child(bs, backing_hd, &child_backing);
> > > +    backing_hd->inherits_from = bs;
> > > +    backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags);
> >
> > Do we really want this, unconditionally? ... After looking through the
> > code, I can't find a place where we wouldn't. It just seems strange to
> > have it here.
>
> Yeah, I understand. In general I think that the API for handling
> bs->children is rather unclear and I wanted to avoid that callers need
> to call bdrv_set_backing_hd() and bdrv_attach/detach_child() separately.

Oh, sorry, I was unclear. The bdrv_attach_child() is fine, but I find 
unconditionally inheriting the flags from the backed BDS strange.

> > > @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState 
> > > *parent_bs,
> > >                                  BlockDriverState *child_bs,
> > >                                  const BdrvChildRole *child_role)
> > >    {
> > > -    BdrvChild *child = g_new(BdrvChild, 1);
> > > +    BdrvChild *child;
> > > +
> > > +    /* Don't attach the child if it's already attached */
> > > +    QLIST_FOREACH(child, &parent_bs->children, next) {
> > > +        if (child->bs == child_bs) {
> > > +            return;
> > > +        }
> > > +    }
> >
> > Hm, it may have been attached with a different role, though... I guess
> > that would be a bug, however. But if it's the same role, trying to
> > re-attach it seems wrong, too. So where could this happen?
>
> The reason I'm doing this is because of bdrv_open_backing_file(). That
> function attaches the backing file to the parent file twice: once in
> bdrv_open_inherit() and the second time in bdrv_set_backing_hd().

Okay, that's fine then.

> One alternative would be not to attach it in bdrv_set_backing_hd(), but
> since that function is used in many other places we would have to add
> new calls to bdrv_attach_child() everywhere.
>
> That's one example of the situation I mentioned earlier: it seems
> logical that bdrv_set_backing_hd() and bdrv_attach_child() go together,
> but none of the solutions that came to my mind feels 100% right.

I think putting it into bdrv_set_backing_hd() is fine.

Still feeling a bit bad about overwriting the backing BDS's flags and 
making it inherit its flags from the backed BDS in 
bdrv_set_backing_hd(), but anyway:

Reviewed-by: Max Reitz <address@hidden>

(I do think it is fine and can't think of any better solution)