| Message ID | 20200917195519.19589-13-vsementsov@virtuozzo.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block: deal with errp: part I | expand |
On Thu 17 Sep 2020 09:55:18 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > qcow2_do_open correctly sets errp on each failure path. So, we can > simplify code in qcow2_co_invalidate_cache() and drop explicit error > propagation. We should use ERRP_GUARD() (accordingly to comment in > include/qapi/error.h) together with error_append() call which we add to > avoid problems with error_fatal. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On Thu, 17 Sep 2020 22:55:18 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > qcow2_do_open correctly sets errp on each failure path. So, we can > simplify code in qcow2_co_invalidate_cache() and drop explicit error > propagation. We should use ERRP_GUARD() (accordingly to comment in > include/qapi/error.h) together with error_append() call which we add to > avoid problems with error_fatal. > The wording gives the impression that we add error_append() to avoid problems with error_fatal which is certainly not true. Also it isn't _append() but _prepend() :) What about ? "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h to avoid problems with the error_prepend() call if errp is &error_fatal." With that fixed, Reviewed-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 2b6ec4b757..cd5f48d3fb 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2702,11 +2702,11 @@ static void qcow2_close(BlockDriverState *bs) > static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, > Error **errp) > { > + ERRP_GUARD(); > BDRVQcow2State *s = bs->opaque; > int flags = s->flags; > QCryptoBlock *crypto = NULL; > QDict *options; > - Error *local_err = NULL; > int ret; > > /* > @@ -2724,16 +2724,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, > > flags &= ~BDRV_O_INACTIVE; > qemu_co_mutex_lock(&s->lock); > - ret = qcow2_do_open(bs, options, flags, &local_err); > + ret = qcow2_do_open(bs, options, flags, errp); > qemu_co_mutex_unlock(&s->lock); > qobject_unref(options); > - if (local_err) { > - error_propagate_prepend(errp, local_err, > - "Could not reopen qcow2 layer: "); > - bs->drv = NULL; > - return; > - } else if (ret < 0) { > - error_setg_errno(errp, -ret, "Could not reopen qcow2 layer"); > + if (ret < 0) { > + error_prepend(errp, "Could not reopen qcow2 layer: "); > bs->drv = NULL; > return; > }
On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote: >> qcow2_do_open correctly sets errp on each failure path. So, we can >> simplify code in qcow2_co_invalidate_cache() and drop explicit error >> propagation. We should use ERRP_GUARD() (accordingly to comment in >> include/qapi/error.h) together with error_append() call which we add to >> avoid problems with error_fatal. >> > > The wording gives the impression that we add error_append() to avoid problems > with error_fatal which is certainly not true. Also it isn't _append() but > _prepend() :) > > What about ? > > "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h > to avoid problems with the error_prepend() call if errp is > &error_fatal." I had to go to the individual error functions to see what "it doesn't work with &error_fatal" actually means. So in a case like qcow2_do_open() which has: error_setg(errp, ...) error_append_hint(errp, ...) As far as I can see this works just fine without ERRP_GUARD() and with error_fatal, the difference is that if we don't use the guard then the process exists during error_setg(), and if we use the guard it exists during the implicit error_propagate() call triggered by its destruction at the end of the function. In this latter case the printed error message would include the hint. Berto
18.09.2020 18:51, Alberto Garcia wrote: > On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote: >>> qcow2_do_open correctly sets errp on each failure path. So, we can >>> simplify code in qcow2_co_invalidate_cache() and drop explicit error >>> propagation. We should use ERRP_GUARD() (accordingly to comment in >>> include/qapi/error.h) together with error_append() call which we add to >>> avoid problems with error_fatal. >>> >> >> The wording gives the impression that we add error_append() to avoid problems >> with error_fatal which is certainly not true. Also it isn't _append() but >> _prepend() :) >> >> What about ? >> >> "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h >> to avoid problems with the error_prepend() call if errp is >> &error_fatal." OK for me. > > I had to go to the individual error functions to see what "it doesn't > work with &error_fatal" actually means. > > So in a case like qcow2_do_open() which has: > > error_setg(errp, ...) > error_append_hint(errp, ...) > > As far as I can see this works just fine without ERRP_GUARD() and with > error_fatal, the difference is that if we don't use the guard then the > process exists during error_setg(), and if we use the guard it exists > during the implicit error_propagate() call triggered by its destruction > at the end of the function. In this latter case the printed error > message would include the hint. > Yes the only problem is that without ERRP_GUARD we lose the hint in case of error_fatal. -- Best regards, Vladimir
On Fri, 18 Sep 2020 19:01:34 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 18.09.2020 18:51, Alberto Garcia wrote: > > On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote: > >>> qcow2_do_open correctly sets errp on each failure path. So, we can > >>> simplify code in qcow2_co_invalidate_cache() and drop explicit error > >>> propagation. We should use ERRP_GUARD() (accordingly to comment in > >>> include/qapi/error.h) together with error_append() call which we add to > >>> avoid problems with error_fatal. > >>> > >> > >> The wording gives the impression that we add error_append() to avoid problems > >> with error_fatal which is certainly not true. Also it isn't _append() but > >> _prepend() :) > >> > >> What about ? > >> > >> "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h > >> to avoid problems with the error_prepend() call if errp is > >> &error_fatal." > > OK for me. > > > > > I had to go to the individual error functions to see what "it doesn't > > work with &error_fatal" actually means. > > > > So in a case like qcow2_do_open() which has: > > > > error_setg(errp, ...) > > error_append_hint(errp, ...) > > > > As far as I can see this works just fine without ERRP_GUARD() and with > > error_fatal, the difference is that if we don't use the guard then the > > process exists during error_setg(), and if we use the guard it exists > > during the implicit error_propagate() call triggered by its destruction > > at the end of the function. In this latter case the printed error > > message would include the hint. > > > > Yes the only problem is that without ERRP_GUARD we lose the hint in case of error_fatal. > Yeah, so rather: "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h so that error_prepend() is actually called even if errp is &error_fatal." Cheers, -- Greg
diff --git a/block/qcow2.c b/block/qcow2.c index 2b6ec4b757..cd5f48d3fb 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2702,11 +2702,11 @@ static void qcow2_close(BlockDriverState *bs) static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, Error **errp) { + ERRP_GUARD(); BDRVQcow2State *s = bs->opaque; int flags = s->flags; QCryptoBlock *crypto = NULL; QDict *options; - Error *local_err = NULL; int ret; /* @@ -2724,16 +2724,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, flags &= ~BDRV_O_INACTIVE; qemu_co_mutex_lock(&s->lock); - ret = qcow2_do_open(bs, options, flags, &local_err); + ret = qcow2_do_open(bs, options, flags, errp); qemu_co_mutex_unlock(&s->lock); qobject_unref(options); - if (local_err) { - error_propagate_prepend(errp, local_err, - "Could not reopen qcow2 layer: "); - bs->drv = NULL; - return; - } else if (ret < 0) { - error_setg_errno(errp, -ret, "Could not reopen qcow2 layer"); + if (ret < 0) { + error_prepend(errp, "Could not reopen qcow2 layer: "); bs->drv = NULL; return; }
qcow2_do_open correctly sets errp on each failure path. So, we can simplify code in qcow2_co_invalidate_cache() and drop explicit error propagation. We should use ERRP_GUARD() (accordingly to comment in include/qapi/error.h) together with error_append() call which we add to avoid problems with error_fatal. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)