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

[Max Reitz <mreitz@redhat.com>]

On 01.07.2015 21:41, Alberto Garcia wrote:
> On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz <mreitz@redhat.com> 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 <mreitz@redhat.com>

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