diff mbox

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

Message ID 1470224274-31522-2-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Aug. 3, 2016, 11:37 a.m. UTC
Commit 9af9e0f, 6daf194d, be62a2eb and 312fd5f got rid of a bunch, but
they keep coming back.  checkpatch.pl tries to flag them since commit
5d596c2, but it's not very good at it.  Offenders tracked down with
Coccinelle script scripts/coccinelle/err-bad-newline.cocci, an updated
version of the script from commit 312fd5f.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/mips/cps.c                            |  2 +-
 hw/nvram/fw_cfg.c                        |  2 +-
 hw/ppc/spapr_cpu_core.c                  |  2 +-
 qemu-img.c                               |  4 ++--
 scripts/coccinelle/err-bad-newline.cocci | 29 +++++++++++++++++++++++++++++
 slirp/slirp.c                            |  8 ++++----
 target-i386/kvm.c                        |  2 +-
 7 files changed, 39 insertions(+), 10 deletions(-)
 create mode 100644 scripts/coccinelle/err-bad-newline.cocci

Comments

Eric Blake Aug. 4, 2016, 8:47 p.m. UTC | #1
On 08/03/2016 05:37 AM, Markus Armbruster wrote:
> Commit 9af9e0f, 6daf194d, be62a2eb and 312fd5f got rid of a bunch, but
> they keep coming back.  checkpatch.pl tries to flag them since commit
> 5d596c2, but it's not very good at it.  Offenders tracked down with
> Coccinelle script scripts/coccinelle/err-bad-newline.cocci, an updated
> version of the script from commit 312fd5f.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/scripts/coccinelle/err-bad-newline.cocci
> @@ -0,0 +1,29 @@
> +// Error messages should not contain newlines.  This script finds
> +// messages that do.  Fixing them is manual.
> +@r@
> +expression errp, eno, cls, fmt;
> +position p;
> +@@
> +(
> +error_report(fmt, ...)@p

So you use Coccinelle to find error messages...

> +@script:python@
> +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)

...and python to then parse the format string and flag violations.
Slick.  Does it still work across formats like "foo %" PRIdMAX "\n"?

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster Aug. 5, 2016, 6:57 a.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> On 08/03/2016 05:37 AM, Markus Armbruster wrote:
>> Commit 9af9e0f, 6daf194d, be62a2eb and 312fd5f got rid of a bunch, but
>> they keep coming back.  checkpatch.pl tries to flag them since commit
>> 5d596c2, but it's not very good at it.  Offenders tracked down with
>> Coccinelle script scripts/coccinelle/err-bad-newline.cocci, an updated
>> version of the script from commit 312fd5f.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/scripts/coccinelle/err-bad-newline.cocci
>> @@ -0,0 +1,29 @@
>> +// Error messages should not contain newlines.  This script finds
>> +// messages that do.  Fixing them is manual.
>> +@r@
>> +expression errp, eno, cls, fmt;
>> +position p;
>> +@@
>> +(
>> +error_report(fmt, ...)@p
>
> So you use Coccinelle to find error messages...
>
>> +@script:python@
>> +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)
>
> ...and python to then parse the format string and flag violations.
> Slick.  Does it still work across formats like "foo %" PRIdMAX "\n"?

Testing... yes, it does.

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

Thanks!
diff mbox

Patch

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 77c6217..4ef337d 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -73,7 +73,7 @@  static void mips_cps_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < s->num_vp; i++) {
         cpu = cpu_mips_init(s->cpu_model);
         if (cpu == NULL) {
-            error_setg(errp, "%s: CPU initialization failed\n",  __func__);
+            error_setg(errp, "%s: CPU initialization failed",  __func__);
             return;
         }
 
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 2873030..4fe52f6 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -744,7 +744,7 @@  static int get_fw_cfg_order(FWCfgState *s, const char *name)
     }
 
     /* Stick unknown stuff at the end. */
-    error_report("warning: Unknown firmware file in legacy mode: %s\n", name);
+    error_report("warning: Unknown firmware file in legacy mode: %s", name);
     return FW_CFG_ORDER_OVERRIDE_LAST;
 }
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ec81ee6..23e6c82 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -241,7 +241,7 @@  void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 
     if (cc->core_id % smp_threads) {
-        error_setg(&local_err, "invalid core id %d\n", cc->core_id);
+        error_setg(&local_err, "invalid core id %d", cc->core_id);
         goto out;
     }
 
diff --git a/qemu-img.c b/qemu-img.c
index 2e40e1f..d2865a5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3490,7 +3490,7 @@  typedef struct BenchData {
 static void bench_undrained_flush_cb(void *opaque, int ret)
 {
     if (ret < 0) {
-        error_report("Failed flush request: %s\n", strerror(-ret));
+        error_report("Failed flush request: %s", strerror(-ret));
         exit(EXIT_FAILURE);
     }
 }
@@ -3501,7 +3501,7 @@  static void bench_cb(void *opaque, int ret)
     BlockAIOCB *acb;
 
     if (ret < 0) {
-        error_report("Failed request: %s\n", strerror(-ret));
+        error_report("Failed request: %s", strerror(-ret));
         exit(EXIT_FAILURE);
     }
 
diff --git a/scripts/coccinelle/err-bad-newline.cocci b/scripts/coccinelle/err-bad-newline.cocci
new file mode 100644
index 0000000..1316cc8
--- /dev/null
+++ b/scripts/coccinelle/err-bad-newline.cocci
@@ -0,0 +1,29 @@ 
+// Error messages should not contain newlines.  This script finds
+// messages that do.  Fixing them is manual.
+@r@
+expression errp, eno, cls, fmt;
+position p;
+@@
+(
+error_report(fmt, ...)@p
+|
+error_setg(errp, fmt, ...)@p
+|
+error_setg_errno(errp, eno, fmt, ...)@p
+|
+error_setg_win32(errp, eno, cls, fmt, ...)@p
+|
+error_prepend(errp, fmt, ...)@p
+|
+error_setg_file_open(errp, eno, cls, fmt, ...)@p
+|
+error_reportf_err(errp, fmt, ...)@p
+|
+error_set(errp, cls, fmt, ...)@p
+)
+@script:python@
+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)
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 7eb183d..47a1652 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1197,8 +1197,8 @@  static void slirp_socket_save(QEMUFile *f, struct socket *so)
         qemu_put_be16(f, so->so_fport);
         break;
     default:
-        error_report(
-                "so_ffamily unknown, unable to save so_faddr and so_fport\n");
+        error_report("so_ffamily unknown, unable to save so_faddr and"
+                     " so_fport");
     }
     qemu_put_be16(f, so->so_lfamily);
     switch (so->so_lfamily) {
@@ -1207,8 +1207,8 @@  static void slirp_socket_save(QEMUFile *f, struct socket *so)
         qemu_put_be16(f, so->so_lport);
         break;
     default:
-        error_report(
-                "so_ffamily unknown, unable to save so_laddr and so_lport\n");
+        error_report("so_ffamily unknown, unable to save so_laddr and"
+                     " so_lport");
     }
     qemu_put_byte(f, so->so_iptos);
     qemu_put_byte(f, so->so_emu);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 9697e16..0b2016a 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -3250,7 +3250,7 @@  int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
     if (machine_kernel_irqchip_split(ms)) {
         ret = kvm_vm_enable_cap(s, KVM_CAP_SPLIT_IRQCHIP, 0, 24);
         if (ret) {
-            error_report("Could not enable split irqchip mode: %s\n",
+            error_report("Could not enable split irqchip mode: %s",
                          strerror(-ret));
             exit(1);
         } else {