I tried this script on the whole tree. Observations:
* $ git-diff --shortstat \*.[ch]
333 files changed, 3480 insertions(+), 4586 deletions(-)
* Twelve functions have "several definitions of Error * local variable".
Eight declare such a variable within a loop. Reported because
Coccinelle matches along control flow, not just along text. Ignore.
Remaining four:
* ivshmem_common_realize()
Two variables (messed up in commit fe44dc91807), should be replaced
by one.
* qmp_query_cpu_model_expansion() two times
Three declarations in separate blocks; two should be replaced by
&error_abort, one moved to the function block.
* xen_block_device_destroy()
Two declarations in seperate blocks; should be replaced by a single
one.
Separate manual cleanup patches, ideally applied before running
Coccinelle to keep Coccinelle's changes as simple and safe as
possible. I'll post patches. Only the one for
xen_block_device_destroy() affects by this series.
* No function "propagates to errp several times"
I tested the rule does detect this as advertized by feeding it an
obvious example. We're good.
* ERRP_AUTO_PROPAGATE() inserted 744 times, always right at the
beginning of a function.
* As far as I can tell, all changed functions have ERRP_AUTO_PROPAGATE()
inserted. Good.
* Almost 1100 error propagations dropped:error_propagate() removed,
error_propagate_prepend() replaced by just error_prepend().
* Four error_propagate() are transformed. Two instances each in
aspeed_soc_ast2600_realize() and aspeed_soc_realize(). Pattern:
{
+ ERRP_AUTO_PROPAGATE();
...
- Error *err = NULL, *local_err = NULL;
+ Error *local_err = NULL;
...
object_property_set_T(...,
- &err);
+ errp);
object_property_set_T(...,
&local_err);
- error_propagate(&err, local_err);
- if (err) {
- error_propagate(errp, err);
+ error_propagate(errp, local_err);
+ if (*errp) {
return;
}
This is what error.h calls "Receive and accumulate multiple errors
(first one wins)".
Result:
ERRP_AUTO_PROPAGATE();
...
Error *local_err = NULL;
...
object_property_set_T(..., errp);
object_property_set_T(..., &local_err);
error_propagate(errp, local_err);
if (*errp) {
return;
}
Could be done without the accumulation:
ERRP_AUTO_PROPAGATE();
...
object_property_set_T(..., errp);
if (*errp) {
return;
}
object_property_set_T(..., errp);
if (*errp) {
return;
}
I find this a bit easier to understand. Matter of taste. If we want
to change to this, do it manually and separately. I'd do it on top.
* Some 90 propagations remain.
Some of them could use cleanup, e.g. file_memory_backend_set_pmem(),
css_clear_io_interrupt(). Out of scope for this series.
Some move errors around in unusual ways, e.g. in block/nbd.c. Could
use review. Out of scope for this series.
I spotted three that should be transformed, but aren't:
- qcrypto_block_luks_store_key()
I believe g_autoptr() confuses Coccinelle. Undermines all our
Coccinelle use, not just this patch. I think we need to update
scripts/cocci-macro-file.h for it.
- armsse_realize()
Something in this huge function confuses Coccinelle, but I don't
know what exactly. If I delete most of it, the error_propagate()
transforms okay. If I delete less, Coccinelle hangs.
- apply_cpu_model()
Gets transformed fine if I remove the #ifndef CONFIG_USER_ONLY. I
have no idea why the #if spooks Coccinelle here, but not elsewhere.
None of these three affects this series. No need to hold it back for
further investigation.
* 30 error_free() and two warn_reportf_err() are transformed. Patterns:
- error_free(local_err);
- local_err = NULL;
+ error_free_errp(errp);
and
- error_free(local_err);
+ error_free_errp(errp);
and
- warn_report_err(local_err);
- local_err = NULL;
+ warn_report_errp(errp);
Good.
* Many error_free(), error_reportf_err() and warn_reportf_err() remain.
None of them have an argument of the form *errp. Such arguments would
have to be reviewed for possible interference with
ERRP_AUTO_PROPAGATE().
* Almost 700 Error *err = NULL removed. Almost 600 remain.
* Error usage in rdma.c is questionable / wrong. Out of scope for this
series.
As far as I can tell, your Coccinelle script is working as intended,
except for three missed error propagations noted above. We can proceed
with this series regardless, if we want. I'd prefer to integrate my
forthcoming cleanup to xen_block_device_destroy(), though.