| Message ID | 20191111160216.197086-7-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix check_to_replace_node() | expand |
11.11.2019 19:01, Max Reitz wrote: > After a couple of follow-up patches, this function will replace > bdrv_recurse_is_first_non_filter() in check_to_replace_node(). > > bdrv_recurse_is_first_non_filter() is both not sufficiently specific for > check_to_replace_node() (it allows cases that should not be allowed, > like replacing child nodes of quorum with dissenting data that have more > parents than just quorum), and it is too restrictive (it is perfectly > fine to replace filters). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 38 ++++++++++++++++++++++++++++++++++++++ > include/block/block_int.h | 10 ++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/block.c b/block.c > index 9b1049786a..de53addeb0 100644 > --- a/block.c > +++ b/block.c > @@ -6205,6 +6205,44 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, > return false; > } > > +/* > + * This function checks whether the given @to_replace is allowed to be > + * replaced by a node that always shows the same data as @bs. This is > + * used for example to verify whether the mirror job can replace > + * @to_replace by the target mirrored from @bs. > + * To be replaceable, @bs and @to_replace may either be guaranteed to > + * always show the same data (because they are only connected through > + * filters), or some driver may allow replacing one of its children > + * because it can guarantee that this child's data is not visible at > + * all (for example, for dissenting quorum children that have no other > + * parents). > + */ > +bool bdrv_recurse_can_replace(BlockDriverState *bs, > + BlockDriverState *to_replace) > +{ > + if (!bs || !bs->drv) { > + return false; > + } > + > + if (bs == to_replace) { > + return true; > + } > + > + /* See what the driver can do */ > + if (bs->drv->bdrv_recurse_can_replace) { > + return bs->drv->bdrv_recurse_can_replace(bs, to_replace); > + } > + > + /* For filters without an own implementation, we can recurse on our own */ > + if (bs->drv->is_filter) { > + BdrvChild *child = bs->file ?: bs->backing; should we check that child != NULL ? > + return bdrv_recurse_can_replace(child->bs, to_replace); > + } > + > + /* Safe default */ > + return false; > +} > + > BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, > const char *node_name, Error **errp) > { > diff --git a/include/block/block_int.h b/include/block/block_int.h > index dd033d0b37..75f03dcc38 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -102,6 +102,13 @@ struct BlockDriver { > */ > bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs, > BlockDriverState *candidate); > + /* > + * Return true if @to_replace can be replaced by a BDS with the > + * same data as @bs without it affecting @bs's behavior (that is, > + * without it being visible to @bs's parents). > + */ > + bool (*bdrv_recurse_can_replace)(BlockDriverState *bs, > + BlockDriverState *to_replace); > > int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename); > int (*bdrv_probe_device)(const char *filename); > @@ -1264,6 +1271,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, > uint64_t perm, uint64_t shared, > uint64_t *nperm, uint64_t *nshared); > > +bool bdrv_recurse_can_replace(BlockDriverState *bs, > + BlockDriverState *to_replace); > + > /* > * Default implementation for drivers to pass bdrv_co_block_status() to > * their file. >
On 29.11.19 10:34, Vladimir Sementsov-Ogievskiy wrote: > 11.11.2019 19:01, Max Reitz wrote: >> After a couple of follow-up patches, this function will replace >> bdrv_recurse_is_first_non_filter() in check_to_replace_node(). >> >> bdrv_recurse_is_first_non_filter() is both not sufficiently specific for >> check_to_replace_node() (it allows cases that should not be allowed, >> like replacing child nodes of quorum with dissenting data that have more >> parents than just quorum), and it is too restrictive (it is perfectly >> fine to replace filters). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 38 ++++++++++++++++++++++++++++++++++++++ >> include/block/block_int.h | 10 ++++++++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/block.c b/block.c >> index 9b1049786a..de53addeb0 100644 >> --- a/block.c >> +++ b/block.c >> @@ -6205,6 +6205,44 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, >> return false; >> } >> >> +/* >> + * This function checks whether the given @to_replace is allowed to be >> + * replaced by a node that always shows the same data as @bs. This is >> + * used for example to verify whether the mirror job can replace >> + * @to_replace by the target mirrored from @bs. >> + * To be replaceable, @bs and @to_replace may either be guaranteed to >> + * always show the same data (because they are only connected through >> + * filters), or some driver may allow replacing one of its children >> + * because it can guarantee that this child's data is not visible at >> + * all (for example, for dissenting quorum children that have no other >> + * parents). >> + */ >> +bool bdrv_recurse_can_replace(BlockDriverState *bs, >> + BlockDriverState *to_replace) >> +{ >> + if (!bs || !bs->drv) { >> + return false; >> + } >> + >> + if (bs == to_replace) { >> + return true; >> + } >> + >> + /* See what the driver can do */ >> + if (bs->drv->bdrv_recurse_can_replace) { >> + return bs->drv->bdrv_recurse_can_replace(bs, to_replace); >> + } >> + >> + /* For filters without an own implementation, we can recurse on our own */ >> + if (bs->drv->is_filter) { >> + BdrvChild *child = bs->file ?: bs->backing; > > should we check that child != NULL ? I’d say that normally (once they are open) filters must have a child, and so I’d make it an assertion. But then again an assertion isn’t much better than the dereferencing that follows, I think. :? Max >> + return bdrv_recurse_can_replace(child->bs, to_replace); >> + } >> + >> + /* Safe default */ >> + return false; >> +} >> + >> BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, >> const char *node_name, Error **errp) >> { >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index dd033d0b37..75f03dcc38 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -102,6 +102,13 @@ struct BlockDriver { >> */ >> bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs, >> BlockDriverState *candidate); >> + /* >> + * Return true if @to_replace can be replaced by a BDS with the >> + * same data as @bs without it affecting @bs's behavior (that is, >> + * without it being visible to @bs's parents). >> + */ >> + bool (*bdrv_recurse_can_replace)(BlockDriverState *bs, >> + BlockDriverState *to_replace); >> >> int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename); >> int (*bdrv_probe_device)(const char *filename); >> @@ -1264,6 +1271,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, >> uint64_t perm, uint64_t shared, >> uint64_t *nperm, uint64_t *nshared); >> >> +bool bdrv_recurse_can_replace(BlockDriverState *bs, >> + BlockDriverState *to_replace); >> + >> /* >> * Default implementation for drivers to pass bdrv_co_block_status() to >> * their file. >> > >
29.11.2019 13:23, Max Reitz wrote: > On 29.11.19 10:34, Vladimir Sementsov-Ogievskiy wrote: >> 11.11.2019 19:01, Max Reitz wrote: >>> After a couple of follow-up patches, this function will replace >>> bdrv_recurse_is_first_non_filter() in check_to_replace_node(). >>> >>> bdrv_recurse_is_first_non_filter() is both not sufficiently specific for >>> check_to_replace_node() (it allows cases that should not be allowed, >>> like replacing child nodes of quorum with dissenting data that have more >>> parents than just quorum), and it is too restrictive (it is perfectly >>> fine to replace filters). >>> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> block.c | 38 ++++++++++++++++++++++++++++++++++++++ >>> include/block/block_int.h | 10 ++++++++++ >>> 2 files changed, 48 insertions(+) >>> >>> diff --git a/block.c b/block.c >>> index 9b1049786a..de53addeb0 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -6205,6 +6205,44 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, >>> return false; >>> } >>> >>> +/* >>> + * This function checks whether the given @to_replace is allowed to be >>> + * replaced by a node that always shows the same data as @bs. This is >>> + * used for example to verify whether the mirror job can replace >>> + * @to_replace by the target mirrored from @bs. >>> + * To be replaceable, @bs and @to_replace may either be guaranteed to >>> + * always show the same data (because they are only connected through >>> + * filters), or some driver may allow replacing one of its children >>> + * because it can guarantee that this child's data is not visible at >>> + * all (for example, for dissenting quorum children that have no other >>> + * parents). >>> + */ >>> +bool bdrv_recurse_can_replace(BlockDriverState *bs, >>> + BlockDriverState *to_replace) >>> +{ >>> + if (!bs || !bs->drv) { >>> + return false; >>> + } >>> + >>> + if (bs == to_replace) { >>> + return true; >>> + } >>> + >>> + /* See what the driver can do */ >>> + if (bs->drv->bdrv_recurse_can_replace) { >>> + return bs->drv->bdrv_recurse_can_replace(bs, to_replace); >>> + } >>> + >>> + /* For filters without an own implementation, we can recurse on our own */ >>> + if (bs->drv->is_filter) { >>> + BdrvChild *child = bs->file ?: bs->backing; >> >> should we check that child != NULL ? > > I’d say that normally (once they are open) filters must have a child, > and so I’d make it an assertion. But then again an assertion isn’t much > better than the dereferencing that follows, I think. :? > > Max OK then. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >>> + return bdrv_recurse_can_replace(child->bs, to_replace); >>> + } >>> + >>> + /* Safe default */ >>> + return false; >>> +} >>> + >>> BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, >>> const char *node_name, Error **errp) >>> { >>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>> index dd033d0b37..75f03dcc38 100644 >>> --- a/include/block/block_int.h >>> +++ b/include/block/block_int.h >>> @@ -102,6 +102,13 @@ struct BlockDriver { >>> */ >>> bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs, >>> BlockDriverState *candidate); >>> + /* >>> + * Return true if @to_replace can be replaced by a BDS with the >>> + * same data as @bs without it affecting @bs's behavior (that is, >>> + * without it being visible to @bs's parents). >>> + */ >>> + bool (*bdrv_recurse_can_replace)(BlockDriverState *bs, >>> + BlockDriverState *to_replace); >>> >>> int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename); >>> int (*bdrv_probe_device)(const char *filename); >>> @@ -1264,6 +1271,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, >>> uint64_t perm, uint64_t shared, >>> uint64_t *nperm, uint64_t *nshared); >>> >>> +bool bdrv_recurse_can_replace(BlockDriverState *bs, >>> + BlockDriverState *to_replace); >>> + >>> /* >>> * Default implementation for drivers to pass bdrv_co_block_status() to >>> * their file. >>> >> >> > >
diff --git a/block.c b/block.c index 9b1049786a..de53addeb0 100644 --- a/block.c +++ b/block.c @@ -6205,6 +6205,44 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, return false; } +/* + * This function checks whether the given @to_replace is allowed to be + * replaced by a node that always shows the same data as @bs. This is + * used for example to verify whether the mirror job can replace + * @to_replace by the target mirrored from @bs. + * To be replaceable, @bs and @to_replace may either be guaranteed to + * always show the same data (because they are only connected through + * filters), or some driver may allow replacing one of its children + * because it can guarantee that this child's data is not visible at + * all (for example, for dissenting quorum children that have no other + * parents). + */ +bool bdrv_recurse_can_replace(BlockDriverState *bs, + BlockDriverState *to_replace) +{ + if (!bs || !bs->drv) { + return false; + } + + if (bs == to_replace) { + return true; + } + + /* See what the driver can do */ + if (bs->drv->bdrv_recurse_can_replace) { + return bs->drv->bdrv_recurse_can_replace(bs, to_replace); + } + + /* For filters without an own implementation, we can recurse on our own */ + if (bs->drv->is_filter) { + BdrvChild *child = bs->file ?: bs->backing; + return bdrv_recurse_can_replace(child->bs, to_replace); + } + + /* Safe default */ + return false; +} + BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, const char *node_name, Error **errp) { diff --git a/include/block/block_int.h b/include/block/block_int.h index dd033d0b37..75f03dcc38 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -102,6 +102,13 @@ struct BlockDriver { */ bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs, BlockDriverState *candidate); + /* + * Return true if @to_replace can be replaced by a BDS with the + * same data as @bs without it affecting @bs's behavior (that is, + * without it being visible to @bs's parents). + */ + bool (*bdrv_recurse_can_replace)(BlockDriverState *bs, + BlockDriverState *to_replace); int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename); int (*bdrv_probe_device)(const char *filename); @@ -1264,6 +1271,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, uint64_t perm, uint64_t shared, uint64_t *nperm, uint64_t *nshared); +bool bdrv_recurse_can_replace(BlockDriverState *bs, + BlockDriverState *to_replace); + /* * Default implementation for drivers to pass bdrv_co_block_status() to * their file.
After a couple of follow-up patches, this function will replace bdrv_recurse_is_first_non_filter() in check_to_replace_node(). bdrv_recurse_is_first_non_filter() is both not sufficiently specific for check_to_replace_node() (it allows cases that should not be allowed, like replacing child nodes of quorum with dissenting data that have more parents than just quorum), and it is too restrictive (it is perfectly fine to replace filters). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 38 ++++++++++++++++++++++++++++++++++++++ include/block/block_int.h | 10 ++++++++++ 2 files changed, 48 insertions(+)