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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> 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?

I don't have a better one at hand.  The only other patch touching this
file is a worse choice: PATCH 23.

>> @@ -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 '.'.

Yes.  But this patch feels too big for me to also improve error
messages.  I can do it separately.

>> +++ 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.

Likewise.

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

Thanks!