// 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 , ...) { ... }