Am 04.10.2017 um 14:23 hat Manos Pitsidianakis geschrieben: > > > diff --git a/block.c b/block.c > > > index 81bd51b670..f874aabbfb 100644 > > > --- a/block.c > > > +++ b/block.c > > > + /* insert 'node' as child bs of 'parent' node */ > > > + if (check_node_edge(parent, child, errp)) { > > > + return; > > > + } > > > + parent_bs = bdrv_find_node(parent); > > > + c = bdrv_find_child(parent_bs, child); > > > + role = c->role; > > > + assert(role == &child_file || role == &child_backing); > > > + > > > + bdrv_ref(node_bs); > > > + > > > + bdrv_drained_begin(parent_bs); > > > + bdrv_unref_child(parent_bs, c); > > > + if (role == &child_file) { > > > + parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file", > > > + &child_file, errp); > > > + if (!parent_bs->file) { > > > + parent_bs->file = bdrv_attach_child(parent_bs, child_bs, > > > "file", > > > + &child_file, > > > &error_abort); > > > + goto out; > > > + } > > > + } else if (role == &child_backing) { > > > + parent_bs->backing = bdrv_attach_child(parent_bs, node_bs, > > > "backing", > > > + &child_backing, errp); > > > + if (!parent_bs->backing) { > > > + parent_bs->backing = bdrv_attach_child(parent_bs, child_bs, > > > + "backing", > > > &child_backing, > > > + &error_abort); > > > + goto out; > > > + } > > > + } > > > > I would prefer if we could find a solution to avoid requiring a specific > > role. I'm not even sure that your assertion above is correct; can you > > explain why c couldn't have any other role? > > > > Instead of bdrv_unref_child/bdrv_attach_child, could we just change > > where the child points to using bdrv_replace_child()? Then > > bdrv_replace_child() uses bdrv_set_perm() and co. When I tried it at first I > got errors like "Conflicts with use by ****** as 'backing', which does not > allow 'write' on disk". Presumably the permissions do not need to change but > can we do bdrv_set_perm without bdrv_check_perm?
Which child is conflicting with which other child? Is c conflicting with itself or something? If unref_child/attach_child works without any other action in between, there is no reason why replace_child shouldn't work, too. Maybe this is a bug in bdrv_ > > parent_bs->file and parent_bs->backing (or whatever other variable > > contains the BdrvChild pointer) can stay unchanged and just keep > > working. > > > > > + bdrv_refresh_filename(parent_bs); > > > + bdrv_refresh_limits(parent_bs, NULL); > > > + > > > +out: > > > + bdrv_drained_end(parent_bs); > > > +} > > > > > diff --git a/blockdev.c b/blockdev.c > > > index 8e2fc6e64c..5195ec1b61 100644 > > > --- a/blockdev.c > > > +++ b/blockdev.c > > > @@ -4238,3 +4238,47 @@ QemuOptsList qemu_drive_opts = { > > > { /* end of list */ } > > > }, > > > }; > > > + > > > +void qmp_block_insert_node(const char *parent, const char *child, > > > + const char *node, Error **errp) > > > +{ > > > + BlockDriverState *bs = bdrv_find_node(node); > > > + if (!bs) { > > > + error_setg(errp, "Node '%s' not found", node); > > > + return; > > > + } > > > + if (!bs->monitor_list.tqe_prev) { > > > + error_setg(errp, "Node '%s' is not owned by the monitor", > > > + bs->node_name); > > > + return; > > > + } > > > + if (!bs->drv->is_filter) { > > > + error_setg(errp, "Block format '%s' used by node '%s' does not > > > support" > > > + "insertion", bs->drv->format_name, bs->node_name); > > > + return; > > > + } > > > + > > > + bdrv_insert_node(parent, child, node, errp); > > > +} > > > > Do we need to acquire an AioContext lock somewhere? > > the *_child() functions call drained_begin/end which I think might cover > this case? I don't think it's enough when you don't own the AioContext lock. Kevin
signature.asc
Description: PGP signature