Message ID | 53A84797.9040304@gmail.com |
---|---|
State | New |
Headers | show |
On Mon, 06/23 23:28, Chen Gang wrote: > When failure occurs, 'ret' need be set, or may return 0 to indicate success. s/need/needs/ > And error_propagate() also need be called only one time within a function. s/need/needs/ > > It is abnormal to prevent bdrv_append_temp_snapshot() return value but still > set errp when error occurs -- although it contents return value internally. s/contents/has/ > > So let bdrv_append_temp_snapshot() internal return value outside, and let > all things normal, then fix the issue too. > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > block.c | 7 ++++--- > include/block/block.h | 2 +- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index 74af8d7..5e4fb60 100644 > --- a/block.c > +++ b/block.c > @@ -1240,7 +1240,7 @@ done: > return ret; > } > > -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) > +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) > { > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ > char *tmp_filename = g_malloc0(PATH_MAX + 1); > @@ -1258,6 +1258,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) > /* Get the required size from the image */ > total_size = bdrv_getlength(bs); > if (total_size < 0) { > + ret = total_size; > error_setg_errno(errp, -total_size, "Could not get image size"); > goto out; > } > @@ -1304,6 +1305,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) > > out: > g_free(tmp_filename); > + return ret; > } > > static QDict *parse_json_filename(const char *filename, Error **errp) > @@ -1495,9 +1497,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > /* For snapshot=on, create a temporary qcow2 overlay. bs points to the > * temporary snapshot afterwards. */ > if (snapshot_flags) { > - bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); > + ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); > if (local_err) { > - error_propagate(errp, local_err); > goto close_and_fail; > } > } > diff --git a/include/block/block.h b/include/block/block.h > index f15b99b..7b3381c 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -217,7 +217,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, > bool allow_none, Error **errp); > void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd); > int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); > -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); > +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); Not used outside block.c, but pre-existing. Reviewed-by: Fam Zheng <famz@redhat.com> Fam > int bdrv_open(BlockDriverState **pbs, const char *filename, > const char *reference, QDict *options, int flags, > BlockDriver *drv, Error **errp); > -- > 1.7.11.7
On 06/24/2014 10:25 AM, Fam Zheng wrote: > On Mon, 06/23 23:28, Chen Gang wrote: >> When failure occurs, 'ret' need be set, or may return 0 to indicate success. > > s/need/needs/ > >> And error_propagate() also need be called only one time within a function. > > s/need/needs/ > For a grammar in my learning, in this situation, 'need' is not a verb, "be set" and "be called" are the verb used with passive mode. >> >> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still >> set errp when error occurs -- although it contents return value internally. > > s/contents/has/ > OK, thanks, and excuse me for my poor English. If necessary I shall send patch v2 for it. >> >> So let bdrv_append_temp_snapshot() internal return value outside, and let >> all things normal, then fix the issue too. >> >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> block.c | 7 ++++--- >> include/block/block.h | 2 +- >> 2 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/block.c b/block.c >> index 74af8d7..5e4fb60 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1240,7 +1240,7 @@ done: >> return ret; >> } >> >> -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) >> +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) >> { >> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ >> char *tmp_filename = g_malloc0(PATH_MAX + 1); >> @@ -1258,6 +1258,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) >> /* Get the required size from the image */ >> total_size = bdrv_getlength(bs); >> if (total_size < 0) { >> + ret = total_size; >> error_setg_errno(errp, -total_size, "Could not get image size"); >> goto out; >> } >> @@ -1304,6 +1305,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) >> >> out: >> g_free(tmp_filename); >> + return ret; >> } >> >> static QDict *parse_json_filename(const char *filename, Error **errp) >> @@ -1495,9 +1497,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, >> /* For snapshot=on, create a temporary qcow2 overlay. bs points to the >> * temporary snapshot afterwards. */ >> if (snapshot_flags) { >> - bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); >> + ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); >> if (local_err) { >> - error_propagate(errp, local_err); >> goto close_and_fail; >> } >> } >> diff --git a/include/block/block.h b/include/block/block.h >> index f15b99b..7b3381c 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -217,7 +217,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, >> bool allow_none, Error **errp); >> void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd); >> int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); >> -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); >> +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); > > Not used outside block.c, but pre-existing. > Yeah, originally, I considered about it, and finally still remained it as pre-existing. > Reviewed-by: Fam Zheng <famz@redhat.com> > Thank you for your work. Thanks.
Am 23.06.2014 um 17:28 hat Chen Gang geschrieben: > When failure occurs, 'ret' need be set, or may return 0 to indicate success. > And error_propagate() also need be called only one time within a function. > > It is abnormal to prevent bdrv_append_temp_snapshot() return value but still > set errp when error occurs -- although it contents return value internally. > > So let bdrv_append_temp_snapshot() internal return value outside, and let > all things normal, then fix the issue too. > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> What does this fix? Having both a return value and an Error* object is duplication and only a sign that a function hasn't been fully converted to the Error framework yet. We shouldn't introduce new instances of this without a very good reason. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 23.06.2014 um 17:28 hat Chen Gang geschrieben: >> When failure occurs, 'ret' need be set, or may return 0 to indicate success. >> And error_propagate() also need be called only one time within a function. >> >> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still >> set errp when error occurs -- although it contents return value internally. >> >> So let bdrv_append_temp_snapshot() internal return value outside, and let >> all things normal, then fix the issue too. >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > > What does this fix? It fixes the return value of bdrv_open() when bdrv_append_temp_snapshot() fails. Before this patch, it returns a positive value, which is wrong. After the patch, it returns the negative error code bdrv_append_temp_snapshot() now returns. > Having both a return value and an Error* object is duplication and > only a sign that a function hasn't been fully converted to the Error > framework yet. We shouldn't introduce new instances of this without a > very good reason. Maybe. But I very much prefer ret = foo(arg, errp); if (ret < 0) { return ret; } over Error *local_err = NULL; foo(arg, &local_err); if (local_err) { error_propagate(errp, local_err); return; }
24.06.2014 15:01, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 23.06.2014 um 17:28 hat Chen Gang geschrieben: >>> When failure occurs, 'ret' need be set, or may return 0 to indicate success. >>> And error_propagate() also need be called only one time within a function. >>> >>> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still >>> set errp when error occurs -- although it contents return value internally. >>> >>> So let bdrv_append_temp_snapshot() internal return value outside, and let >>> all things normal, then fix the issue too. >>> >>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> >> What does this fix? > > It fixes the return value of bdrv_open() when > bdrv_append_temp_snapshot() fails. Before this patch, it returns a > positive value, which is wrong. After the patch, it returns the > negative error code bdrv_append_temp_snapshot() now returns. So, what should be done there? Kevin, maybe you should pick this up instead of going -trivial route? >> Having both a return value and an Error* object is duplication and >> only a sign that a function hasn't been fully converted to the Error >> framework yet. We shouldn't introduce new instances of this without a >> very good reason. > > Maybe. But I very much prefer > > ret = foo(arg, errp); > if (ret < 0) { > return ret; > } > > over > > Error *local_err = NULL; > > foo(arg, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > } Yes, this new error propagation is a bit ugly, I dislike it too. Thanks, /mjt
Thank you for all of your work, if necessary to send patch v3 for it (may change the comments), please let me know. Thanks. On 06/25/2014 12:00 AM, Michael Tokarev wrote: > 24.06.2014 15:01, Markus Armbruster wrote: >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Am 23.06.2014 um 17:28 hat Chen Gang geschrieben: >>>> When failure occurs, 'ret' need be set, or may return 0 to indicate success. >>>> And error_propagate() also need be called only one time within a function. >>>> >>>> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still >>>> set errp when error occurs -- although it contents return value internally. >>>> >>>> So let bdrv_append_temp_snapshot() internal return value outside, and let >>>> all things normal, then fix the issue too. >>>> >>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >>> >>> What does this fix? >> >> It fixes the return value of bdrv_open() when >> bdrv_append_temp_snapshot() fails. Before this patch, it returns a >> positive value, which is wrong. After the patch, it returns the >> negative error code bdrv_append_temp_snapshot() now returns. > > So, what should be done there? Kevin, maybe you should pick this up > instead of going -trivial route? > >>> Having both a return value and an Error* object is duplication and >>> only a sign that a function hasn't been fully converted to the Error >>> framework yet. We shouldn't introduce new instances of this without a >>> very good reason. >> >> Maybe. But I very much prefer >> >> ret = foo(arg, errp); >> if (ret < 0) { >> return ret; >> } >> >> over >> >> Error *local_err = NULL; >> >> foo(arg, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> return; >> } > > Yes, this new error propagation is a bit ugly, I dislike it too. > > Thanks, > > /mjt >
On Tue, Jun 24, 2014 at 01:01:52PM +0200, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 23.06.2014 um 17:28 hat Chen Gang geschrieben: > >> When failure occurs, 'ret' need be set, or may return 0 to indicate success. > >> And error_propagate() also need be called only one time within a function. > >> > >> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still > >> set errp when error occurs -- although it contents return value internally. > >> > >> So let bdrv_append_temp_snapshot() internal return value outside, and let > >> all things normal, then fix the issue too. > >> > >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > > > > What does this fix? > > It fixes the return value of bdrv_open() when > bdrv_append_temp_snapshot() fails. Before this patch, it returns a > positive value, which is wrong. After the patch, it returns the > negative error code bdrv_append_temp_snapshot() now returns. Exactly. I asked for the -errno return because otherwise bdrv_open() would have no accurate errno. Stefan
Am 23.06.2014 um 17:28 hat Chen Gang geschrieben: > When failure occurs, 'ret' need be set, or may return 0 to indicate success. > And error_propagate() also need be called only one time within a function. > > It is abnormal to prevent bdrv_append_temp_snapshot() return value but still > set errp when error occurs -- although it contents return value internally. > > So let bdrv_append_temp_snapshot() internal return value outside, and let > all things normal, then fix the issue too. > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> Thanks, applied to the block branch. Kevin
diff --git a/block.c b/block.c index 74af8d7..5e4fb60 100644 --- a/block.c +++ b/block.c @@ -1240,7 +1240,7 @@ done: return ret; } -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) { /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ char *tmp_filename = g_malloc0(PATH_MAX + 1); @@ -1258,6 +1258,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) /* Get the required size from the image */ total_size = bdrv_getlength(bs); if (total_size < 0) { + ret = total_size; error_setg_errno(errp, -total_size, "Could not get image size"); goto out; } @@ -1304,6 +1305,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) out: g_free(tmp_filename); + return ret; } static QDict *parse_json_filename(const char *filename, Error **errp) @@ -1495,9 +1497,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, /* For snapshot=on, create a temporary qcow2 overlay. bs points to the * temporary snapshot afterwards. */ if (snapshot_flags) { - bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); + ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); if (local_err) { - error_propagate(errp, local_err); goto close_and_fail; } } diff --git a/include/block/block.h b/include/block/block.h index f15b99b..7b3381c 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -217,7 +217,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, bool allow_none, Error **errp); void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd); int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); int bdrv_open(BlockDriverState **pbs, const char *filename, const char *reference, QDict *options, int flags, BlockDriver *drv, Error **errp);
When failure occurs, 'ret' need be set, or may return 0 to indicate success. And error_propagate() also need be called only one time within a function. It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally. So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- block.c | 7 ++++--- include/block/block.h | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)