If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
    spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
    --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
    ...
    goto out;
    ...
    out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/misc/ivshmem.c | 37 ++++++++++++++++---------------------
 hw/misc/tmp105.c  |  7 +++----
 hw/misc/tmp421.c  |  7 +++----
 3 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 5e3b05eae0..31292c3f43 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -474,11 +474,11 @@ static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector,
 
 static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
     bool with_irqfd = kvm_msi_via_irqfd_enabled() &&
         ivshmem_has_feature(s, IVSHMEM_MSI);
     PCIDevice *pdev = PCI_DEVICE(s);
-    Error *err = NULL;
 
     IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector);
 
@@ -487,9 +487,8 @@ static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
         watch_vector_notifier(s, n, vector);
     } else if (msix_enabled(pdev)) {
         IVSHMEM_DPRINTF("with irqfd\n");
-        ivshmem_add_kvm_msi_virq(s, vector, &err);
-        if (err) {
-            error_propagate(errp, err);
+        ivshmem_add_kvm_msi_virq(s, vector, errp);
+        if (*errp) {
             return;
         }
 
@@ -506,7 +505,7 @@ static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
 
 static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
 {
-    Error *local_err = NULL;
+    ERRP_AUTO_PROPAGATE();
     struct stat buf;
     size_t size;
 
@@ -527,9 +526,8 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
 
     /* mmap the region and map into the BAR2 */
     memory_region_init_ram_from_fd(&s->server_bar2, OBJECT(s),
-                                   "ivshmem.bar2", size, true, fd, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+                                   "ivshmem.bar2", size, true, fd, errp);
+    if (*errp) {
         return;
     }
 
@@ -662,13 +660,12 @@ static int64_t ivshmem_recv_msg(IVShmemState *s, int *pfd, Error **errp)
 
 static void ivshmem_recv_setup(IVShmemState *s, Error **errp)
 {
-    Error *err = NULL;
+    ERRP_AUTO_PROPAGATE();
     int64_t msg;
     int fd;
 
-    msg = ivshmem_recv_msg(s, &fd, &err);
-    if (err) {
-        error_propagate(errp, err);
+    msg = ivshmem_recv_msg(s, &fd, errp);
+    if (*errp) {
         return;
     }
     if (msg != IVSHMEM_PROTOCOL_VERSION) {
@@ -694,9 +691,8 @@ static void ivshmem_recv_setup(IVShmemState *s, Error **errp)
      * older versions of the device accepted it out of order, but
      * broke when an interrupt setup message arrived before it.
      */
-    msg = ivshmem_recv_msg(s, &fd, &err);
-    if (err) {
-        error_propagate(errp, err);
+    msg = ivshmem_recv_msg(s, &fd, errp);
+    if (*errp) {
         return;
     }
     if (fd != -1 || msg < 0 || msg > IVSHMEM_MAX_PEERS) {
@@ -709,14 +705,12 @@ static void ivshmem_recv_setup(IVShmemState *s, Error **errp)
      * Receive more messages until we got shared memory.
      */
     do {
-        msg = ivshmem_recv_msg(s, &fd, &err);
-        if (err) {
-            error_propagate(errp, err);
+        msg = ivshmem_recv_msg(s, &fd, errp);
+        if (*errp) {
             return;
         }
-        process_msg(s, msg, fd, &err);
-        if (err) {
-            error_propagate(errp, err);
+        process_msg(s, msg, fd, errp);
+        if (*errp) {
             return;
         }
     } while (msg != -1);
@@ -864,6 +858,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
 
 static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     IVShmemState *s = IVSHMEM_COMMON(dev);
     Error *err = NULL;
     uint8_t *pci_conf;
diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
index 75ddad3a12..c603f7024d 100644
--- a/hw/misc/tmp105.c
+++ b/hw/misc/tmp105.c
@@ -71,13 +71,12 @@ static void tmp105_get_temperature(Object *obj, Visitor *v, const char *name,
 static void tmp105_set_temperature(Object *obj, Visitor *v, const char *name,
                                    void *opaque, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     TMP105State *s = TMP105(obj);
-    Error *local_err = NULL;
     int64_t temp;
 
-    visit_type_int(v, name, &temp, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    visit_type_int(v, name, &temp, errp);
+    if (*errp) {
         return;
     }
     if (temp >= 128000 || temp < -128000) {
diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c
index 9f044705fa..20f069513c 100644
--- a/hw/misc/tmp421.c
+++ b/hw/misc/tmp421.c
@@ -140,16 +140,15 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name,
 static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name,
                                    void *opaque, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     TMP421State *s = TMP421(obj);
-    Error *local_err = NULL;
     int64_t temp;
     bool ext_range = (s->config[0] & TMP421_CONFIG_RANGE);
     int offset = ext_range * 64 * 256;
     int tempid;
 
-    visit_type_int(v, name, &temp, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    visit_type_int(v, name, &temp, errp);
+    if (*errp) {
         return;
     }
 
-- 
2.21.0

On 10/11/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> If we want to add some info to errp (by error_prepend() or
> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
> Otherwise, this info will not be added when errp == &fatal_err
> (the program will exit prior to the error_append_hint() or
> error_prepend() call).  Fix such cases.
> 
> If we want to check error after errp-function call, we need to
> introduce local_err and than propagate it to errp. Instead, use
> ERRP_AUTO_PROPAGATE macro, benefits are:
> 1. No need of explicit error_propagate call
> 2. No need of explicit local_err variable: use errp directly
> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
>     &error_fatel, this means that we don't break error_abort
>     (we'll abort on error_set, not on error_propagate)
> 
> This commit (together with its neighbors) was generated by
> 
> for f in $(git grep -l errp \*.[ch]); do \
>      spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>      --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
> done;
> 
> then fix a bit of compilation problems: coccinelle for some reason
> leaves several
> f() {
>      ...
>      goto out;
>      ...
>      out:
> }
> patterns, with "out:" at function end.

Was that still happening even after your tweaks to the .cocci script? 
But manual touch-up after cocci is not unheard of, so it is not a 
showstopper to the series.  Still, it might be nicer if this disclaimer 
only appears on the patches within the series where it actually matters, 
rather than on every message in the series even when no tweaks were 
needed (as this patch is an example where the touchup was not needed).

> 
> then
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
> 

If we don't check the python script into git, then changing this to a 
URL to one of the threads where you posted the script in an earlier 
version of the patch is also acceptable.

> (auto-msg was a file with this commit message)
> 
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   hw/misc/ivshmem.c | 37 ++++++++++++++++---------------------
>   hw/misc/tmp105.c  |  7 +++----
>   hw/misc/tmp421.c  |  7 +++----
>   3 files changed, 22 insertions(+), 29 deletions(-)
> 

> @@ -864,6 +858,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>   
>   static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       IVShmemState *s = IVSHMEM_COMMON(dev);
>       Error *err = NULL;

Umm, so why did Coccinelle not remove this line, or retouch lower down at:

     if (!ivshmem_is_master(s)) {
         error_setg(&s->migration_blocker,
                    "Migration is disabled when using feature 'peer 
mode' in devi
ce 'ivshmem'");
         migrate_add_blocker(s->migration_blocker, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             error_free(s->migration_blocker);
             return;
         }
     }


But the conversions that Coccinelle made look correct.

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

11.10.2019 21:44, Eric Blake wrote:
> On 10/11/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>> If we want to add some info to errp (by error_prepend() or
>> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
>> Otherwise, this info will not be added when errp == &fatal_err
>> (the program will exit prior to the error_append_hint() or
>> error_prepend() call).  Fix such cases.
>>
>> If we want to check error after errp-function call, we need to
>> introduce local_err and than propagate it to errp. Instead, use
>> ERRP_AUTO_PROPAGATE macro, benefits are:
>> 1. No need of explicit error_propagate call
>> 2. No need of explicit local_err variable: use errp directly
>> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
>>     &error_fatel, this means that we don't break error_abort
>>     (we'll abort on error_set, not on error_propagate)
>>
>> This commit (together with its neighbors) was generated by
>>
>> for f in $(git grep -l errp \*.[ch]); do \
>>      spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>      --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
>> done;
>>
>> then fix a bit of compilation problems: coccinelle for some reason
>> leaves several
>> f() {
>>      ...
>>      goto out;
>>      ...
>>      out:
>> }
>> patterns, with "out:" at function end.
> 
> Was that still happening even after your tweaks to the .cocci script?

Yes, seem coccinella succesfully removes

out:
error_prapagate

pattern in general, but fails to work if there is additional "return;" :

return;
out:
error_proapagate

> But manual touch-up after cocci is not unheard of, so it is not a showstopper to the series.  Still, it might be nicer if this disclaimer only appears on the patches within the series where it actually matters, rather than on every message in the series even when no tweaks were needed (as this patch is an example where the touchup was not needed).

Hmm.. Not sure. I think it's good to have in each commit an instruction how to generate the whole sequence. Still, what you want is not difficult: just instead of fixing all compilation errors at once, commit the changes and than play with git rebase -x 'make -j9'.

> 
>>
>> then
>> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>>
> 
> If we don't check the python script into git, then changing this to a URL to one of the threads where you posted the script in an earlier version of the patch is also acceptable.
> 
>> (auto-msg was a file with this commit message)
>>
>> Still, for backporting it may be more comfortable to use only the first
>> command and then do one huge commit.
>>
>> Reported-by: Kevin Wolf <kwolf@redhat.com>
>> Reported-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   hw/misc/ivshmem.c | 37 ++++++++++++++++---------------------
>>   hw/misc/tmp105.c  |  7 +++----
>>   hw/misc/tmp421.c  |  7 +++----
>>   3 files changed, 22 insertions(+), 29 deletions(-)
>>
> 
>> @@ -864,6 +858,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>   static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>>   {
>> +    ERRP_AUTO_PROPAGATE();
>>       IVShmemState *s = IVSHMEM_COMMON(dev);
>>       Error *err = NULL;
> 
> Umm, so why did Coccinelle not remove this line, or retouch lower down at:
> 
>      if (!ivshmem_is_master(s)) {
>          error_setg(&s->migration_blocker,
>                     "Migration is disabled when using feature 'peer mode' in devi
> ce 'ivshmem'");
>          migrate_add_blocker(s->migration_blocker, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              error_free(s->migration_blocker);
>              return;
>          }
>      }
> 
> 
> But the conversions that Coccinelle made look correct.
> 

Hmmm. strange. So it does nothing, except add a macro invocation?

Intersting: if I comment definition for local_err, it correctly updates code around err,
if I comment definition for err, it correctly updates code around local_err.

So, rule0 works, but rule1 don't know what to do with two Error * variables.

Seems, simplest thing is to pre-refactor it, to drop local_err variable.

-- 
Best regards,
Vladimir