Message ID | 20200703090816.3295-8-vsementsov@virtuozzo.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | error: auto propagated local_err part I | expand |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > If we want to add some info to errp (by error_prepend() or > error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro. > Otherwise, this info will not be added when errp == &error_fatal > (the program will exit prior to the error_append_hint() or > error_prepend() call). Fix such cases. > > If we want to check error after errp-function call, we need to > introduce local_err and then propagate it to errp. Instead, use > ERRP_AUTO_PROPAGATE macro, benefits are: > 1. No need of explicit error_propagate call > 2. No need of explicit local_err variable: use errp directly > 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or > &error_fatal, this means that we don't break error_abort > (we'll abort on error_set, not on error_propagate) > > This commit is generated by command > > sed -n '/^Network Block Device (NBD)$/,/^$/{s/^F: //p}' \ > MAINTAINERS | \ > xargs git ls-files | grep '\.[hc]$' | \ > xargs spatch \ > --sp-file scripts/coccinelle/auto-propagated-errp.cocci \ > --macro-file scripts/cocci-macro-file.h \ > --in-place --no-show-diff --max-width 80 > > Reported-by: Kevin Wolf <kwolf@redhat.com> > Reported-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > If we want to add some info to errp (by error_prepend() or > error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro. > Otherwise, this info will not be added when errp == &error_fatal Same for &error_abort, but to fix that, ERRP_AUTO_PROPAGATE() would have to compromise stack backtraces, so it doesn't. > (the program will exit prior to the error_append_hint() or > error_prepend() call). Fix such cases. Unclear what exactly is being fixed here, if anything. What are the cases? Can they happen? The following look like possible cases: nbd_read(), nbd_send_option_request(), nbd_handle_reply_err(), nbd_opt_info_or_go(), nbd_start_negotiate(), nbd_receive_negotiate(), nbd_negotiate_send_rep_verr(), nbd_negotiate_send_rep_list(), nbd_negotiate_handle_export_name(), nbd_negotiate(), nbd_export_remove(). Additionally in PATCH 6, v9fs_device_realize_common(). Nothing in PATCH 3+4+5+8 as far as I can tell. If we lack the time to provide a complete assessment in the commit messages, let's say so there. > If we want to check error after errp-function call, we need to > introduce local_err and then propagate it to errp. Instead, use > ERRP_AUTO_PROPAGATE macro, benefits are: > 1. No need of explicit error_propagate call > 2. No need of explicit local_err variable: use errp directly > 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or > &error_fatal, this means that we don't break error_abort > (we'll abort on error_set, not on error_propagate) Direct use of @errp improves behavior when the argument is &error_abort. Worth mentioning, I think. > > This commit is generated by command > > sed -n '/^Network Block Device (NBD)$/,/^$/{s/^F: //p}' \ > MAINTAINERS | \ > xargs git ls-files | grep '\.[hc]$' | \ > xargs spatch \ > --sp-file scripts/coccinelle/auto-propagated-errp.cocci \ > --macro-file scripts/cocci-macro-file.h \ > --in-place --no-show-diff --max-width 80 > > Reported-by: Kevin Wolf <kwolf@redhat.com> > Reported-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/include/block/nbd.h b/include/block/nbd.h index 20363280ae..f7d87636d3 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -361,6 +361,7 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp); static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size, const char *desc, Error **errp) { + ERRP_AUTO_PROPAGATE(); int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0; if (ret < 0) { diff --git a/block/nbd.c b/block/nbd.c index 6876da04a7..b7cea0f650 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -1408,16 +1408,15 @@ static void nbd_client_close(BlockDriverState *bs) static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr, Error **errp) { + ERRP_AUTO_PROPAGATE(); QIOChannelSocket *sioc; - Error *local_err = NULL; sioc = qio_channel_socket_new(); qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client"); - qio_channel_socket_connect_sync(sioc, saddr, &local_err); - if (local_err) { + qio_channel_socket_connect_sync(sioc, saddr, errp); + if (*errp) { object_unref(OBJECT(sioc)); - error_propagate(errp, local_err); return NULL; } diff --git a/nbd/client.c b/nbd/client.c index ba173108ba..e258ef3f7e 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -68,6 +68,7 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt, uint32_t len, const char *data, Error **errp) { + ERRP_AUTO_PROPAGATE(); NBDOption req; QEMU_BUILD_BUG_ON(sizeof(req) != 16); @@ -153,6 +154,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt, static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply, bool strict, Error **errp) { + ERRP_AUTO_PROPAGATE(); g_autofree char *msg = NULL; if (!(reply->type & (1 << 31))) { @@ -337,6 +339,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt, NBDExportInfo *info, Error **errp) { + ERRP_AUTO_PROPAGATE(); NBDOptionReply reply; uint32_t len = strlen(info->name); uint16_t type; @@ -882,6 +885,7 @@ static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc, bool structured_reply, bool *zeroes, Error **errp) { + ERRP_AUTO_PROPAGATE(); uint64_t magic; trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>"); @@ -1017,6 +1021,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc, const char *hostname, QIOChannel **outioc, NBDExportInfo *info, Error **errp) { + ERRP_AUTO_PROPAGATE(); int result; bool zeroes; bool base_allocation = info->base_allocation; diff --git a/nbd/server.c b/nbd/server.c index 20754e9ebc..8a12e586d7 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -211,6 +211,7 @@ static int GCC_FMT_ATTR(4, 0) nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type, Error **errp, const char *fmt, va_list va) { + ERRP_AUTO_PROPAGATE(); g_autofree char *msg = NULL; int ret; size_t len; @@ -382,6 +383,7 @@ static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length, static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp, Error **errp) { + ERRP_AUTO_PROPAGATE(); size_t name_len, desc_len; uint32_t len; const char *name = exp->name ? exp->name : ""; @@ -445,6 +447,7 @@ static void nbd_check_meta_export(NBDClient *client) static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, Error **errp) { + ERRP_AUTO_PROPAGATE(); g_autofree char *name = NULL; char buf[NBD_REPLY_EXPORT_NAME_SIZE] = ""; size_t len; @@ -1289,6 +1292,7 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp) */ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp) { + ERRP_AUTO_PROPAGATE(); char buf[NBD_OLDSTYLE_NEGOTIATE_SIZE] = ""; int ret; @@ -1663,6 +1667,7 @@ void nbd_export_close(NBDExport *exp) void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp) { + ERRP_AUTO_PROPAGATE(); if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) { nbd_export_close(exp); return;
If we want to add some info to errp (by error_prepend() or error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro. Otherwise, this info will not be added when errp == &error_fatal (the program will exit prior to the error_append_hint() or error_prepend() call). Fix such cases. If we want to check error after errp-function call, we need to introduce local_err and then propagate it to errp. Instead, use ERRP_AUTO_PROPAGATE macro, benefits are: 1. No need of explicit error_propagate call 2. No need of explicit local_err variable: use errp directly 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or &error_fatal, this means that we don't break error_abort (we'll abort on error_set, not on error_propagate) This commit is generated by command sed -n '/^Network Block Device (NBD)$/,/^$/{s/^F: //p}' \ MAINTAINERS | \ xargs git ls-files | grep '\.[hc]$' | \ xargs spatch \ --sp-file scripts/coccinelle/auto-propagated-errp.cocci \ --macro-file scripts/cocci-macro-file.h \ --in-place --no-show-diff --max-width 80 Reported-by: Kevin Wolf <kwolf@redhat.com> Reported-by: Greg Kurz <groug@kaod.org> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/nbd.h | 1 + block/nbd.c | 7 +++---- nbd/client.c | 5 +++++ nbd/server.c | 5 +++++ 4 files changed, 14 insertions(+), 4 deletions(-)