| Message ID | 20200204170848.614480-33-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Introduce real BdrvChildRole | expand |
On 2/4/20 11:08 AM, Max Reitz wrote: > These calls have no real use for the child role yet, but it will not > harm to give one. > > Notably, the bdrv_root_attach_child() call in blockjob.c is left > unmodified because there is not much the generic BlockJob object wants > from its children. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/block-backend.c | 11 +++++++---- > block/vvfat.c | 2 +- > 2 files changed, 8 insertions(+), 5 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> Is it worth an assert(role) somewhere now that you've converted all callers to pass at least one role?
On 11.02.20 16:53, Eric Blake wrote: > On 2/4/20 11:08 AM, Max Reitz wrote: >> These calls have no real use for the child role yet, but it will not >> harm to give one. >> >> Notably, the bdrv_root_attach_child() call in blockjob.c is left >> unmodified because there is not much the generic BlockJob object wants >> from its children. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/block-backend.c | 11 +++++++---- >> block/vvfat.c | 2 +- >> 2 files changed, 8 insertions(+), 5 deletions(-) >> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Is it worth an assert(role) somewhere now that you've converted all > callers to pass at least one role? Well, as the commit message states, block_job_add_bdrv() in blockjob.c still passes BdrvChildRole=0 to bdrv_root_attach_child(). So it depends on what function we’re looking at. I suppose we could add such an assertion to bdrv_attach_child() because we could expect all BDSs to pass some role for their children. OTOH, maybe a BDS has a legitimate reason not to: Maybe it just wants to take some permissions on some BDS without having any real relationship to it. Right now, some block jobs do that, well, except they do so through the back door of adding the child BDS to the block job object (which then passes no child role). So maybe I’d actually rather not add such an assertion anywhere. Max
On 2/18/20 6:01 AM, Max Reitz wrote: >> >> Is it worth an assert(role) somewhere now that you've converted all >> callers to pass at least one role? > > Well, as the commit message states, block_job_add_bdrv() in blockjob.c > still passes BdrvChildRole=0 to bdrv_root_attach_child(). So it depends > on what function we’re looking at. > > I suppose we could add such an assertion to bdrv_attach_child() because > we could expect all BDSs to pass some role for their children. > > OTOH, maybe a BDS has a legitimate reason not to: Maybe it just wants to > take some permissions on some BDS without having any real relationship > to it. Right now, some block jobs do that, well, except they do so > through the back door of adding the child BDS to the block job object > (which then passes no child role). So maybe I’d actually rather not add > such an assertion anywhere. Fair enough - you have more knowledge of which callers remain that still have a legitimate reason to not request a role.
diff --git a/block/block-backend.c b/block/block-backend.c index 9e0078bfb5..e655e15675 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -401,8 +401,9 @@ BlockBackend *blk_new_open(const char *filename, const char *reference, return NULL; } - blk->root = bdrv_root_attach_child(bs, "root", &child_root, 0, blk->ctx, - perm, BLK_PERM_ALL, blk, errp); + blk->root = bdrv_root_attach_child(bs, "root", &child_root, + BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, + blk->ctx, perm, BLK_PERM_ALL, blk, errp); if (!blk->root) { blk_unref(blk); return NULL; @@ -812,8 +813,10 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp) { ThrottleGroupMember *tgm = &blk->public.throttle_group_member; bdrv_ref(bs); - blk->root = bdrv_root_attach_child(bs, "root", &child_root, 0, blk->ctx, - blk->perm, blk->shared_perm, blk, errp); + blk->root = bdrv_root_attach_child(bs, "root", &child_root, + BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, + blk->ctx, blk->perm, blk->shared_perm, + blk, errp); if (blk->root == NULL) { return -EPERM; } diff --git a/block/vvfat.c b/block/vvfat.c index 2bc5812dc5..96d7c6eca8 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -3193,7 +3193,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp) options = qdict_new(); qdict_put_str(options, "write-target.driver", "qcow"); s->qcow = bdrv_open_child(s->qcow_filename, options, "write-target", bs, - &child_vvfat_qcow, 0, false, errp); + &child_vvfat_qcow, BDRV_CHILD_DATA, false, errp); qobject_unref(options); if (!s->qcow) { ret = -EINVAL;
These calls have no real use for the child role yet, but it will not harm to give one. Notably, the bdrv_root_attach_child() call in blockjob.c is left unmodified because there is not much the generic BlockJob object wants from its children. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/block-backend.c | 11 +++++++---- block/vvfat.c | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-)