On 04.04.2016 15:43, Alberto Garcia wrote: > Currently, block jobs can only be owned by root nodes. This patch > allows block jobs to be in any arbitrary node, by making the following > changes: > > - Block jobs can now be identified by the node name of their > BlockDriverState in addition to the device name. Since both device > and node names live in the same namespace there's no ambiguity. > > - The "device" parameter used by all commands that operate on block > jobs can also be a node name now. > > - The node name is used as a fallback to fill in the BlockJobInfo > structure and all BLOCK_JOB_* events if there is no device name for > that job. > > Signed-off-by: Alberto Garcia <[email protected]> > --- > blockdev.c | 18 ++++++++++-------- > blockjob.c | 5 +++-- > docs/qmp-events.txt | 8 ++++---- > qapi/block-core.json | 20 ++++++++++---------- > 4 files changed, 27 insertions(+), 24 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index edbcc19..d1f5dfb 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3685,8 +3685,10 @@ void qmp_blockdev_mirror(const char *device, const > char *target, > aio_context_release(aio_context); > } > > -/* Get the block job for a given device name and acquire its AioContext */ > -static BlockJob *find_block_job(const char *device, AioContext **aio_context, > +/* Get the block job for a given device or node name > + * and acquire its AioContext */ > +static BlockJob *find_block_job(const char *device_or_node, > + AioContext **aio_context, > Error **errp) > { > BlockBackend *blk; > @@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *device, > AioContext **aio_context, > > *aio_context = NULL; > > - blk = blk_by_name(device); > - if (!blk) { > + bs = bdrv_lookup_bs(device_or_node, device_or_node, errp); > + if (!bs) {
If we get here, *errp is already set... [1]
> goto notfound;
> }
>
> - *aio_context = blk_get_aio_context(blk);
> + *aio_context = bdrv_get_aio_context(bs);
> aio_context_acquire(*aio_context);
>
> - if (!blk_is_available(blk)) {
> + blk = blk_by_name(device_or_node);
> + if (blk && !blk_is_available(blk)) {
I'd just drop this. The reason blk_is_available() was added here because
it became possible for BBs not to have a BDS.
Now that you get the BDS directly through bdrv_lookup_bs(), it's no
longer necessary.
> goto notfound;
> }
> - bs = blk_bs(blk);
>
> if (!bs->job) {
> goto notfound;
> @@ -3715,7 +3717,7 @@ static BlockJob *find_block_job(const char *device,
> AioContext **aio_context,
>
> notfound:
> error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> - "No active block job on device '%s'", device);
> + "No active block job on node '%s'", device_or_node);
[1] ...and then we'll get a failed assertion here (through error_setv()).
> if (*aio_context) {
> aio_context_release(*aio_context);
> *aio_context = NULL;
> diff --git a/blockjob.c b/blockjob.c
> index 3557048..2ab4794 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -67,7 +67,8 @@ void *block_job_create(const BlockJobDriver *driver,
> BlockDriverState *bs,
> BlockJob *job;
>
> if (bs->job) {
> - error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
> + error_setg(errp, "Node '%s' is in use",
> + bdrv_get_device_or_node_name(bs));
> return NULL;
> }
> bdrv_ref(bs);
> @@ -78,7 +79,7 @@ void *block_job_create(const BlockJobDriver *driver,
> BlockDriverState *bs,
> bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
>
> job->driver = driver;
> - job->id = g_strdup(bdrv_get_device_name(bs));
> + job->id = g_strdup(bdrv_get_device_or_node_name(bs));
Hm, since this is only used for events, it's not too bad that a block
job will have a different name if it was created on a root BDS by
specifying its node name. But it still is kind of strange.
But as I said, it should be fine as long as one can still use the node
name for controlling it (which is the case).
Max
signature.asc
Description: OpenPGP digital signature
