|
From: | Max Reitz |
Subject: | Re: [Qemu-block] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd() |
Date: | Sat, 4 Jul 2015 18:13:51 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 |
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)
[Prev in Thread] | Current Thread | [Next in Thread] |