Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/11] block: Accept node-name for block-stream

[Kevin Wolf]

Am 07.07.2016 um 16:39 hat Alberto Garcia geschrieben:
> On Thu 07 Jul 2016 04:17:21 PM CEST, Kevin Wolf wrote:
> >> > +static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp)
> >> > +{
> >> > +    BlockDriverState *bs;
> >> > +
> >> > +    bs = bdrv_lookup_bs(name, name, errp);
> >> > +    if (bs == NULL) {
> >> > +        return NULL;
> >> > +    }
> >> > +
> >> > +    if (!bdrv_has_blk(bs)) {
> >> > +        error_setg(errp, "Need a root block node");
> >> > +        return NULL;
> >> > +    }
> >> 
> >> Since b6d2e59995f when you create a block job a new BlockBackend is
> >> created on top of the BDS. What happens with this check if we allow
> >> creating jobs in a non-root BDS?
> >
> > Hm, you mean I'd start first an intermediate streaming job and then I
> > can call commands on the target node that I shouldn't be able to call?
> > It's a good point, but I think op blockers would prevent that this
> > actually works.
> 
> Yes, they would but
> 
> a) the user would get a misleading error message ("you cannot start that
>    job because the device is temporarily being used" rather than "you
>    cannot start that block job there at all"). Probably not so
>    important.
> 
> b) all the code after the qmp_get_root_bs() call would run under the
>    (incorrect) assumption that the node is a root BDS. This probably
>    doesn't break anything at the moment but leaves a door open for
>    surprises.

Yes, I understand that. The truth is that our current op blockers just
don't quite cut it and we need to move away from them so that we can
finally allow jobs on every node without giving up safety.

> > Another option - and maybe that makes more sense than the old rule
> > anyway because you already can have a BB anywhere in the middle of the
> > graph - would be to check that the node doesn't have any non-BB
> > parent.  This would restrict some cases that are possible today.
> 
> Which ones?

-drive if=none,file=backing.qcow2,id=backing
-drive if=virtio,file=overlay.qcow2,backing=backing

You got a BB name for the backing file node, so can run any command that
wants a root node on it. The backing file blocker will still prevent
most actions, but that's the same situation as with node names.

So I guess the new surprises won't be any worse. :-)

Kevin