[PULL 0/4] Error reporting patches patches for 2020-07-24

[PULL 0/4] Error reporting patches patches for 2020-07-24

[Markus Armbruster]

The following changes since commit 09e0cd773723219d21655587954da2769f64ba01:

  Merge remote-tracking branch 'remotes/alistair/tags/pull-riscv-to-apply-20200722-1' into staging (2020-07-23 19:00:42 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-error-2020-07-24

for you to fetch changes up to 192cf54ac5408d21c20c17b3794a632970bbb880:

  qapi/error: Check format string argument in error_*prepend() (2020-07-24 15:03:09 +0200)

----------------------------------------------------------------
Error reporting patches patches for 2020-07-24

----------------------------------------------------------------
Markus Armbruster (2):
      coccinelle/err-bad-newline: Fix for Python 3, and add patterns
      error: Strip trailing '\n' from error string arguments (again)

Philippe Mathieu-Daudé (1):
      qapi/error: Check format string argument in error_*prepend()

Stefan Weil (1):
      sd/milkymist-memcard: Fix format string

 scripts/coccinelle/err-bad-newline.cocci | 24 ++++++++++++++++++++++--
 include/qapi/error.h                     |  6 ++++--
 hw/i386/intel_iommu.c                    |  6 +++---
 hw/sd/milkymist-memcard.c                |  2 +-
 target/ppc/mmu-hash64.c                  |  2 +-
 5 files changed, 31 insertions(+), 9 deletions(-)

-- 
2.26.2

[PULL 1/4] coccinelle/err-bad-newline: Fix for Python 3, and add patterns

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200722084048.1726105-2-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/coccinelle/err-bad-newline.cocci | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/err-bad-newline.cocci b/scripts/coccinelle/err-bad-newline.cocci
index 1316cc86a6..5394421873 100644
--- a/scripts/coccinelle/err-bad-newline.cocci
+++ b/scripts/coccinelle/err-bad-newline.cocci
@@ -1,22 +1,42 @@
 // Error messages should not contain newlines.  This script finds
 // messages that do.  Fixing them is manual.
 @r@
-expression errp, eno, cls, fmt;
+expression errp, err, eno, cls, fmt, ap;
 position p;
 @@
 (
+error_vreport(fmt, ap)@p
+|
+warn_vreport(fmt, ap)@p
+|
+info_vreport(fmt, ap)@p
+|
 error_report(fmt, ...)@p
 |
+warn_report(fmt, ...)@p
+|
+info_report(fmt, ...)@p
+|
+error_report_once(fmt, ...)@p
+|
+warn_report_once(fmt, ...)@p
+|
 error_setg(errp, fmt, ...)@p
 |
 error_setg_errno(errp, eno, fmt, ...)@p
 |
 error_setg_win32(errp, eno, cls, fmt, ...)@p
 |
+error_propagate_prepend(errp, err, fmt, ...)@p
+|
+error_vprepend(errp, fmt, ap)@p
+|
 error_prepend(errp, fmt, ...)@p
 |
 error_setg_file_open(errp, eno, cls, fmt, ...)@p
 |
+warn_reportf_err(errp, fmt, ...)@p
+|
 error_reportf_err(errp, fmt, ...)@p
 |
 error_set(errp, cls, fmt, ...)@p
@@ -26,4 +46,4 @@ fmt << r.fmt;
 p << r.p;
 @@
 if "\\n" in str(fmt):
-    print "%s:%s:%s:%s" % (p[0].file, p[0].line, p[0].column, fmt)
+    print("%s:%s:%s:%s" % (p[0].file, p[0].line, p[0].column, fmt))
-- 
2.26.2

[PULL 2/4] error: Strip trailing '\n' from error string arguments (again)

[Markus Armbruster]

Tracked down with scripts/coccinelle/err-bad-newline.cocci.

Cc: Peter Xu <peterx@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200722084048.1726105-3-armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c   | 6 +++---
 target/ppc/mmu-hash64.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 0c286635cf..5284bb68b6 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2356,7 +2356,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
     if ((inv_desc->lo & VTD_INV_DESC_IOTLB_RSVD_LO) ||
         (inv_desc->hi & VTD_INV_DESC_IOTLB_RSVD_HI)) {
         error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
-                          ", lo=0x%"PRIx64" (reserved bits unzero)\n",
+                          ", lo=0x%"PRIx64" (reserved bits unzero)",
                           __func__, inv_desc->hi, inv_desc->lo);
         return false;
     }
@@ -2377,7 +2377,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
         am = VTD_INV_DESC_IOTLB_AM(inv_desc->hi);
         if (am > VTD_MAMV) {
             error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
-                              ", lo=0x%"PRIx64" (am=%u > VTD_MAMV=%u)\n",
+                              ", lo=0x%"PRIx64" (am=%u > VTD_MAMV=%u)",
                               __func__, inv_desc->hi, inv_desc->lo,
                               am, (unsigned)VTD_MAMV);
             return false;
@@ -2387,7 +2387,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
 
     default:
         error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
-                          ", lo=0x%"PRIx64" (type mismatch: 0x%llx)\n",
+                          ", lo=0x%"PRIx64" (type mismatch: 0x%llx)",
                           __func__, inv_desc->hi, inv_desc->lo,
                           inv_desc->lo & VTD_INV_DESC_IOTLB_G);
         return false;
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index e5baabf0e1..c31d21e6a9 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -859,7 +859,7 @@ static int build_vrma_slbe(PowerPCCPU *cpu, ppc_slb_t *slb)
     }
 
     error_report("Bad page size encoding in LPCR[VRMASD]; LPCR=0x"
-                 TARGET_FMT_lx"\n", lpcr);
+                 TARGET_FMT_lx, lpcr);
 
     return -1;
 }
