[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGA

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
Date: Fri, 13 Mar 2020 18:22:21 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

13.03.2020 17:58, Markus Armbruster wrote:
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

   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:

     -    Error *err = NULL, *local_err = NULL;
     +    Error *local_err = NULL;

     -                              &err);
     +                              errp);
     -        error_propagate(&err, local_err);
     -        if (err) {
     -            error_propagate(errp, err);
     +        error_propagate(errp, local_err);
     +        if (*errp) {

   This is what error.h calls "Receive and accumulate multiple errors
   (first one wins)".


         Error *local_err = NULL;

             object_property_set_T(..., errp);
             object_property_set_T(..., &local_err);
             error_propagate(errp, local_err);
             if (*errp) {

   Could be done without the accumulation:


             object_property_set_T(..., errp);
             if (*errp) {
             object_property_set_T(..., errp);
             if (*errp) {

   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);


     -    error_free(local_err);
     +    error_free_errp(errp);


     -    warn_report_err(local_err);
     -    local_err = NULL;
     +    warn_report_errp(errp);


* 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

* Almost 700 Error *err = NULL removed.  Almost 600 remain.

* Error usage in rdma.c is questionable / wrong.  Out of scope for this

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.

Great investigation!!!

I'm for proceeding as is, or with your cleanup for xen_block_device_destroy().
Still, script converts xen_block_device_destroy correctly anyway, resulting code
will be the same and we are OK without cleanup as well.

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]