qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 1/1] block: update BlockDriverState's children i


From: Alberto Garcia
Subject: Re: [Qemu-block] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()
Date: Wed, 01 Jul 2015 21:41:34 +0200
User-agent: Notmuch/0.13.2 (http://notmuchmail.org) Emacs/23.2.1 (i486-pc-linux-gnu)

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.

>> @@ -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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]