-- 
2.26.2

[PULL 3/4] sd/milkymist-memcard: Fix format string

[Markus Armbruster]

From: Stefan Weil <sw@weilnetz.de>

Fixes: b98e8d1230ff7023bb34ddeb7194424dfcbaf789
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Message-Id: <20200722204054.1400555-1-sw@weilnetz.de>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sd/milkymist-memcard.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index afdb8aa0c0..11f61294fc 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -281,7 +281,7 @@ static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
     carddev = qdev_new(TYPE_SD_CARD);
     qdev_prop_set_drive(carddev, "drive", blk);
     if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err)) {
-        error_propagate_prepend(errp, err, "failed to init SD card: %s");
+        error_propagate_prepend(errp, err, "failed to init SD card");
         return;
     }
     s->enabled = blk && blk_is_inserted(blk);
-- 
2.26.2

[PULL 4/4] qapi/error: Check format string argument in error_*prepend()

[Markus Armbruster]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

error_propagate_prepend() "behaves like error_prepend()", and
error_prepend() uses "formatting @fmt, ... like printf()".
error_prepend() checks its format string argument, but
error_propagate_prepend() does not. Fix by addint the format
attribute to error_propagate_prepend() and error_vprepend().

This would have caught the bug fixed in the previous commit.

Missed in commit 4b5766488f "error: Fix use of error_prepend() with
&error_fatal, &error_abort".

Inspired-by: Stefan Weil <sw@weilnetz.de>
Suggested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200723171205.14949-1-philmd@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7932594dce..eaa05c4837 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -382,13 +382,15 @@ void error_propagate(Error **dst_errp, Error *local_err);
  * Please use ERRP_GUARD() and error_prepend() instead when possible.
  */
 void error_propagate_prepend(Error **dst_errp, Error *local_err,
-                             const char *fmt, ...);
+                             const char *fmt, ...)
+    GCC_FMT_ATTR(3, 4);
 
 /*
  * Prepend some text to @errp's human-readable error message.
  * The text is made by formatting @fmt, @ap like vprintf().
  */
