On 01.07.2015 21:41, Alberto Garcia wrote:
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.
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 <mre...@redhat.com>
(I do think it is fine and can't think of any better solution)
