| Message ID | 20200428132629.796753-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Do not call BlockDriver.bdrv_make_empty() directly | expand |
On 4/28/20 8:26 AM, Max Reitz wrote: > Two callers of BlockDriver.bdrv_make_empty() remain that should not call > this method directly. Both do not have access to a BdrvChild, but they > can use a BlockBackend, so we add this function that lets them use it. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/sysemu/block-backend.h | 2 ++ > block/block-backend.c | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index d37c1244dd..14338b76dc 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, > > const BdrvChild *blk_root(BlockBackend *blk); > > +int blk_make_empty(BlockBackend *blk, Error **errp); > + Again, a flag parameter to allow use of BDRV_REQ_NO_FALLBACK would be nice. > #endif > diff --git a/block/block-backend.c b/block/block-backend.c > index 3592066b42..5d36efd32f 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk) > { > return blk->root; > } > + > +int blk_make_empty(BlockBackend *blk, Error **errp) > +{ > + return bdrv_make_empty(blk->root, errp); > +} > Otherwise looks fine.
On 4/28/20 8:55 AM, Eric Blake wrote: >> +++ b/include/sysemu/block-backend.h >> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend >> *blk_in, int64_t off_in, >> const BdrvChild *blk_root(BlockBackend *blk); >> +int blk_make_empty(BlockBackend *blk, Error **errp); >> + > > Again, a flag parameter to allow use of BDRV_REQ_NO_FALLBACK would be nice. Or maybe not, after reading Kevin's responses. Making an image empty is not the same as making it read as zero. If we can't come up with a use for a flag, then deferring the addition of a flag until later is a perfectly reasonable approach (rather than adding a flag now that will never get set to anything other than 0). This isn't quite the same as a public API where we would regret being locked out of a flag down the road.
Am 28.04.2020 um 15:26 hat Max Reitz geschrieben: > Two callers of BlockDriver.bdrv_make_empty() remain that should not call > this method directly. Both do not have access to a BdrvChild, but they > can use a BlockBackend, so we add this function that lets them use it. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/sysemu/block-backend.h | 2 ++ > block/block-backend.c | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index d37c1244dd..14338b76dc 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, > > const BdrvChild *blk_root(BlockBackend *blk); > > +int blk_make_empty(BlockBackend *blk, Error **errp); > + > #endif > diff --git a/block/block-backend.c b/block/block-backend.c > index 3592066b42..5d36efd32f 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk) > { > return blk->root; > } > + > +int blk_make_empty(BlockBackend *blk, Error **errp) > +{ > + return bdrv_make_empty(blk->root, errp); > +} Should we check that blk->root != NULL? Most other functions do that through blk_is_available(). Kevin
On 28.04.20 16:47, Kevin Wolf wrote: > Am 28.04.2020 um 15:26 hat Max Reitz geschrieben: >> Two callers of BlockDriver.bdrv_make_empty() remain that should not call >> this method directly. Both do not have access to a BdrvChild, but they >> can use a BlockBackend, so we add this function that lets them use it. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> include/sysemu/block-backend.h | 2 ++ >> block/block-backend.c | 5 +++++ >> 2 files changed, 7 insertions(+) >> >> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h >> index d37c1244dd..14338b76dc 100644 >> --- a/include/sysemu/block-backend.h >> +++ b/include/sysemu/block-backend.h >> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, >> >> const BdrvChild *blk_root(BlockBackend *blk); >> >> +int blk_make_empty(BlockBackend *blk, Error **errp); >> + >> #endif >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 3592066b42..5d36efd32f 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk) >> { >> return blk->root; >> } >> + >> +int blk_make_empty(BlockBackend *blk, Error **errp) >> +{ >> + return bdrv_make_empty(blk->root, errp); >> +} > > Should we check that blk->root != NULL? Most other functions do that > through blk_is_available(). Why not. Max
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index d37c1244dd..14338b76dc 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, const BdrvChild *blk_root(BlockBackend *blk); +int blk_make_empty(BlockBackend *blk, Error **errp); + #endif diff --git a/block/block-backend.c b/block/block-backend.c index 3592066b42..5d36efd32f 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk) { return blk->root; } + +int blk_make_empty(BlockBackend *blk, Error **errp) +{ + return bdrv_make_empty(blk->root, errp); +}
Two callers of BlockDriver.bdrv_make_empty() remain that should not call this method directly. Both do not have access to a BdrvChild, but they can use a BlockBackend, so we add this function that lets them use it. Signed-off-by: Max Reitz <mreitz@redhat.com> --- include/sysemu/block-backend.h | 2 ++ block/block-backend.c | 5 +++++ 2 files changed, 7 insertions(+)