| Message ID | 20200218124242.584644-19-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Introduce real BdrvChildRole | expand |
Am 18.02.2020 um 13:42 hat Max Reitz geschrieben: > This callback can be used by BDSs that use child_of_bds with the > appropriate BdrvChildRole for their children. > > Also, make bdrv_format_default_perms() use it for child_of_bds children > (just a temporary solution until we can drop bdrv_format_default_perms() > altogether). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > block.c | 46 ++++++++++++++++++++++++++++++++------- > include/block/block_int.h | 11 ++++++++++ > 2 files changed, 49 insertions(+), 8 deletions(-) > > diff --git a/block.c b/block.c > index c0ba274743..3e5b0bc345 100644 > --- a/block.c > +++ b/block.c > @@ -2383,14 +2383,12 @@ static void bdrv_default_perms_for_metadata(BlockDriverState *bs, BdrvChild *c, > *nshared = shared; > } > > -/* TODO: Use */ > -static void __attribute__((unused)) > -bdrv_default_perms_for_data(BlockDriverState *bs, BdrvChild *c, > - const BdrvChildClass *child_class, > - BdrvChildRole role, > - BlockReopenQueue *reopen_queue, > - uint64_t perm, uint64_t shared, > - uint64_t *nperm, uint64_t *nshared) > +static void bdrv_default_perms_for_data(BlockDriverState *bs, BdrvChild *c, > + const BdrvChildClass *child_class, > + BdrvChildRole role, > + BlockReopenQueue *reopen_queue, > + uint64_t perm, uint64_t shared, > + uint64_t *nperm, uint64_t *nshared) > { > assert(child_class == &child_of_bds && (role & BDRV_CHILD_DATA)); > > @@ -2425,6 +2423,13 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, > uint64_t *nperm, uint64_t *nshared) > { > bool backing = (child_class == &child_backing); > + > + if (child_class == &child_of_bds) { > + bdrv_default_perms(bs, c, child_class, role, reopen_queue, > + perm, shared, nperm, nshared); > + return; > + } > + > assert(child_class == &child_backing || child_class == &child_file); > > if (!backing) { > @@ -2436,6 +2441,31 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, > } > } > > +void bdrv_default_perms(BlockDriverState *bs, BdrvChild *c, > + const BdrvChildClass *child_class, BdrvChildRole role, > + BlockReopenQueue *reopen_queue, > + uint64_t perm, uint64_t shared, > + uint64_t *nperm, uint64_t *nshared) > +{ > + assert(child_class == &child_of_bds); > + > + if (role & BDRV_CHILD_FILTERED) { > + bdrv_filter_default_perms(bs, c, child_class, role, reopen_queue, > + perm, shared, nperm, nshared); > + } else if (role & BDRV_CHILD_COW) { > + bdrv_default_perms_for_backing(bs, c, child_class, role, reopen_queue, > + perm, shared, nperm, nshared); > + } else if (role & BDRV_CHILD_METADATA) { > + bdrv_default_perms_for_metadata(bs, c, child_class, role, reopen_queue, > + perm, shared, nperm, nshared); > + } else if (role & BDRV_CHILD_DATA) { > + bdrv_default_perms_for_data(bs, c, child_class, role, reopen_queue, > + perm, shared, nperm, nshared); > + } else { > + g_assert_not_reached(); > + } > +} Here the discussion from the start of the series becomes relevant: Which flags can be combined with which other flags, and if multiple flags are set, which one takes precedence? First undocumented requirement: You must set at least one of FILTERED, COW, METADATA and DATA. Then, for FILTERED we decided to document that DATA shouldn't be set at the same time. I guess neither should COW and METADATA. Apart for documentation, worth an assertion? COW seems to be exclusive in practice, too. I guess you could construct a driver that somehow keeps its own (meta)data in its backing file, maybe in the VM state area. It also sounds like a really bad idea. So forbid it, document it and assert it, too? METADATA and DATA can be set at the same time. As your previous patch shows, the function for DATA is a laxer version of the one for METADATA, requesting a subset of the METADATA permissions and sharing a superset. So the order in the code makes sense. But can we make sure that this condition will be true in the future? Imagine we find a need for a new permission that is used for data files, but not for metadata. I think at the very least, this deserves a comment. But probably it means that both should stay a single function that can check each flag for the exact permission bits it influences. Kevin
On 06.05.20 15:47, Kevin Wolf wrote: > Am 18.02.2020 um 13:42 hat Max Reitz geschrieben: >> This callback can be used by BDSs that use child_of_bds with the >> appropriate BdrvChildRole for their children. >> >> Also, make bdrv_format_default_perms() use it for child_of_bds children >> (just a temporary solution until we can drop bdrv_format_default_perms() >> altogether). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> block.c | 46 ++++++++++++++++++++++++++++++++------- >> include/block/block_int.h | 11 ++++++++++ >> 2 files changed, 49 insertions(+), 8 deletions(-) >> >> diff --git a/block.c b/block.c >> index c0ba274743..3e5b0bc345 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2383,14 +2383,12 @@ static void bdrv_default_perms_for_metadata(BlockDriverState *bs, BdrvChild *c, >> *nshared = shared; >> } >> >> -/* TODO: Use */ >> -static void __attribute__((unused)) >> -bdrv_default_perms_for_data(BlockDriverState *bs, BdrvChild *c, >> - const BdrvChildClass *child_class, >> - BdrvChildRole role, >> - BlockReopenQueue *reopen_queue, >> - uint64_t perm, uint64_t shared, >> - uint64_t *nperm, uint64_t *nshared) >> +static void bdrv_default_perms_for_data(BlockDriverState *bs, BdrvChild *c, >> + const BdrvChildClass *child_class, >> + BdrvChildRole role, >> + BlockReopenQueue *reopen_queue, >> + uint64_t perm, uint64_t shared, >> + uint64_t *nperm, uint64_t *nshared) >> { >> assert(child_class == &child_of_bds && (role & BDRV_CHILD_DATA)); >> >> @@ -2425,6 +2423,13 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, >> uint64_t *nperm, uint64_t *nshared) >> { >> bool backing = (child_class == &child_backing); >> + >> + if (child_class == &child_of_bds) { >> + bdrv_default_perms(bs, c, child_class, role, reopen_queue, >> + perm, shared, nperm, nshared); >> + return; >> + } >> + >> assert(child_class == &child_backing || child_class == &child_file); >> >> if (!backing) { >> @@ -2436,6 +2441,31 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, >> } >> } >> >> +void bdrv_default_perms(BlockDriverState *bs, BdrvChild *c, >> + const BdrvChildClass *child_class, BdrvChildRole role, >> + BlockReopenQueue *reopen_queue, >> + uint64_t perm, uint64_t shared, >> + uint64_t *nperm, uint64_t *nshared) >> +{ >> + assert(child_class == &child_of_bds); >> + >> + if (role & BDRV_CHILD_FILTERED) { >> + bdrv_filter_default_perms(bs, c, child_class, role, reopen_queue, >> + perm, shared, nperm, nshared); >> + } else if (role & BDRV_CHILD_COW) { >> + bdrv_default_perms_for_backing(bs, c, child_class, role, reopen_queue, >> + perm, shared, nperm, nshared); >> + } else if (role & BDRV_CHILD_METADATA) { >> + bdrv_default_perms_for_metadata(bs, c, child_class, role, reopen_queue, >> + perm, shared, nperm, nshared); >> + } else if (role & BDRV_CHILD_DATA) { >> + bdrv_default_perms_for_data(bs, c, child_class, role, reopen_queue, >> + perm, shared, nperm, nshared); >> + } else { >> + g_assert_not_reached(); >> + } >> +} > > Here the discussion from the start of the series becomes relevant: Which > flags can be combined with which other flags, and if multiple flags are > set, which one takes precedence? > > First undocumented requirement: You must set at least one of FILTERED, > COW, METADATA and DATA. > > Then, for FILTERED we decided to document that DATA shouldn't be set at > the same time. I guess neither should COW and METADATA. Apart for > documentation, worth an assertion? > > COW seems to be exclusive in practice, too. I guess you could construct > a driver that somehow keeps its own (meta)data in its backing file, > maybe in the VM state area. It also sounds like a really bad idea. So > forbid it, document it and assert it, too? Sounds all good. > METADATA and DATA can be set at the same time. As your previous patch > shows, the function for DATA is a laxer version of the one for METADATA, > requesting a subset of the METADATA permissions and sharing a superset. > So the order in the code makes sense. > > But can we make sure that this condition will be true in the future? > Imagine we find a need for a new permission that is used for data files, > but not for metadata. I think at the very least, this deserves a > comment. But probably it means that both should stay a single function > that can check each flag for the exact permission bits it influences. Maybe, I’ll see whether it looks good. If it doesn’t, I could also rename the _metadata function to _storage, so that it’s generally a function that handles metadata+data children (i.e. defined to be stricter than the _data function). Max
diff --git a/block.c b/block.c index c0ba274743..3e5b0bc345 100644 --- a/block.c +++ b/block.c @@ -2383,14 +2383,12 @@ static void bdrv_default_perms_for_metadata(BlockDriverState *bs, BdrvChild *c, *nshared = shared; } -/* TODO: Use */ -static void __attribute__((unused)) -bdrv_default_perms_for_data(BlockDriverState *bs, BdrvChild *c, - const BdrvChildClass *child_class, - BdrvChildRole role, - BlockReopenQueue *reopen_queue, - uint64_t perm, uint64_t shared, - uint64_t *nperm, uint64_t *nshared) +static void bdrv_default_perms_for_data(BlockDriverState *bs, BdrvChild *c, + const BdrvChildClass *child_class, + BdrvChildRole role, + BlockReopenQueue *reopen_queue, + uint64_t perm, uint64_t shared, + uint64_t *nperm, uint64_t *nshared) { assert(child_class == &child_of_bds && (role & BDRV_CHILD_DATA)); @@ -2425,6 +2423,13 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, uint64_t *nperm, uint64_t *nshared) { bool backing = (child_class == &child_backing); + + if (child_class == &child_of_bds) { + bdrv_default_perms(bs, c, child_class, role, reopen_queue, + perm, shared, nperm, nshared); + return; + } + assert(child_class == &child_backing || child_class == &child_file); if (!backing) { @@ -2436,6 +2441,31 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, } } +void bdrv_default_perms(BlockDriverState *bs, BdrvChild *c, + const BdrvChildClass *child_class, BdrvChildRole role, + BlockReopenQueue *reopen_queue, + uint64_t perm, uint64_t shared, + uint64_t *nperm, uint64_t *nshared) +{ + assert(child_class == &child_of_bds); + + if (role & BDRV_CHILD_FILTERED) { + bdrv_filter_default_perms(bs, c, child_class, role, reopen_queue, + perm, shared, nperm, nshared); + } else if (role & BDRV_CHILD_COW) { + bdrv_default_perms_for_backing(bs, c, child_class, role, reopen_queue, + perm, shared, nperm, nshared); + } else if (role & BDRV_CHILD_METADATA) { + bdrv_default_perms_for_metadata(bs, c, child_class, role, reopen_queue, + perm, shared, nperm, nshared); + } else if (role & BDRV_CHILD_DATA) { + bdrv_default_perms_for_data(bs, c, child_class, role, reopen_queue, + perm, shared, nperm, nshared); + } else { + g_assert_not_reached(); + } +} + uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) { static const uint64_t permissions[] = { diff --git a/include/block/block_int.h b/include/block/block_int.h index dd7ccea35e..d1d1af2a5c 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1279,6 +1279,17 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, bool bdrv_recurse_can_replace(BlockDriverState *bs, BlockDriverState *to_replace); +/* + * Default implementation for BlockDriver.bdrv_child_perm() that can + * be used by block filters and image formats, as long as they use the + * child_of_bds child class and set an appropriate BdrvChildRole. + */ +void bdrv_default_perms(BlockDriverState *bs, BdrvChild *c, + const BdrvChildClass *child_class, BdrvChildRole role, + BlockReopenQueue *reopen_queue, + uint64_t perm, uint64_t shared, + uint64_t *nperm, uint64_t *nshared); + /* * Default implementation for drivers to pass bdrv_co_block_status() to * their file.