-void error_vprepend(Error *const *errp, const char *fmt, va_list ap);
+void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);
 
 /*
  * Prepend some text to @errp's human-readable error message.
-- 
2.26.2

Re: [PULL 4/4] qapi/error: Check format string argument in error_*prepend()

[Daniel P. Berrangé]

On Fri, Jul 24, 2020 at 03:47:04PM +0200, Markus Armbruster wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> error_propagate_prepend() "behaves like error_prepend()", and
> error_prepend() uses "formatting @fmt, ... like printf()".
> error_prepend() checks its format string argument, but
> error_propagate_prepend() does not. Fix by addint the format
> attribute to error_propagate_prepend() and error_vprepend().
> 
> This would have caught the bug fixed in the previous commit.
> 
> Missed in commit 4b5766488f "error: Fix use of error_prepend() with
> &error_fatal, &error_abort".

FWIW, I'd suggest a followup patch that adds -Wsuggest-attribute=format
to CFLAGS, as after a quick hack to try a build, I think all the things
it reports are valid cases needing the format attribute.

qemu/util/error.c:62:5: warning: function ‘error_setv’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/util/error.c:133:5: warning: function ‘error_vprepend’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/util/qemu-error.c:236:5: warning: function ‘vreport’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/contrib/libvhost-user/libvhost-user.c:161:5: warning: function ‘vu_panic’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/tools/virtiofsd/fuse_log.c:20:5: warning: function ‘default_log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/tools/virtiofsd/passthrough_ll.c:2752:9: warning: function ‘log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/tools/virtiofsd/passthrough_ll.c:2754:9: warning: function ‘log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
qemu/hw/xen/xen-bus-helper.c:124:9: warning: function ‘xs_node_vscanf’ might be a candidate for ‘gnu_scanf’ format attribute [-Wsuggest-attribute=format]
qemu/disas.c:497:5: warning: function ‘plugin_printf’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 4/4] qapi/error: Check format string argument in error_*prepend()

[Philippe Mathieu-Daudé]

On 7/24/20 4:08 PM, Daniel P. Berrangé wrote:
> On Fri, Jul 24, 2020 at 03:47:04PM +0200, Markus Armbruster wrote:
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> error_propagate_prepend() "behaves like error_prepend()", and
>> error_prepend() uses "formatting @fmt, ... like printf()".
>> error_prepend() checks its format string argument, but
>> error_propagate_prepend() does not. Fix by addint the format
>> attribute to error_propagate_prepend() and error_vprepend().
>>
>> This would have caught the bug fixed in the previous commit.
>>
>> Missed in commit 4b5766488f "error: Fix use of error_prepend() with
>> &error_fatal, &error_abort".
> 
> FWIW, I'd suggest a followup patch that adds -Wsuggest-attribute=format
> to CFLAGS, as after a quick hack to try a build, I think all the things
> it reports are valid cases needing the format attribute.
> 
> qemu/util/error.c:62:5: warning: function ‘error_setv’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/util/error.c:133:5: warning: function ‘error_vprepend’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/util/qemu-error.c:236:5: warning: function ‘vreport’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/contrib/libvhost-user/libvhost-user.c:161:5: warning: function ‘vu_panic’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/tools/virtiofsd/fuse_log.c:20:5: warning: function ‘default_log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/tools/virtiofsd/passthrough_ll.c:2752:9: warning: function ‘log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/tools/virtiofsd/passthrough_ll.c:2754:9: warning: function ‘log_func’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> qemu/hw/xen/xen-bus-helper.c:124:9: warning: function ‘xs_node_vscanf’ might be a candidate for ‘gnu_scanf’ format attribute [-Wsuggest-attribute=format]
> qemu/disas.c:497:5: warning: function ‘plugin_printf’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]

I have the printf() ones ready but am waiting to be closer to 5.2.

> 
> 
> Regards,
> Daniel
> 

Re: [PULL 0/4] Error reporting patches patches for 2020-07-24

[Peter Maydell]

On Fri, 24 Jul 2020 at 14:47, Markus Armbruster <armbru@redhat.com> wrote:
>
> The following changes since commit 09e0cd773723219d21655587954da2769f64ba01:
>
>   Merge remote-tracking branch 'remotes/alistair/tags/pull-riscv-to-apply-20200722-1' into staging (2020-07-23 19:00:42 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-error-2020-07-24
>
> for you to fetch changes up to 192cf54ac5408d21c20c17b3794a632970bbb880:
>
>   qapi/error: Check format string argument in error_*prepend() (2020-07-24 15:03:09 +0200)
>
> ----------------------------------------------------------------
> Error reporting patches patches for 2020-07-24
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM