Re: [PATCH v2 33/44] error: Avoid unnecessary error_propagate() after error_setg()

[Markus Armbruster]

Eric Blake  writes:

> On 7/2/20 10:49 AM, Markus Armbruster wrote:
>> Replace
>>
>>  error_setg(&err, ...);
>>  error_propagate(errp, err);
>>
>> by
>>
>>  error_setg(errp, ...);
>>
>
>>
>> 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 
>> ---
>
>> +++ b/backends/cryptodev.c
>> @@ -158,16 +158,15 @@ cryptodev_backend_set_queues(Object *obj, Visitor *v, 
>> const char *name,
>>   uint32_t value;
>> if (!visit_type_uint32(v, name, &value, &local_err)) {
>> -goto out;
>> +error_propagate(errp, local_err);
>> +return;
>>   }
>
> Looks like this error_propgate is spurious if you just use
>  if (!visit_type_uint32(..., errp)) {
>
> Oh - that's not the pattern you flagged, and a later patch then
> addresses it.  It might help if the commit message for this patch
> mentions that further cleanups are still forthcoming.

Perhaps:

In some places, the transformation results in obviously unnecessary
error_propagate().  The next few commits will eliminate them.

>> +++ b/backends/hostmem-file.c
>> @@ -114,18 +114,16 @@ static void file_memory_backend_set_align(Object *o, 
>> Visitor *v,
>>   uint64_t val;
>> if (host_memory_backend_mr_inited(backend)) {
>> -error_setg(&local_err, "cannot change property '%s' of %s",
>> -   name, object_get_typename(o));
>> -goto out;
>> +error_setg(errp, "cannot change property '%s' of %s", name,
>> +   object_get_typename(o));
>> +return;
>>   }
>> if (!visit_type_size(v, name, &val, &local_err)) {
>> -goto out;
>> +error_propagate(errp, local_err);
>> +return;
>>   }
>
> Another case where the first 'if' matches the subject of this patch,
> and the second 'if' can avoid local_err but that the change will be
> done in a later patch.  And several more later on, but this is how far
> it took me to realize that you intentionally saved them for later.
>
> Reviewed-by: Eric Blake 

Thanks!

Re: [PATCH v2 33/44] error: Avoid unnecessary error_propagate() after error_setg()

[Eric Blake]

On 7/2/20 10:49 AM, Markus Armbruster wrote:

Replace

 error_setg(&err, ...);
 error_propagate(errp, err);

by

 error_setg(errp, ...);





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



+++ b/backends/cryptodev.c
@@ -158,16 +158,15 @@ cryptodev_backend_set_queues(Object *obj, Visitor *v, 
const char *name,
  uint32_t value;
  
  if (!visit_type_uint32(v, name, &value, &local_err)) {

-goto out;
+error_propagate(errp, local_err);
+return;
  }


Looks like this error_propgate is spurious if you just use
 if (!visit_type_uint32(..., errp)) {

Oh - that's not the pattern you flagged, and a later patch then 
addresses it.  It might help if the commit message for this patch 
mentions that further cleanups are still forthcoming.




+++ b/backends/hostmem-file.c
@@ -114,18 +114,16 @@ static void file_memory_backend_set_align(Object *o, 
Visitor *v,
  uint64_t val;
  
  if (host_memory_backend_mr_inited(backend)) {

-error_setg(&local_err, "cannot change property '%s' of %s",
-   name, object_get_typename(o));
-goto out;
+error_setg(errp, "cannot change property '%s' of %s", name,
+   object_get_typename(o));
+return;
  }
  
  if (!visit_type_size(v, name, &val, &local_err)) {

-goto out;
+error_propagate(errp, local_err);
+return;
  }


Another case where the first 'if' matches the subject of this patch, and 
the second 'if' can avoid local_err but that the change will be done in 
a later patch.  And several more later on, but this is how far it took 
me to realize that you intentionally saved them for later.


Reviewed-by: Eric Blake 

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

[PATCH v2 33/44] error: Avoid unnecessary error_propagate() after error_setg()

[Markus Armbruster]

Replace

error_setg(&err, ...);
error_propagate(errp, err);

by

error_setg(errp, ...);

Related pattern:

if (...) {
error_setg(&err, ...);
goto out;
}
...
 out:
error_propagate(errp, err);
return;

When all paths to label out are that way, replace by

if (...) {
error_setg(errp, ...);
return;
}

and delete the label along with the error_propagate().

When we have at most one other path that actually needs to propagate,
and maybe one at the end that where propagation is unnecessary, e.g.

foo(..., &err);
if (err) {
goto out;
}
...
bar(..., &err);
 out:
error_propagate(errp, err);
return;

move the error_propagate() to where it's needed, like

if (...) {
foo(..., &err);
error_propagate(errp, err);
return;
}
...
bar(..., errp);
return;

and transform the error_setg() as above.

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 
---
 backends/cryptodev.c| 11 +++---
 backends/hostmem-file.c | 19 +++---
 backends/hostmem-memfd.c| 15 
 backends/hostmem.c  | 27 ++
 block/quorum.c  | 16 
 block/replication.c | 11 +++---
 block/throttle-groups.c | 22 +--
 block/vxhs.c|  9 ++---
 hw/acpi/core.c  | 15 +++-
 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/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| 19 +++---
 24 files changed, 169 insertions(+), 243 deletions(-)

diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index 72b7077475..4de378532b 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -158,16 +158,15 @@ cryptodev_backend_set_queues(Object *obj, Visitor *v, 
const char *name,
 uint32_t value;
 
 if (!visit_type_uint32(v, name, &value, &local_err)) {
-goto out;
+error_propagate(errp, local_err);
+return;
 }
 if (!value) {
-error_setg(&local_err, "Property '%s.%s' doesn't take value '%"
-   PRIu32 "'", object_get_typename(obj), name, value);
-goto out;
+error_setg(errp, "Property '%s.%s' doesn't take value '%" PRIu32 "'",
+   object_get_typename(obj), name, value);
+return;
 }
 backend->conf.peers.queues = value;
-out:
-error_propagate(errp, local_err);
 }
 
 static void
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 320dffbaa9..a44f5a61ac 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -114,18 +114,16 @@ static void file_memory_backend_set_align(Object *o, 
Visitor *v,
 uint64_t val;
 
 if (host_memory_backend_mr_inited(backend)) {
-error_setg(&local_err, "cannot change property '%s' of %s",
-   name, object_get_typename(o));
-goto out;
+error_setg(errp, "cannot change property '%s' of %s", name,
+   object_get_typename(o));
+return;
 }
 
 if (!visit_type_size(v, name, &val, &local_err)) {
-goto out;
+error_propagate(errp, local_err);
+return;
 }
 fb->align = val;
-
- out:
-error_propagate(errp, local_err);
 }
 
 static bool file_memory_backend_get_pmem(Object *o, Error **errp)
@@ -139,7 +137,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;
@@ -147,13 +144,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.",
object_get_typename(o));