[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 01/11] block: Accept node-name for block-stre
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v3 01/11] block: Accept node-name for block-stream |
Date: |
Wed, 13 Jul 2016 11:46:58 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
[Qemu-block] [PATCH v3 02/11] block: Accept node-name for block-commit, Kevin Wolf, 2016/07/07
[Qemu-block] [PATCH v3 03/11] block: Accept node-name for blockdev-backup, Kevin Wolf, 2016/07/07
[Qemu-block] [PATCH v3 04/11] block: Accept node-name for blockdev-mirror, Kevin Wolf, 2016/07/07
[Qemu-block] [PATCH v3 05/11] block: Accept node-name for blockdev-snapshot-delete-internal-sync, Kevin Wolf, 2016/07/07
[Qemu-block] [PATCH v3 06/11] block: Accept node-name for blockdev-snapshot-internal-sync, Kevin Wolf, 2016/07/07
[Qemu-block] [PATCH v3 07/11] block: Accept node-name for change-backing-file, Kevin Wolf, 2016/07/07