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

[Kevin Wolf]

Am 07.07.2016 um 14:59 hat Alberto Garcia geschrieben:
> On Thu 07 Jul 2016 02:11:27 PM CEST, Kevin Wolf wrote:
> > In order to remove the necessity to use BlockBackend names in the
> > external API, we want to allow node-names everywhere. This converts
> > block-stream to accept a node-name without lifting the restriction that
> > we're operating at a root node.
> >
> > In case of an invalid device name, the command returns the GenericError
> > error class now instead of DeviceNotFound, because this is what
> > qmp_get_root_bs() returns.
> >
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  blockdev.c             | 32 ++++++++++++++++++++------------
> >  qapi/block-core.json   |  5 +----
> >  qmp-commands.hx        |  2 +-
> >  tests/qemu-iotests/030 |  2 +-
> >  4 files changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 0f8065c..01e57c9 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1172,6 +1172,23 @@ fail:
> >      return dinfo;
> >  }
> >  
> > +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.

If we wanted to keep exactly the old set of nodes that is allowed, we
could make qmp_get_root_bs() look for a _named_ BlockBackend. But that
would kind of defeat the purpose of this series, which wants to allow
these commands on named nodes that are directly used for -device.

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.

Or, considering that requiring a device name didn't really work as a
check because you can have BBs anywhere, just leave it. Sooner or later
we'll want all commands to work on any node anyway. I think the main
reason to require root nodes today is just op blockers.

Kevin