From 47ff5ac81e8bb3096500de7b132051691d533d36 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 7 Jul 2020 18:05:29 +0200 Subject: error: Fix examples in error.h's big comment Mark a bad example more clearly. Fix the error_propagate_prepend() example. Add a missing declaration and a second error pileup example. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Message-Id: <20200707160613.848843-2-armbru@redhat.com> --- include/qapi/error.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'include/qapi/error.h') diff --git a/include/qapi/error.h b/include/qapi/error.h index ad5b6e896d..e8960eaad5 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -24,7 +24,7 @@ * "charm, top, bottom.\n"); * * Do *not* contract this to - * error_setg(&err, "invalid quark\n" + * error_setg(&err, "invalid quark\n" // WRONG! * "Valid quarks are up, down, strange, charm, top, bottom."); * * Report an error to the current monitor if we have one, else stderr: @@ -52,7 +52,8 @@ * where Error **errp is a parameter, by convention the last one. * * Pass an existing error to the caller with the message modified: - * error_propagate_prepend(errp, err); + * error_propagate_prepend(errp, err, + * "Could not frobnicate '%s': ", name); * * Avoid * error_propagate(errp, err); @@ -108,12 +109,23 @@ * } * * Do *not* "optimize" this to + * Error *err = NULL; * foo(arg, &err); * bar(arg, &err); // WRONG! * if (err) { * handle the error... * } * because this may pass a non-null err to bar(). + * + * Likewise, do *not* + * Error *err = NULL; + * if (cond1) { + * error_setg(&err, ...); + * } + * if (cond2) { + * error_setg(&err, ...); // WRONG! + * } + * because this may pass a non-null err to error_setg(). */ #ifndef ERROR_H -- cgit 1.4.1 From 9aac7d486cc792191c25c30851f501624b0c2751 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 7 Jul 2020 18:05:30 +0200 Subject: error: Improve error.h's big comment Add headlines to the big comment. Explain examples for NULL, &error_abort and &error_fatal argument better. Tweak rationale for error_propagate_prepend(). Signed-off-by: Markus Armbruster Message-Id: <20200707160613.848843-3-armbru@redhat.com> Reviewed-by: Eric Blake Reviewed-by: Greg Kurz --- include/qapi/error.h | 51 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 15 deletions(-) (limited to 'include/qapi/error.h') diff --git a/include/qapi/error.h b/include/qapi/error.h index e8960eaad5..6d079c58b7 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -15,6 +15,8 @@ /* * Error reporting system loosely patterned after Glib's GError. * + * = Creating errors = + * * Create an error: * error_setg(&err, "situation normal, all fouled up"); * @@ -27,6 +29,8 @@ * error_setg(&err, "invalid quark\n" // WRONG! * "Valid quarks are up, down, strange, charm, top, bottom."); * + * = Reporting and destroying errors = + * * Report an error to the current monitor if we have one, else stderr: * error_report_err(err); * This frees the error object. @@ -40,6 +44,30 @@ * error_free(err); * Note that this loses hints added with error_append_hint(). * + * Call a function ignoring errors: + * foo(arg, NULL); + * This is more concise than + * Error *err = NULL; + * foo(arg, &err); + * error_free(err); // don't do this + * + * Call a function aborting on errors: + * foo(arg, &error_abort); + * This is more concise and fails more nicely than + * Error *err = NULL; + * foo(arg, &err); + * assert(!err); // don't do this + * + * Call a function treating errors as fatal: + * foo(arg, &error_fatal); + * This is more concise than + * Error *err = NULL; + * foo(arg, &err); + * if (err) { // don't do this + * error_report_err(err); + * exit(1); + * } + * * Handle an error without reporting it (just for completeness): * error_free(err); * @@ -47,6 +75,11 @@ * reporting it (primarily useful in testsuites): * error_free_or_abort(&err); * + * = Passing errors around = + * + * Errors get passed to the caller through the conventional @errp + * parameter. + * * Pass an existing error to the caller: * error_propagate(errp, err); * where Error **errp is a parameter, by convention the last one. @@ -54,11 +87,10 @@ * Pass an existing error to the caller with the message modified: * error_propagate_prepend(errp, err, * "Could not frobnicate '%s': ", name); - * - * Avoid - * error_propagate(errp, err); + * This is more concise than + * error_propagate(errp, err); // don't do this * error_prepend(errp, "Could not frobnicate '%s': ", name); - * because this fails to prepend when @errp is &error_fatal. + * and works even when @errp is &error_fatal. * * Create a new error and pass it to the caller: * error_setg(errp, "situation normal, all fouled up"); @@ -70,15 +102,6 @@ * handle the error... * } * - * Call a function ignoring errors: - * foo(arg, NULL); - * - * Call a function aborting on errors: - * foo(arg, &error_abort); - * - * Call a function treating errors as fatal: - * foo(arg, &error_fatal); - * * Receive an error and pass it on to the caller: * Error *err = NULL; * foo(arg, &err); @@ -86,8 +109,6 @@ * handle the error... * error_propagate(errp, err); * } - * where Error **errp is a parameter, by convention the last one. - * * Do *not* "optimize" this to * foo(arg, errp); * if (*errp) { // WRONG! -- cgit 1.4.1 From e3fe3988d7851cac30abffae06d2f555ff7bee62 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 7 Jul 2020 18:05:31 +0200 Subject: error: Document Error API usage rules This merely codifies existing practice, with one exception: the rule advising against returning void, where existing practice is mixed. When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which recommends to return true on success and false on error then. When a function returns a distinct error value, say false, a checked call that passes the error up looks like if (!frobnicate(..., errp)) { handle the error... } When it returns void, we need Error *err = NULL; frobnicate(..., &err); if (err) { handle the error... error_propagate(errp, err); } Not only is this more verbose, it also creates an Error object even when @errp is null, &error_abort or &error_fatal. People got tired of the additional boilerplate, and started to ignore the unwritten rule. The result is confusion among developers about the preferred usage. Make the rule advising against returning void official by putting it in writing. This will hopefully reduce confusion. Update the examples accordingly. The remainder of this series will update a substantial amount of code to honor the rule. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Message-Id: <20200707160613.848843-4-armbru@redhat.com> [Tweak prose as per advice from Eric] --- include/qapi/error.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 6 deletions(-) (limited to 'include/qapi/error.h') diff --git a/include/qapi/error.h b/include/qapi/error.h index 6d079c58b7..2c189abb04 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -15,6 +15,33 @@ /* * Error reporting system loosely patterned after Glib's GError. * + * = Rules = + * + * - Functions that use Error to report errors have an Error **errp + * parameter. It should be the last parameter, except for functions + * taking variable arguments. + * + * - You may pass NULL to not receive the error, &error_abort to abort + * on error, &error_fatal to exit(1) on error, or a pointer to a + * variable containing NULL to receive the error. + * + * - Separation of concerns: the function is responsible for detecting + * errors and failing cleanly; handling the error is its caller's + * job. Since the value of @errp is about handling the error, the + * function should not examine it. + * + * - On success, the function should not touch *errp. On failure, it + * should set a new error, e.g. with error_setg(errp, ...), or + * propagate an existing one, e.g. with error_propagate(errp, ...). + * + * - Whenever practical, also return a value that indicates success / + * failure. This can make the error checking more concise, and can + * avoid useless error object creation and destruction. Note that + * we still have many functions returning void. We recommend + * • bool-valued functions return true on success / false on failure, + * • pointer-valued functions return non-null / null pointer, and + * • integer-valued functions return non-negative / negative. + * * = Creating errors = * * Create an error: @@ -95,14 +122,13 @@ * Create a new error and pass it to the caller: * error_setg(errp, "situation normal, all fouled up"); * - * Call a function and receive an error from it: - * Error *err = NULL; - * foo(arg, &err); - * if (err) { + * Call a function, receive an error from it, and pass it to the caller + * - when the function returns a value that indicates failure, say + * false: + * if (!foo(arg, errp)) { * handle the error... * } - * - * Receive an error and pass it on to the caller: + * - when it does not, say because it is a void function: * Error *err = NULL; * foo(arg, &err); * if (err) { @@ -120,6 +146,20 @@ * foo(arg, errp); * for readability. * + * Receive an error, and handle it locally + * - when the function returns a value that indicates failure, say + * false: + * Error *err = NULL; + * if (!foo(arg, &err)) { + * handle the error... + * } + * - when it does not, say because it is a void function: + * Error *err = NULL; + * foo(arg, &err); + * if (err) { + * handle the error... + * } + * * Receive and accumulate multiple errors (first one wins): * Error *err = NULL, *local_err = NULL; * foo(arg, &err); -- cgit 1.4.1 From ae7c80a7bd73685437bf6ba9d7c26098351f4166 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 7 Jul 2020 18:50:30 +0200 Subject: error: New macro ERRP_GUARD() Introduce a new ERRP_GUARD() macro, to be used at start of functions with an errp OUT parameter. It has three goals: 1. Fix issue with error_fatal and error_prepend/error_append_hint: the user can't see this additional information, because exit() happens in error_setg earlier than information is added. [Reported by Greg Kurz] 2. Fix issue with error_abort and error_propagate: when we wrap error_abort by local_err+error_propagate, the resulting coredump will refer to error_propagate and not to the place where error happened. (the macro itself doesn't fix the issue, but it allows us to [3.] drop the local_err+error_propagate pattern, which will definitely fix the issue) [Reported by Kevin Wolf] 3. Drop local_err+error_propagate pattern, which is used to workaround void functions with errp parameter, when caller wants to know resulting status. (Note: actually these functions could be merely updated to return int error code). To achieve these goals, later patches will add invocations of this macro at the start of functions with either use error_prepend/error_append_hint (solving 1) or which use local_err+error_propagate to check errors, switching those functions to use *errp instead (solving 2 and 3). Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Paul Durrant Reviewed-by: Greg Kurz Reviewed-by: Eric Blake [Merge comments properly with recent commit "error: Document Error API usage rules", and edit for clarity. Put ERRP_AUTO_PROPAGATE() before its helpers, and touch up style. Tweak commit message.] Signed-off-by: Markus Armbruster Message-Id: <20200707165037.1026246-2-armbru@redhat.com> [Rename ERRP_AUTO_PROPAGATE() to ERRP_GUARD(), tweak commit message again] --- include/qapi/error.h | 158 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 139 insertions(+), 19 deletions(-) (limited to 'include/qapi/error.h') diff --git a/include/qapi/error.h b/include/qapi/error.h index 2c189abb04..85df875a3a 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -30,6 +30,10 @@ * job. Since the value of @errp is about handling the error, the * function should not examine it. * + * - The function may pass @errp to functions it calls to pass on + * their errors to its caller. If it dereferences @errp to check + * for errors, it must use ERRP_GUARD(). + * * - On success, the function should not touch *errp. On failure, it * should set a new error, e.g. with error_setg(errp, ...), or * propagate an existing one, e.g. with error_propagate(errp, ...). @@ -45,15 +49,17 @@ * = Creating errors = * * Create an error: - * error_setg(&err, "situation normal, all fouled up"); + * error_setg(errp, "situation normal, all fouled up"); + * where @errp points to the location to receive the error. * * Create an error and add additional explanation: - * error_setg(&err, "invalid quark"); - * error_append_hint(&err, "Valid quarks are up, down, strange, " + * error_setg(errp, "invalid quark"); + * error_append_hint(errp, "Valid quarks are up, down, strange, " * "charm, top, bottom.\n"); + * This may require use of ERRP_GUARD(); more on that below. * * Do *not* contract this to - * error_setg(&err, "invalid quark\n" // WRONG! + * error_setg(errp, "invalid quark\n" // WRONG! * "Valid quarks are up, down, strange, charm, top, bottom."); * * = Reporting and destroying errors = @@ -107,18 +113,6 @@ * Errors get passed to the caller through the conventional @errp * parameter. * - * Pass an existing error to the caller: - * error_propagate(errp, err); - * where Error **errp is a parameter, by convention the last one. - * - * Pass an existing error to the caller with the message modified: - * error_propagate_prepend(errp, err, - * "Could not frobnicate '%s': ", name); - * This is more concise than - * error_propagate(errp, err); // don't do this - * error_prepend(errp, "Could not frobnicate '%s': ", name); - * and works even when @errp is &error_fatal. - * * Create a new error and pass it to the caller: * error_setg(errp, "situation normal, all fouled up"); * @@ -129,18 +123,26 @@ * handle the error... * } * - when it does not, say because it is a void function: + * ERRP_GUARD(); + * foo(arg, errp); + * if (*errp) { + * handle the error... + * } + * More on ERRP_GUARD() below. + * + * Code predating ERRP_GUARD() still exists, and looks like this: * Error *err = NULL; * foo(arg, &err); * if (err) { * handle the error... - * error_propagate(errp, err); + * error_propagate(errp, err); // deprecated * } - * Do *not* "optimize" this to + * Avoid in new code. Do *not* "optimize" it to * foo(arg, errp); * if (*errp) { // WRONG! * handle the error... * } - * because errp may be NULL! + * because errp may be NULL without the ERRP_GUARD() guard. * * But when all you do with the error is pass it on, please use * foo(arg, errp); @@ -160,6 +162,19 @@ * handle the error... * } * + * Pass an existing error to the caller: + * error_propagate(errp, err); + * This is rarely needed. When @err is a local variable, use of + * ERRP_GUARD() commonly results in more readable code. + * + * Pass an existing error to the caller with the message modified: + * error_propagate_prepend(errp, err, + * "Could not frobnicate '%s': ", name); + * This is more concise than + * error_propagate(errp, err); // don't do this + * error_prepend(errp, "Could not frobnicate '%s': ", name); + * and works even when @errp is &error_fatal. + * * Receive and accumulate multiple errors (first one wins): * Error *err = NULL, *local_err = NULL; * foo(arg, &err); @@ -187,6 +202,69 @@ * error_setg(&err, ...); // WRONG! * } * because this may pass a non-null err to error_setg(). + * + * = Why, when and how to use ERRP_GUARD() = + * + * Without ERRP_GUARD(), use of the @errp parameter is restricted: + * - It must not be dereferenced, because it may be null. + * - It should not be passed to error_prepend() or + * error_append_hint(), because that doesn't work with &error_fatal. + * ERRP_GUARD() lifts these restrictions. + * + * To use ERRP_GUARD(), add it right at the beginning of the function. + * @errp can then be used without worrying about the argument being + * NULL or &error_fatal. + * + * Using it when it's not needed is safe, but please avoid cluttering + * the source with useless code. + * + * = Converting to ERRP_GUARD() = + * + * To convert a function to use ERRP_GUARD(): + * + * 0. If the Error ** parameter is not named @errp, rename it to + * @errp. + * + * 1. Add an ERRP_GUARD() invocation, by convention right at the + * beginning of the function. This makes @errp safe to use. + * + * 2. Replace &err by errp, and err by *errp. Delete local variable + * @err. + * + * 3. Delete error_propagate(errp, *errp), replace + * error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...) + * + * 4. Ensure @errp is valid at return: when you destroy *errp, set + * errp = NULL. + * + * Example: + * + * bool fn(..., Error **errp) + * { + * Error *err = NULL; + * + * foo(arg, &err); + * if (err) { + * handle the error... + * error_propagate(errp, err); + * return false; + * } + * ... + * } + * + * becomes + * + * bool fn(..., Error **errp) + * { + * ERRP_GUARD(); + * + * foo(arg, errp); + * if (*errp) { + * handle the error... + * return false; + * } + * ... + * } */ #ifndef ERROR_H @@ -287,6 +365,7 @@ void error_setg_win32_internal(Error **errp, * the error object. * Else, move the error object from @local_err to *@dst_errp. * On return, @local_err is invalid. + * Please use ERRP_GUARD() instead when possible. * Please don't error_propagate(&error_fatal, ...), use * error_report_err() and exit(), because that's more obvious. */ @@ -298,6 +377,7 @@ void error_propagate(Error **dst_errp, Error *local_err); * Behaves like * error_prepend(&local_err, fmt, ...); * error_propagate(dst_errp, local_err); + * Please use ERRP_GUARD() and error_prepend() instead when possible. */ void error_propagate_prepend(Error **dst_errp, Error *local_err, const char *fmt, ...); @@ -395,6 +475,46 @@ void error_set_internal(Error **errp, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(6, 7); +/* + * Make @errp parameter easier to use regardless of argument value + * + * This macro is for use right at the beginning of a function that + * takes an Error **errp parameter to pass errors to its caller. The + * parameter must be named @errp. + * + * It must be used when the function dereferences @errp or passes + * @errp to error_prepend(), error_vprepend(), or error_append_hint(). + * It is safe to use even when it's not needed, but please avoid + * cluttering the source with useless code. + * + * If @errp is NULL or &error_fatal, rewrite it to point to a local + * Error variable, which will be automatically propagated to the + * original @errp on function exit. + * + * Note: &error_abort is not rewritten, because that would move the + * abort from the place where the error is created to the place where + * it's propagated. + */ +#define ERRP_GUARD() \ + g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \ + do { \ + if (!errp || errp == &error_fatal) { \ + errp = &_auto_errp_prop.local_err; \ + } \ + } while (0) + +typedef struct ErrorPropagator { + Error *local_err; + Error **errp; +} ErrorPropagator; + +static inline void error_propagator_cleanup(ErrorPropagator *prop) +{ + error_propagate(prop->errp, prop->local_err); +} + +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup); + /* * Special error destination to abort on error. * See error_setg() and error_propagate() for details. -- cgit 1.4.1 From 8220f3ac74a4698b0734aa882e74c5d9337e3d99 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 7 Jul 2020 18:50:31 +0200 Subject: scripts: Coccinelle script to use ERRP_GUARD() Script adds ERRP_GUARD() macro invocations where appropriate and does corresponding changes in code (look for details in include/qapi/error.h) Usage example: spatch --sp-file scripts/coccinelle/errp-guard.cocci \ --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \ --max-width 80 FILES... Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster Message-Id: <20200707165037.1026246-3-armbru@redhat.com> Reviewed-by: Eric Blake [ERRP_AUTO_PROPAGATE() renamed to ERRP_GUARD(), and auto-propagated-errp.cocci to errp-guard.cocci] --- MAINTAINERS | 1 + include/qapi/error.h | 2 + scripts/coccinelle/errp-guard.cocci | 336 ++++++++++++++++++++++++++++++++++++ 3 files changed, 339 insertions(+) create mode 100644 scripts/coccinelle/errp-guard.cocci (limited to 'include/qapi/error.h') diff --git a/MAINTAINERS b/MAINTAINERS index f01284ebce..6aa54f7f8f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2176,6 +2176,7 @@ F: scripts/coccinelle/error-use-after-free.cocci F: scripts/coccinelle/error_propagate_null.cocci F: scripts/coccinelle/remove_local_err.cocci F: scripts/coccinelle/use-error_fatal.cocci +F: scripts/coccinelle/errp-guard.cocci GDB stub M: Alex Bennée diff --git a/include/qapi/error.h b/include/qapi/error.h index 85df875a3a..7932594dce 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -265,6 +265,8 @@ * } * ... * } + * + * For mass-conversion, use scripts/coccinelle/errp-guard.cocci. */ #ifndef ERROR_H diff --git a/scripts/coccinelle/errp-guard.cocci b/scripts/coccinelle/errp-guard.cocci new file mode 100644 index 0000000000..6e789acf2d --- /dev/null +++ b/scripts/coccinelle/errp-guard.cocci @@ -0,0 +1,336 @@ +// Use ERRP_GUARD() (see include/qapi/error.h) +// +// Copyright (c) 2020 Virtuozzo International GmbH. +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License as +// published by the Free Software Foundation; either version 2 of the +// License, or (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see +// . +// +// Usage example: +// spatch --sp-file scripts/coccinelle/errp-guard.cocci \ +// --macro-file scripts/cocci-macro-file.h --in-place \ +// --no-show-diff --max-width 80 FILES... +// +// Note: --max-width 80 is needed because coccinelle default is less +// than 80, and without this parameter coccinelle may reindent some +// lines which fit into 80 characters but not to coccinelle default, +// which in turn produces extra patch hunks for no reason. + +// Switch unusual Error ** parameter names to errp +// (this is necessary to use ERRP_GUARD). +// +// Disable optional_qualifier to skip functions with +// "Error *const *errp" parameter. +// +// Skip functions with "assert(_errp && *_errp)" statement, because +// that signals unusual semantics, and the parameter name may well +// serve a purpose. (like nbd_iter_channel_error()). +// +// Skip util/error.c to not touch, for example, error_propagate() and +// error_propagate_prepend(). +@ depends on !(file in "util/error.c") disable optional_qualifier@ +identifier fn; +identifier _errp != errp; +@@ + + fn(..., +- Error **_errp ++ Error **errp + ,...) + { +( + ... when != assert(_errp && *_errp) +& + <... +- _errp ++ errp + ...> +) + } + +// Add invocation of ERRP_GUARD() to errp-functions where // necessary +// +// Note, that without "when any" the final "..." does not mach +// something matched by previous pattern, i.e. the rule will not match +// double error_prepend in control flow like in +// vfio_set_irq_signaling(). +// +// Note, "exists" says that we want apply rule even if it does not +// match on all possible control flows (otherwise, it will not match +// standard pattern when error_propagate() call is in if branch). +@ disable optional_qualifier exists@ +identifier fn, local_err; +symbol errp; +@@ + + fn(..., Error **errp, ...) + { ++ ERRP_GUARD(); + ... when != ERRP_GUARD(); +( +( + error_append_hint(errp, ...); +| + error_prepend(errp, ...); +| + error_vprepend(errp, ...); +) + ... when any +| + Error *local_err = NULL; + ... +( + error_propagate_prepend(errp, local_err, ...); +| + error_propagate(errp, local_err); +) + ... +) + } + +// Warn when several Error * definitions are in the control flow. +// This rule is not chained to rule1 and less restrictive, to cover more +// functions to warn (even those we are not going to convert). +// +// Note, that even with one (or zero) Error * definition in the each +// control flow we may have several (in total) Error * definitions in +// the function. This case deserves attention too, but I don't see +// simple way to match with help of coccinelle. +@check1 disable optional_qualifier exists@ +identifier fn, _errp, local_err, local_err2; +position p1, p2; +@@ + + fn(..., Error **_errp, ...) + { + ... + Error *local_err = NULL;@p1 + ... when any + Error *local_err2 = NULL;@p2 + ... when any + } + +@ script:python @ +fn << check1.fn; +p1 << check1.p1; +p2 << check1.p2; +@@ + +print('Warning: function {} has several definitions of ' + 'Error * local variable: at {}:{} and then at {}:{}'.format( + fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line)) + +// Warn when several propagations are in the control flow. +@check2 disable optional_qualifier exists@ +identifier fn, _errp; +position p1, p2; +@@ + + fn(..., Error **_errp, ...) + { + ... +( + error_propagate_prepend(_errp, ...);@p1 +| + error_propagate(_errp, ...);@p1 +) + ... +( + error_propagate_prepend(_errp, ...);@p2 +| + error_propagate(_errp, ...);@p2 +) + ... when any + } + +@ script:python @ +fn << check2.fn; +p1 << check2.p1; +p2 << check2.p2; +@@ + +print('Warning: function {} propagates to errp several times in ' + 'one control flow: at {}:{} and then at {}:{}'.format( + fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line)) + +// Match functions with propagation of local error to errp. +// We want to refer these functions in several following rules, but I +// don't know a proper way to inherit a function, not just its name +// (to not match another functions with same name in following rules). +// Not-proper way is as follows: rename errp parameter in functions +// header and match it in following rules. Rename it back after all +// transformations. +// +// The common case is a single definition of local_err with at most one +// error_propagate_prepend() or error_propagate() on each control-flow +// path. Functions with multiple definitions or propagates we want to +// examine manually. Rules check1 and check2 emit warnings to guide us +// to them. +// +// Note that we match not only this "common case", but any function, +// which has the "common case" on at least one control-flow path. +@rule1 disable optional_qualifier exists@ +identifier fn, local_err; +symbol errp; +@@ + + fn(..., Error ** +- errp ++ ____ + , ...) + { + ... + Error *local_err = NULL; + ... +( + error_propagate_prepend(errp, local_err, ...); +| + error_propagate(errp, local_err); +) + ... + } + +// Convert special case with goto separately. +// I tried merging this into the following rule the obvious way, but +// it made Coccinelle hang on block.c +// +// Note interesting thing: if we don't do it here, and try to fixup +// "out: }" things later after all transformations (the rule will be +// the same, just without error_propagate() call), coccinelle fails to +// match this "out: }". +@ disable optional_qualifier@ +identifier rule1.fn, rule1.local_err, out; +symbol errp; +@@ + + fn(..., Error ** ____, ...) + { + <... +- goto out; ++ return; + ...> +- out: +- error_propagate(errp, local_err); + } + +// Convert most of local_err related stuff. +// +// Note, that we inherit rule1.fn and rule1.local_err names, not +// objects themselves. We may match something not related to the +// pattern matched by rule1. For example, local_err may be defined with +// the same name in different blocks inside one function, and in one +// block follow the propagation pattern and in other block doesn't. +// +// Note also that errp-cleaning functions +// error_free_errp +// error_report_errp +// error_reportf_errp +// warn_report_errp +// warn_reportf_errp +// are not yet implemented. They must call corresponding Error* - +// freeing function and then set *errp to NULL, to avoid further +// propagation to original errp (consider ERRP_GUARD in use). +// For example, error_free_errp may look like this: +// +// void error_free_errp(Error **errp) +// { +// error_free(*errp); +// *errp = NULL; +// } +@ disable optional_qualifier exists@ +identifier rule1.fn, rule1.local_err; +expression list args; +symbol errp; +@@ + + fn(..., Error ** ____, ...) + { + <... +( +- Error *local_err = NULL; +| + +// Convert error clearing functions +( +- error_free(local_err); ++ error_free_errp(errp); +| +- error_report_err(local_err); ++ error_report_errp(errp); +| +- error_reportf_err(local_err, args); ++ error_reportf_errp(errp, args); +| +- warn_report_err(local_err); ++ warn_report_errp(errp); +| +- warn_reportf_err(local_err, args); ++ warn_reportf_errp(errp, args); +) +?- local_err = NULL; + +| +- error_propagate_prepend(errp, local_err, args); ++ error_prepend(errp, args); +| +- error_propagate(errp, local_err); +| +- &local_err ++ errp +) + ...> + } + +// Convert remaining local_err usage. For example, different kinds of +// error checking in if conditionals. We can't merge this into +// previous hunk, as this conflicts with other substitutions in it (at +// least with "- local_err = NULL"). +@ disable optional_qualifier@ +identifier rule1.fn, rule1.local_err; +symbol errp; +@@ + + fn(..., Error ** ____, ...) + { + <... +- local_err ++ *errp + ...> + } + +// Always use the same pattern for checking error +@ disable optional_qualifier@ +identifier rule1.fn; +symbol errp; +@@ + + fn(..., Error ** ____, ...) + { + <... +- *errp != NULL ++ *errp + ...> + } + +// Revert temporary ___ identifier. +@ disable optional_qualifier@ +identifier rule1.fn; +@@ + + fn(..., Error ** +- ____ ++ errp + , ...) + { + ... + } -- cgit 1.4.1