Re: [PATCH 08/46] error: Avoid unnecessary error_propagate() after error_setg()

[Eric Blake]

On 6/24/20 11:43 AM, Markus Armbruster wrote:
> Replace
>
>      error_setg(&err, ...);
>      error_propagate(errp, err);
>
> by
>
>      error_setg(errp, ...);
>
> Related pattern:

Nice explanation.

> Bonus: the elimination of gotos will make later patches in this series
> easier to review.
>
> Candidates for conversion tracked down with this Coccinelle script:
>
>      @@
>      identifier err, errp;
>      expression list args;
>      @@
>      -    error_setg(&err, args);
>      +    error_setg(errp, args);
>          ... when != err
>          error_propagate(errp, err);
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   backends/cryptodev.c        | 11 +++---
>   backends/hostmem-file.c     | 19 +++-------
>   backends/hostmem-memfd.c    | 15 ++++----
>   backends/hostmem.c          | 27 ++++++--------
>   block/throttle-groups.c     | 22 +++++------
>   hw/hyperv/vmbus.c           |  5 +--
>   hw/i386/pc.c                | 35 ++++++------------
>   hw/mem/nvdimm.c             | 17 ++++-----
>   hw/mem/pc-dimm.c            | 14 +++----
>   hw/misc/aspeed_sdmc.c       |  3 +-
>   hw/ppc/rs6000_mc.c          |  9 ++---
>   hw/ppc/spapr.c              | 73 ++++++++++++++++---------------------
>   hw/ppc/spapr_pci.c          | 14 +++----
>   hw/s390x/ipl.c              | 23 +++++-------
>   hw/s390x/sclp.c             | 12 ++----
>   hw/xen/xen_pt_config_init.c |  3 +-
>   iothread.c                  | 12 +++---
>   net/colo-compare.c          | 20 ++++------
>   net/dump.c                  | 10 ++---
>   net/filter-buffer.c         | 10 ++---
>   qga/commands-win32.c        | 16 +++-----
>   21 files changed, 151 insertions(+), 219 deletions(-)

A bit bigger, and starts to be too complex to ask Coccinelle to directly 
fix it (but at least using it for identification is nice).  But the 
patch is still manageable, and hopefully not too many instances creep 
back in during the meantime while waiting for this series to land.

> @@ -140,7 +138,6 @@ static void file_memory_backend_set_pmem(Object *o, bool 
> value, Error **errp)
>       HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
>   
>       if (host_memory_backend_mr_inited(backend)) {
> -
>           error_setg(errp, "cannot change property 'pmem' of %s.",
>                      object_get_typename(o));
>           return;

Unrelated cleanup.  Does it belong in a different patch?

> @@ -148,13 +145,9 @@ static void file_memory_backend_set_pmem(Object *o, bool 
> value, Error **errp)
>   
>   #ifndef CONFIG_LIBPMEM
>       if (value) {
> -        Error *local_err = NULL;
> -
> -        error_setg(&local_err,
> -                   "Lack of libpmem support while setting the 'pmem=on'"
> +        error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'"
>                      " of %s. We can't ensure data persistence.",

Pre-existing - doesn't follow our usual error message content 
conventions regarding trailing '.'.

> +++ b/hw/ppc/spapr_pci.c
> @@ -1517,15 +1517,16 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
>        */
>       if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
>           PCI_FUNC(pdev->devfn) != 0) {
> -        error_setg(&local_err, "PCI: slot %d function 0 already ocuppied by 
> %s,"
> +        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
>                      " additional functions can no longer be exposed to guest.",

Another one.  Also, s/ocuppied/occupied/ while touching it.

Reviewed-by: Eric Blake <eblake@redhat.com>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org