[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command |
Date: |
Wed, 4 Oct 2017 14:53:17 +0200 |
User-agent: |
Mutt/1.9.0 (2017-09-02) |
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