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

[Kevin Wolf]

Am 07.07.2016 um 16:17 hat Kevin Wolf geschrieben:
> 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 is what I'm implementing now. The reason for this is that
bdrv_has_blk() obviously rejects configurations where you have only a
node name, but no BB. And the whole point of the series is to move
towards a model without named BBs, so this would mean that you can only
run block job on attached nodes, which doesn't make a lot of sense (and
gives qemu-iotests some trouble).

With this option implemented, a node that isn't attached anywhere can be
used for root node commands, as it should.

Kevin