Message ID | 20201127144522.29991-10-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | block: update graph permissions update | expand |
On Fri 27 Nov 2020 03:44:55 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Functions with errp argument are not recommened to be void-functions. > Improve bdrv_replace_node(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > Functions with errp argument are not recommened to be void-functions. > Improve bdrv_replace_node(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/block.h | 4 ++-- > block.c | 14 ++++++++------ > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 5d59984ad4..8f6100dad7 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -346,8 +346,8 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp); > BlockDriverState *bdrv_new(void); > int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, > Error **errp); > -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, > - Error **errp); > +int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, > + Error **errp); > > int bdrv_parse_aio(const char *mode, int *flags); > int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough); > diff --git a/block.c b/block.c > index 3765c7caed..29082c6d47 100644 > --- a/block.c > +++ b/block.c > @@ -4537,14 +4537,14 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) > * With auto_skip=false the error is returned if from has a parent which should > * not be updated. > */ > -static void bdrv_replace_node_common(BlockDriverState *from, > - BlockDriverState *to, > - bool auto_skip, Error **errp) > +static int bdrv_replace_node_common(BlockDriverState *from, > + BlockDriverState *to, > + bool auto_skip, Error **errp) > { > + int ret = -EPERM; > BdrvChild *c, *next; > GSList *list = NULL, *p; > uint64_t perm = 0, shared = BLK_PERM_ALL; > - int ret; I think I'd prefer setting ret in each error path. This makes it more obvious that ret has the right value and hasn't been modified between the initialisation and the error. > > /* Make sure that @from doesn't go away until we have successfully attached > * all of its parents to @to. */ > @@ -4600,10 +4600,12 @@ out: Let's add an explicit ret = 0 right before the out: label. > g_slist_free(list); > bdrv_drained_end(from); > bdrv_unref(from); > + > + return ret; > } With these changes: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
diff --git a/include/block/block.h b/include/block/block.h index 5d59984ad4..8f6100dad7 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -346,8 +346,8 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp); BlockDriverState *bdrv_new(void); int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, Error **errp); -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, - Error **errp); +int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, + Error **errp); int bdrv_parse_aio(const char *mode, int *flags); int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough); diff --git a/block.c b/block.c index 3765c7caed..29082c6d47 100644 --- a/block.c +++ b/block.c @@ -4537,14 +4537,14 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) * With auto_skip=false the error is returned if from has a parent which should * not be updated. */ -static void bdrv_replace_node_common(BlockDriverState *from, - BlockDriverState *to, - bool auto_skip, Error **errp) +static int bdrv_replace_node_common(BlockDriverState *from, + BlockDriverState *to, + bool auto_skip, Error **errp) { + int ret = -EPERM; BdrvChild *c, *next; GSList *list = NULL, *p; uint64_t perm = 0, shared = BLK_PERM_ALL; - int ret; /* Make sure that @from doesn't go away until we have successfully attached * all of its parents to @to. */ @@ -4600,10 +4600,12 @@ out: g_slist_free(list); bdrv_drained_end(from); bdrv_unref(from); + + return ret; } -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, - Error **errp) +int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, + Error **errp) { return bdrv_replace_node_common(from, to, true, errp); }
Functions with errp argument are not recommened to be void-functions. Improve bdrv_replace_node(). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block.h | 4 ++-- block.c | 14 ++++++++------ 2 files changed, 10 insertions(+), 8 deletions(-)