Re: [Xen-devel] [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster <armbru@xxxxxxxxxx>]

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel