On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz <mre...@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.
>> @@ -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().
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.
Berto
