Message ID | 20190911150054.90936-3-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | proper locking on bitmap add/remove paths | expand |
On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote: > It's more comfortable to not deal with local_err. > I agree. > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.h | 5 ++--- > include/block/block_int.h | 6 +++--- > include/block/dirty-bitmap.h | 5 ++--- > block/dirty-bitmap.c | 9 +++++---- > block/qcow2-bitmap.c | 20 +++++++++++--------- > blockdev.c | 7 +++---- > 6 files changed, 26 insertions(+), 26 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 998bcdaef1..99ee88f802 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > const char *name, > uint32_t granularity, > Error **errp); > -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > - const char *name, > - Error **errp); > +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, > + Error **errp); > > ssize_t coroutine_fn > qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size, > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 0422acdf1c..503ac9e3cd 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -556,9 +556,9 @@ struct BlockDriver { > const char *name, > uint32_t granularity, > Error **errp); > - void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, > - const char *name, > - Error **errp); > + int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, > + const char *name, > + Error **errp); > > /** > * Register/unregister a buffer for I/O. For example, when the driver is > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h > index 4b4b731b46..07503b03b5 100644 > --- a/include/block/dirty-bitmap.h > +++ b/include/block/dirty-bitmap.h > @@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, > Error **errp); > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); > void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); > -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, > - const char *name, > - Error **errp); > +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, > + Error **errp); > void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap); > void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap); > void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap); > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 8f42015db9..a52b83b619 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) > * not fail. > * This function doesn't release corresponding BdrvDirtyBitmap. > */ > -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, > - const char *name, > - Error **errp) > +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, > + Error **errp) > { > if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) { > - bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp); > + return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp); > } > + But is it a problem if we return an error code without setting errp now? If this is for the sake of not having to deal with local_err, we should make sure that a non-zero return means that errp is set. Right? > + return -ENOTSUP; > } > > bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index b2487101ed..1aaedb3b55 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1404,11 +1404,10 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, > return NULL; > } > > -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > - const char *name, > - Error **errp) > +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, > + Error **errp) > { > - int ret; > + int ret = 0; I was going to say I'd rather not initialize this, but is this related to ubsan linting? > BDRVQcow2State *s = bs->opaque; > Qcow2Bitmap *bm; > Qcow2BitmapList *bm_list; > @@ -1416,18 +1415,19 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > if (s->nb_bitmaps == 0) { > /* Absence of the bitmap is not an error: see explanation above > * bdrv_remove_persistent_dirty_bitmap() definition. */ > - return; > + return 0; > } > > bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, > s->bitmap_directory_size, errp); > if (bm_list == NULL) { > - return; > + return -EIO; > } > > bm = find_bitmap_by_name(bm_list, name); > if (bm == NULL) { > - goto fail; > + ret = -EINVAL; > + goto out; > } > > QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry); > @@ -1435,14 +1435,16 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > ret = update_ext_header_and_dir(bs, bm_list); > if (ret < 0) { > error_setg_errno(errp, -ret, "Failed to update bitmap extension"); > - goto fail; > + goto out; > } > > free_bitmap_clusters(bs, &bm->table); > > -fail: > +out: > bitmap_free(bm); > bitmap_list_free(bm_list); > + > + return ret; > } > > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > diff --git a/blockdev.c b/blockdev.c > index fbef6845c8..0813adfb2b 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2940,15 +2940,14 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( > } > > if (bdrv_dirty_bitmap_get_persistence(bitmap)) { > + int ret; > AioContext *aio_context = bdrv_get_aio_context(bs); > - Error *local_err = NULL; > > aio_context_acquire(aio_context); > - bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err); > + ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp); > aio_context_release(aio_context); > > - if (local_err != NULL) { > - error_propagate(errp, local_err); > + if (ret < 0) { > return NULL; > } > } >
14.09.2019 1:01, John Snow wrote: > > > On 9/11/19 11:00 AM, Vladimir Sementsov-Ogievskiy wrote: >> It's more comfortable to not deal with local_err. >> > > I agree. > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2.h | 5 ++--- >> include/block/block_int.h | 6 +++--- >> include/block/dirty-bitmap.h | 5 ++--- >> block/dirty-bitmap.c | 9 +++++---- >> block/qcow2-bitmap.c | 20 +++++++++++--------- >> blockdev.c | 7 +++---- >> 6 files changed, 26 insertions(+), 26 deletions(-) >> >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 998bcdaef1..99ee88f802 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >> const char *name, >> uint32_t granularity, >> Error **errp); >> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> - const char *name, >> - Error **errp); >> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, >> + Error **errp); >> >> ssize_t coroutine_fn >> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size, >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index 0422acdf1c..503ac9e3cd 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -556,9 +556,9 @@ struct BlockDriver { >> const char *name, >> uint32_t granularity, >> Error **errp); >> - void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, >> - const char *name, >> - Error **errp); >> + int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, >> + const char *name, >> + Error **errp); >> >> /** >> * Register/unregister a buffer for I/O. For example, when the driver is >> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >> index 4b4b731b46..07503b03b5 100644 >> --- a/include/block/dirty-bitmap.h >> +++ b/include/block/dirty-bitmap.h >> @@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, >> Error **errp); >> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); >> void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); >> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> - const char *name, >> - Error **errp); >> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, >> + Error **errp); >> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap); >> void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap); >> void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap); >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 8f42015db9..a52b83b619 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) >> * not fail. >> * This function doesn't release corresponding BdrvDirtyBitmap. >> */ >> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> - const char *name, >> - Error **errp) >> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, >> + Error **errp) >> { >> if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) { >> - bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp); >> + return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp); >> } >> + > > But is it a problem if we return an error code without setting errp now? > If this is for the sake of not having to deal with local_err, we should > make sure that a non-zero return means that errp is set. Right? Oops, of course, I should set errp here > >> + return -ENOTSUP; >> } >> >> bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index b2487101ed..1aaedb3b55 100644 >> --- a/block/qcow2-bitmap.c >> +++ b/block/qcow2-bitmap.c >> @@ -1404,11 +1404,10 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, >> return NULL; >> } >> >> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> - const char *name, >> - Error **errp) >> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, >> + Error **errp) >> { >> - int ret; >> + int ret = 0; > > I was going to say I'd rather not initialize this, but is this related > to ubsan linting? Hmm, don't remember. For some reason I thought it's needed.. Will drop and check what compiler says. > >> BDRVQcow2State *s = bs->opaque; >> Qcow2Bitmap *bm; >> Qcow2BitmapList *bm_list; >> @@ -1416,18 +1415,19 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> if (s->nb_bitmaps == 0) { >> /* Absence of the bitmap is not an error: see explanation above >> * bdrv_remove_persistent_dirty_bitmap() definition. */ >> - return; >> + return 0; >> } >> >> bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, >> s->bitmap_directory_size, errp); >> if (bm_list == NULL) { >> - return; >> + return -EIO; >> } >> >> bm = find_bitmap_by_name(bm_list, name); >> if (bm == NULL) { >> - goto fail; >> + ret = -EINVAL; >> + goto out; >> } >> >> QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry); >> @@ -1435,14 +1435,16 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> ret = update_ext_header_and_dir(bs, bm_list); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Failed to update bitmap extension"); >> - goto fail; >> + goto out; >> } >> >> free_bitmap_clusters(bs, &bm->table); >> >> -fail: >> +out: >> bitmap_free(bm); >> bitmap_list_free(bm_list); >> + >> + return ret; >> } >> >> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) >> diff --git a/blockdev.c b/blockdev.c >> index fbef6845c8..0813adfb2b 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2940,15 +2940,14 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( >> } >> >> if (bdrv_dirty_bitmap_get_persistence(bitmap)) { >> + int ret; >> AioContext *aio_context = bdrv_get_aio_context(bs); >> - Error *local_err = NULL; >> >> aio_context_acquire(aio_context); >> - bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err); >> + ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp); >> aio_context_release(aio_context); >> >> - if (local_err != NULL) { >> - error_propagate(errp, local_err); >> + if (ret < 0) { >> return NULL; >> } >> } >>
diff --git a/block/qcow2.h b/block/qcow2.h index 998bcdaef1..99ee88f802 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, uint32_t granularity, Error **errp); -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, - const char *name, - Error **errp); +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, + Error **errp); ssize_t coroutine_fn qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size, diff --git a/include/block/block_int.h b/include/block/block_int.h index 0422acdf1c..503ac9e3cd 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -556,9 +556,9 @@ struct BlockDriver { const char *name, uint32_t granularity, Error **errp); - void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, - const char *name, - Error **errp); + int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, + const char *name, + Error **errp); /** * Register/unregister a buffer for I/O. For example, when the driver is diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 4b4b731b46..07503b03b5 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, Error **errp); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, - const char *name, - Error **errp); +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, + Error **errp); void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap); void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap); void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap); diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 8f42015db9..a52b83b619 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) * not fail. * This function doesn't release corresponding BdrvDirtyBitmap. */ -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, - const char *name, - Error **errp) +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, + Error **errp) { if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) { - bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp); + return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp); } + + return -ENOTSUP; } bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index b2487101ed..1aaedb3b55 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1404,11 +1404,10 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, return NULL; } -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, - const char *name, - Error **errp) +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, + Error **errp) { - int ret; + int ret = 0; BDRVQcow2State *s = bs->opaque; Qcow2Bitmap *bm; Qcow2BitmapList *bm_list; @@ -1416,18 +1415,19 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, if (s->nb_bitmaps == 0) { /* Absence of the bitmap is not an error: see explanation above * bdrv_remove_persistent_dirty_bitmap() definition. */ - return; + return 0; } bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, s->bitmap_directory_size, errp); if (bm_list == NULL) { - return; + return -EIO; } bm = find_bitmap_by_name(bm_list, name); if (bm == NULL) { - goto fail; + ret = -EINVAL; + goto out; } QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry); @@ -1435,14 +1435,16 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, ret = update_ext_header_and_dir(bs, bm_list); if (ret < 0) { error_setg_errno(errp, -ret, "Failed to update bitmap extension"); - goto fail; + goto out; } free_bitmap_clusters(bs, &bm->table); -fail: +out: bitmap_free(bm); bitmap_list_free(bm_list); + + return ret; } void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) diff --git a/blockdev.c b/blockdev.c index fbef6845c8..0813adfb2b 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2940,15 +2940,14 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( } if (bdrv_dirty_bitmap_get_persistence(bitmap)) { + int ret; AioContext *aio_context = bdrv_get_aio_context(bs); - Error *local_err = NULL; aio_context_acquire(aio_context); - bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err); + ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp); aio_context_release(aio_context); - if (local_err != NULL) { - error_propagate(errp, local_err); + if (ret < 0) { return NULL; } }
It's more comfortable to not deal with local_err. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2.h | 5 ++--- include/block/block_int.h | 6 +++--- include/block/dirty-bitmap.h | 5 ++--- block/dirty-bitmap.c | 9 +++++---- block/qcow2-bitmap.c | 20 +++++++++++--------- blockdev.c | 7 +++---- 6 files changed, 26 insertions(+), 26 deletions(-)