On 13.02.2017 18:22, Kevin Wolf wrote:
> Block jobs don't actually do I/O through the the reference they create
> with block_job_add_bdrv(), but they might want to use the permisssion
> system to express what the block job does to intermediate nodes. This
> adds permissions to block_job_add_bdrv() to provide the means to request
> permissions.
>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> block/backup.c | 3 ++-
> block/commit.c | 7 +++++--
> block/mirror.c | 7 +++++--
> block/stream.c | 3 ++-
> blockjob.c | 27 +++++++++++++++++++++------
> include/block/blockjob.h | 4 +++-
> 6 files changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 12ab25c..22171f4 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -657,7 +657,8 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT,
> bdi.cluster_size);
> }
>
> - block_job_add_bdrv(&job->common, target);
> + /* FIXME Use real permissions */
> + block_job_add_bdrv(&job->common, target, 0, BLK_PERM_ALL, &error_abort);
> job->common.len = len;
> block_job_txn_add_job(txn, &job->common);
>
> diff --git a/block/commit.c b/block/commit.c
> index 60d29a9..68fa2a4 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -267,13 +267,16 @@ void commit_start(const char *job_id, BlockDriverState
> *bs,
> * disappear from the chain after this operation. */
> assert(bdrv_chain_contains(top, base));
> for (iter = top; iter != backing_bs(base); iter = backing_bs(iter)) {
> - block_job_add_bdrv(&s->common, iter);
> + /* FIXME Use real permissions */
> + block_job_add_bdrv(&s->common, iter, 0, BLK_PERM_ALL, &error_abort);
> }
> /* overlay_bs must be blocked because it needs to be modified to
> * update the backing image string, but if it's the root node then
> * don't block it again */
> if (bs != overlay_bs) {
> - block_job_add_bdrv(&s->common, overlay_bs);
> + /* FIXME Use real permissions */
> + block_job_add_bdrv(&s->common, overlay_bs, 0, BLK_PERM_ALL,
> + &error_abort);
> }
>
> /* FIXME Use real permissions */
> diff --git a/block/mirror.c b/block/mirror.c
> index 22680d7..9532b18 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1020,13 +1020,16 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
> return;
> }
>
> - block_job_add_bdrv(&s->common, target);
> + /* FIXME Use real permissions */
> + block_job_add_bdrv(&s->common, target, 0, BLK_PERM_ALL, &error_abort);
> +
> /* In commit_active_start() all intermediate nodes disappear, so
> * any jobs in them must be blocked */
> if (bdrv_chain_contains(bs, target)) {
> BlockDriverState *iter;
> for (iter = backing_bs(bs); iter != target; iter = backing_bs(iter))
> {
> - block_job_add_bdrv(&s->common, iter);
> + /* FIXME Use real permissions */
> + block_job_add_bdrv(&s->common, iter, 0, BLK_PERM_ALL,
> &error_abort);
> }
> }
>
> diff --git a/block/stream.c b/block/stream.c
> index 7f49279..47f0ffb 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -248,7 +248,8 @@ void stream_start(const char *job_id, BlockDriverState
> *bs,
> /* Block all intermediate nodes between bs and base, because they
> * will disappear from the chain after this operation */
> for (iter = backing_bs(bs); iter && iter != base; iter =
> backing_bs(iter)) {
> - block_job_add_bdrv(&s->common, iter);
> + /* FIXME Use real permissions */
> + block_job_add_bdrv(&s->common, iter, 0, BLK_PERM_ALL, &error_abort);
> }
>
> s->base = base;
> diff --git a/blockjob.c b/blockjob.c
> index 27833c7..0bcc099 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -55,6 +55,10 @@ struct BlockJobTxn {
>
> static QLIST_HEAD(, BlockJob) block_jobs =
> QLIST_HEAD_INITIALIZER(block_jobs);
>
> +static const BdrvChildRole child_job = {
> + .stay_at_node = true,
(1) Not a big fan of such alignment without further users.
(2) No problem though because I'd like to ask for get_link_name() and/or
get_name() here.
It should be easy to generate a nice description when using the BlockJob
as BdrvChild.opaque.
> +};
> +
> BlockJob *block_job_next(BlockJob *job)
> {
> if (!job) {
> @@ -115,11 +119,22 @@ static void block_job_detach_aio_context(void *opaque)
> block_job_unref(job);
> }
>
> -void block_job_add_bdrv(BlockJob *job, BlockDriverState *bs)
> +int block_job_add_bdrv(BlockJob *job, BlockDriverState *bs,
> + uint64_t perm, uint64_t shared_perm, Error **errp)
> {
> - job->nodes = g_slist_prepend(job->nodes, bs);
> + BdrvChild *c;
> +
> + c = bdrv_root_attach_child(bs, "job", &child_job, perm, shared_perm,
"job" doesn't make much sense as the child's name, it's rather a generic
name for the parent.
"job-child" or something caller-provided would be better. (I'd vote for
caller-provided. We don't have that many callers.)
Max
> + NULL, errp);
> + if (c == NULL) {
> + return -EPERM;
> + }
> +
> + job->nodes = g_slist_prepend(job->nodes, c);
> bdrv_ref(bs);
> bdrv_op_block_all(bs, job->blocker);
> +
> + return 0;
> }
>
> void *block_job_create(const char *job_id, const BlockJobDriver *driver,
[...]
signature.asc
Description: OpenPGP digital signature
