Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4278 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]