[Qemu-devel] [PATCH 0/4] Error reporting cleanups

[Qemu-devel] [PATCH 0/4] Error reporting cleanups

[Markus Armbruster]

Mostly newlines.  Making checkpatch catch them would be nice, but
that's more than I can do today.  Help welcome, of course.

I intend to take these patches through my tree.

Markus Armbruster (4):
  error: Strip trailing '\n' from error string arguments (again)
  error: Clean up errors with embedded newlines (again), part 1
  error: Clean up errors with embedded newlines (again), part 2
  hw/s390x: Rename local variables Error *l_err to just err

 block/vhdx-log.c          |  9 +++++----
 block/vmdk.c              | 13 ++++++++-----
 hw/arm/xlnx-zynqmp.c      |  2 +-
 hw/i386/kvm/pci-assign.c  | 12 ++++++------
 hw/i386/pc.c              |  4 ++--
 hw/ppc/spapr.c            |  3 ++-
 hw/s390x/ipl.c            | 12 ++++++------
 hw/s390x/s390-skeys-kvm.c |  2 +-
 hw/s390x/s390-skeys.c     | 16 ++++++++--------
 hw/s390x/sclp.c           | 14 +++++++-------
 hw/tpm/tpm_tis.c          |  2 +-
 kvm-all.c                 |  6 +++---
 migration/ram.c           |  2 +-
 migration/savevm.c        |  4 ++--
 net/vhost-user.c          |  6 +++---
 qga/commands-posix.c      |  2 +-
 target-arm/cpu.c          |  2 +-
 target-arm/machine.c      |  4 ++--
 18 files changed, 60 insertions(+), 55 deletions(-)

-- 
2.4.3

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

[Markus Armbruster]

Commit 6daf194d, be62a2eb and 312fd5f got rid of a bunch, but they
keep coming back.  Tracked down with the Coccinelle semantic patch
from commit 312fd5f.

Cc: Fam Zheng <famz@redhat.com>
Cc: Peter Crosthwaite <crosthwaitepeter@gmail.com>
Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Changchun Ouyang <changchun.ouyang@intel.com>
Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
Cc: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vmdk.c              |  4 ++--
 hw/arm/xlnx-zynqmp.c      |  2 +-
 hw/ppc/spapr.c            |  3 ++-
 hw/s390x/ipl.c            |  8 ++++----
 hw/s390x/s390-skeys-kvm.c |  2 +-
 hw/s390x/s390-skeys.c     | 16 ++++++++--------
 hw/tpm/tpm_tis.c          |  2 +-
 migration/ram.c           |  2 +-
 migration/savevm.c        |  4 ++--
 net/vhost-user.c          |  6 +++---
 qga/commands-posix.c      |  2 +-
 target-arm/cpu.c          |  2 +-
 target-arm/machine.c      |  4 ++--
 13 files changed, 29 insertions(+), 28 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 6f819e4..b4a224e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1494,8 +1494,8 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
 
     if (sector_num > bs->total_sectors) {
         error_report("Wrong offset: sector_num=0x%" PRIx64
-                " total_sectors=0x%" PRIx64 "\n",
-                sector_num, bs->total_sectors);
+                     " total_sectors=0x%" PRIx64,
+                     sector_num, bs->total_sectors);
         return -EIO;
     }
 
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 87553bb..20a3b2b 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -227,7 +227,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     }
 
     if (!s->boot_cpu_ptr) {
-        error_setg(errp, "ZynqMP Boot cpu %s not found\n", boot_cpu);
+        error_setg(errp, "ZynqMP Boot cpu %s not found", boot_cpu);
         return;
     }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6bfb908..6ba68d7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1831,7 +1831,8 @@ static void ppc_spapr_init(MachineState *machine)
         ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
 
         if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
-            error_report("Specified number of memory slots %"PRIu64" exceeds max supported %d\n",
+            error_report("Specified number of memory slots %" PRIu64
+                         " exceeds max supported %d",
                          machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
             exit(EXIT_FAILURE);
         }
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index b91fcc6..e100428 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -94,7 +94,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
 
         bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
         if (bios_filename == NULL) {
-            error_setg(&l_err, "could not find stage1 bootloader\n");
+            error_setg(&l_err, "could not find stage1 bootloader");
             goto error;
         }
 
@@ -113,7 +113,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
         g_free(bios_filename);
 
         if (bios_size == -1) {
-            error_setg(&l_err, "could not load bootloader '%s'\n", bios_name);
+            error_setg(&l_err, "could not load bootloader '%s'", bios_name);
             goto error;
         }
 
@@ -128,7 +128,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
             kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
         }
         if (kernel_size < 0) {
-            error_setg(&l_err, "could not load kernel '%s'\n", ipl->kernel);
+            error_setg(&l_err, "could not load kernel '%s'", ipl->kernel);
             goto error;
         }
         /*
@@ -156,7 +156,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
             initrd_size = load_image_targphys(ipl->initrd, initrd_offset,
                                               ram_size - initrd_offset);
             if (initrd_size == -1) {
-                error_setg(&l_err, "could not load initrd '%s'\n", ipl->initrd);
+                error_setg(&l_err, "could not load initrd '%s'", ipl->initrd);
                 goto error;
             }
 
diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
index 682949a..eaa37ba 100644
--- a/hw/s390x/s390-skeys-kvm.c
+++ b/hw/s390x/s390-skeys-kvm.c
@@ -21,7 +21,7 @@ static int kvm_s390_skeys_enabled(S390SKeysState *ss)
 
     r = skeyclass->get_skeys(ss, 0, 1, &single_key);
     if (r != 0 && r != KVM_S390_GET_SKEYS_NONE) {
-        error_report("S390_GET_KEYS error %d\n", r);
+        error_report("S390_GET_KEYS error %d", r);
     }
     return (r == 0);
 }
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 539ef6d..c9acc18 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -192,8 +192,8 @@ static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
     /* Check for uint64 overflow and access beyond end of key data */
     if (start_gfn + count > skeydev->key_count || start_gfn + count < count) {
         error_report("Error: Setting storage keys for page beyond the end "
-                "of memory: gfn=%" PRIx64 " count=%" PRId64 "\n", start_gfn,
-                count);
+                     "of memory: gfn=%" PRIx64 " count=%" PRId64,
+                     start_gfn, count);
         return -EINVAL;
     }
 
@@ -212,8 +212,8 @@ static int qemu_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
     /* Check for uint64 overflow and access beyond end of key data */
     if (start_gfn + count > skeydev->key_count || start_gfn + count < count) {
         error_report("Error: Getting storage keys for page beyond the end "
-                "of memory: gfn=%" PRIx64 " count=%" PRId64 "\n", start_gfn,
-                count);
+                     "of memory: gfn=%" PRIx64 " count=%" PRId64,
+                     start_gfn, count);
         return -EINVAL;
     }
 
@@ -257,7 +257,7 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
 
     buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
     if (!buf) {
-        error_report("storage key save could not allocate memory\n");
+        error_report("storage key save could not allocate memory");
         goto end_stream;
     }
 
@@ -277,7 +277,7 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
                  * use S390_SKEYS_SAVE_FLAG_ERROR to indicate failure to the
                  * reading side.
                  */
-                error_report("S390_GET_KEYS error %d\n", error);
+                error_report("S390_GET_KEYS error %d", error);
                 memset(buf, 0, S390_SKEYS_BUFFER_SIZE);
                 eos = S390_SKEYS_SAVE_FLAG_ERROR;
             }
@@ -315,7 +315,7 @@ static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
             uint8_t *buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
 
             if (!buf) {
-                error_report("storage key load could not allocate memory\n");
+                error_report("storage key load could not allocate memory");
                 ret = -ENOMEM;
                 break;
             }
@@ -327,7 +327,7 @@ static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
 
                 ret = skeyclass->set_skeys(ss, cur_gfn, cur_count, buf);
                 if (ret < 0) {
-                    error_report("S390_SET_KEYS error %d\n", ret);
+                    error_report("S390_SET_KEYS error %d", ret);
                     break;
                 }
                 handled_count += cur_count;
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index ff073d5..95fc66e 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -1051,7 +1051,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
 
     if (tis->irq_num > 15) {
         error_setg(errp, "tpm_tis: IRQ %d for TPM TIS is outside valid range "
-                   "of 0 to 15.\n", tis->irq_num);
+                   "of 0 to 15", tis->irq_num);
         return;
     }
 
diff --git a/migration/ram.c b/migration/ram.c
index 1eb155a..d1611cb 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2314,7 +2314,7 @@ static int ram_load_postcopy(QEMUFile *f)
             } else {
                 /* not the 1st TP within the HP */
                 if (host != (last_host + TARGET_PAGE_SIZE)) {
-                    error_report("Non-sequential target page %p/%p\n",
+                    error_report("Non-sequential target page %p/%p",
                                   host, last_host);
                     ret = -EINVAL;
                     break;
diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..f7d12e1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1563,8 +1563,8 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
     ret = qemu_get_buffer(mis->from_src_file, buffer, (int)length);
     if (ret != length) {
         g_free(buffer);
-        error_report("CMD_PACKAGED: Buffer receive fail ret=%d length=%d\n",
-                ret, length);
+        error_report("CMD_PACKAGED: Buffer receive fail ret=%d length=%d",
+                     ret, length);
         return (ret < 0) ? ret : -EAGAIN;
     }
     trace_loadvm_handle_cmd_packaged_received(ret);
diff --git a/net/vhost-user.c b/net/vhost-user.c
index b368a90..e4dd089 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -82,15 +82,15 @@ static int vhost_user_start(int queues, NetClientState *ncs[])
         options.opaque      = s->chr;
         s->vhost_net = vhost_net_init(&options);
         if (!s->vhost_net) {
-            error_report("failed to init vhost_net for queue %d\n", i);
+            error_report("failed to init vhost_net for queue %d", i);
             goto err;
         }
 
         if (i == 0) {
             max_queues = vhost_net_get_max_queues(s->vhost_net);
             if (queues > max_queues) {
-                error_report("you are asking more queues than "
-                             "supported: %d\n", max_queues);
+                error_report("you are asking more queues than supported: %d",
+                             max_queues);
                 goto err;
             }
         }
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c2ff970..ec0e997 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2247,7 +2247,7 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
          */
         if (errno != ENOENT) {
             error_setg_errno(errp, errno, "Can't open directory"
-                             "\"/sys/devices/system/memory/\"\n");
+                             "\"/sys/devices/system/memory/\"");
         }
         return NULL;
     }
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 30739fc..cac27f9 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -649,7 +649,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         uint32_t nr = cpu->pmsav7_dregion;
 
         if (nr > 0xff) {
-            error_setg(errp, "PMSAv7 MPU #regions invalid %" PRIu32 "\n", nr);
+            error_setg(errp, "PMSAv7 MPU #regions invalid %" PRIu32, nr);
             return;
         }
 
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 36a0d15..b1e1418 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -337,11 +337,11 @@ const char *gicv3_class_name(void)
         return "kvm-arm-gicv3";
 #else
         error_report("KVM GICv3 acceleration is not supported on this "
-                     "platform\n");
+                     "platform");
 #endif
     } else {
         /* TODO: Software emulation is not implemented yet */
-        error_report("KVM is currently required for GICv3 emulation\n");
+        error_report("KVM is currently required for GICv3 emulation");
     }
 
     exit(1);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Commit 6daf194d, be62a2eb and 312fd5f got rid of a bunch, but they
> keep coming back.  Tracked down with the Coccinelle semantic patch
> from commit 312fd5f.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Cc: Fam Zheng <famz@redhat.com>
> Cc: Peter Crosthwaite <crosthwaitepeter@gmail.com>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Cc: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Changchun Ouyang <changchun.ouyang@intel.com>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Cc: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/vmdk.c              |  4 ++--
>  hw/arm/xlnx-zynqmp.c      |  2 +-
>  hw/ppc/spapr.c            |  3 ++-
>  hw/s390x/ipl.c            |  8 ++++----
>  hw/s390x/s390-skeys-kvm.c |  2 +-
>  hw/s390x/s390-skeys.c     | 16 ++++++++--------
>  hw/tpm/tpm_tis.c          |  2 +-
>  migration/ram.c           |  2 +-
>  migration/savevm.c        |  4 ++--
>  net/vhost-user.c          |  6 +++---
>  qga/commands-posix.c      |  2 +-
>  target-arm/cpu.c          |  2 +-
>  target-arm/machine.c      |  4 ++--
>  13 files changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 6f819e4..b4a224e 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1494,8 +1494,8 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
>  
>      if (sector_num > bs->total_sectors) {
>          error_report("Wrong offset: sector_num=0x%" PRIx64
> -                " total_sectors=0x%" PRIx64 "\n",
> -                sector_num, bs->total_sectors);
> +                     " total_sectors=0x%" PRIx64,
> +                     sector_num, bs->total_sectors);
>          return -EIO;
>      }
>  
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 87553bb..20a3b2b 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -227,7 +227,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (!s->boot_cpu_ptr) {
> -        error_setg(errp, "ZynqMP Boot cpu %s not found\n", boot_cpu);
> +        error_setg(errp, "ZynqMP Boot cpu %s not found", boot_cpu);
>          return;
>      }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6bfb908..6ba68d7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1831,7 +1831,8 @@ static void ppc_spapr_init(MachineState *machine)
>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
>  
>          if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> -            error_report("Specified number of memory slots %"PRIu64" exceeds max supported %d\n",
> +            error_report("Specified number of memory slots %" PRIu64
> +                         " exceeds max supported %d",
>                           machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
>              exit(EXIT_FAILURE);
>          }
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index b91fcc6..e100428 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -94,7 +94,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>  
>          bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>          if (bios_filename == NULL) {
> -            error_setg(&l_err, "could not find stage1 bootloader\n");
> +            error_setg(&l_err, "could not find stage1 bootloader");
>              goto error;
>          }
>  
> @@ -113,7 +113,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>          g_free(bios_filename);
>  
>          if (bios_size == -1) {
> -            error_setg(&l_err, "could not load bootloader '%s'\n", bios_name);
> +            error_setg(&l_err, "could not load bootloader '%s'", bios_name);
>              goto error;
>          }
>  
> @@ -128,7 +128,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>              kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
>          }
>          if (kernel_size < 0) {
> -            error_setg(&l_err, "could not load kernel '%s'\n", ipl->kernel);
> +            error_setg(&l_err, "could not load kernel '%s'", ipl->kernel);
>              goto error;
>          }
>          /*
> @@ -156,7 +156,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>              initrd_size = load_image_targphys(ipl->initrd, initrd_offset,
>                                                ram_size - initrd_offset);
>              if (initrd_size == -1) {
> -                error_setg(&l_err, "could not load initrd '%s'\n", ipl->initrd);
> +                error_setg(&l_err, "could not load initrd '%s'", ipl->initrd);
>                  goto error;
>              }
>  
> diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
> index 682949a..eaa37ba 100644
> --- a/hw/s390x/s390-skeys-kvm.c
> +++ b/hw/s390x/s390-skeys-kvm.c
> @@ -21,7 +21,7 @@ static int kvm_s390_skeys_enabled(S390SKeysState *ss)
>  
>      r = skeyclass->get_skeys(ss, 0, 1, &single_key);
>      if (r != 0 && r != KVM_S390_GET_SKEYS_NONE) {
> -        error_report("S390_GET_KEYS error %d\n", r);
> +        error_report("S390_GET_KEYS error %d", r);
>      }
>      return (r == 0);
>  }
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 539ef6d..c9acc18 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -192,8 +192,8 @@ static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
>      /* Check for uint64 overflow and access beyond end of key data */
>      if (start_gfn + count > skeydev->key_count || start_gfn + count < count) {
>          error_report("Error: Setting storage keys for page beyond the end "
> -                "of memory: gfn=%" PRIx64 " count=%" PRId64 "\n", start_gfn,
> -                count);
> +                     "of memory: gfn=%" PRIx64 " count=%" PRId64,
> +                     start_gfn, count);
>          return -EINVAL;
>      }
>  
> @@ -212,8 +212,8 @@ static int qemu_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
>      /* Check for uint64 overflow and access beyond end of key data */
>      if (start_gfn + count > skeydev->key_count || start_gfn + count < count) {
>          error_report("Error: Getting storage keys for page beyond the end "
> -                "of memory: gfn=%" PRIx64 " count=%" PRId64 "\n", start_gfn,
> -                count);
> +                     "of memory: gfn=%" PRIx64 " count=%" PRId64,
> +                     start_gfn, count);
>          return -EINVAL;
>      }
>  
> @@ -257,7 +257,7 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
>  
>      buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
>      if (!buf) {
> -        error_report("storage key save could not allocate memory\n");
> +        error_report("storage key save could not allocate memory");
>          goto end_stream;
>      }
>  
> @@ -277,7 +277,7 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
>                   * use S390_SKEYS_SAVE_FLAG_ERROR to indicate failure to the
>                   * reading side.
>                   */
> -                error_report("S390_GET_KEYS error %d\n", error);
> +                error_report("S390_GET_KEYS error %d", error);
>                  memset(buf, 0, S390_SKEYS_BUFFER_SIZE);
>                  eos = S390_SKEYS_SAVE_FLAG_ERROR;
>              }
> @@ -315,7 +315,7 @@ static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
>              uint8_t *buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
>  
>              if (!buf) {
> -                error_report("storage key load could not allocate memory\n");
> +                error_report("storage key load could not allocate memory");
>                  ret = -ENOMEM;
>                  break;
>              }
> @@ -327,7 +327,7 @@ static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
>  
>                  ret = skeyclass->set_skeys(ss, cur_gfn, cur_count, buf);
>                  if (ret < 0) {
> -                    error_report("S390_SET_KEYS error %d\n", ret);
> +                    error_report("S390_SET_KEYS error %d", ret);
>                      break;
>                  }
>                  handled_count += cur_count;
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index ff073d5..95fc66e 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -1051,7 +1051,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>  
>      if (tis->irq_num > 15) {
>          error_setg(errp, "tpm_tis: IRQ %d for TPM TIS is outside valid range "
> -                   "of 0 to 15.\n", tis->irq_num);
> +                   "of 0 to 15", tis->irq_num);
>          return;
>      }
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index 1eb155a..d1611cb 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2314,7 +2314,7 @@ static int ram_load_postcopy(QEMUFile *f)
>              } else {
>                  /* not the 1st TP within the HP */
>                  if (host != (last_host + TARGET_PAGE_SIZE)) {
> -                    error_report("Non-sequential target page %p/%p\n",
> +                    error_report("Non-sequential target page %p/%p",
>                                    host, last_host);
>                      ret = -EINVAL;
>                      break;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0ad1b93..f7d12e1 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1563,8 +1563,8 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
>      ret = qemu_get_buffer(mis->from_src_file, buffer, (int)length);
>      if (ret != length) {
>          g_free(buffer);
> -        error_report("CMD_PACKAGED: Buffer receive fail ret=%d length=%d\n",
> -                ret, length);
> +        error_report("CMD_PACKAGED: Buffer receive fail ret=%d length=%d",
> +                     ret, length);
>          return (ret < 0) ? ret : -EAGAIN;
>      }
>      trace_loadvm_handle_cmd_packaged_received(ret);
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index b368a90..e4dd089 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -82,15 +82,15 @@ static int vhost_user_start(int queues, NetClientState *ncs[])
>          options.opaque      = s->chr;
>          s->vhost_net = vhost_net_init(&options);
>          if (!s->vhost_net) {
> -            error_report("failed to init vhost_net for queue %d\n", i);
> +            error_report("failed to init vhost_net for queue %d", i);
>              goto err;
>          }
>  
>          if (i == 0) {
>              max_queues = vhost_net_get_max_queues(s->vhost_net);
>              if (queues > max_queues) {
> -                error_report("you are asking more queues than "
> -                             "supported: %d\n", max_queues);
> +                error_report("you are asking more queues than supported: %d",
> +                             max_queues);
>                  goto err;
>              }
>          }
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index c2ff970..ec0e997 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2247,7 +2247,7 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>           */
>          if (errno != ENOENT) {
>              error_setg_errno(errp, errno, "Can't open directory"
> -                             "\"/sys/devices/system/memory/\"\n");
> +                             "\"/sys/devices/system/memory/\"");
>          }
>          return NULL;
>      }
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 30739fc..cac27f9 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -649,7 +649,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          uint32_t nr = cpu->pmsav7_dregion;
>  
>          if (nr > 0xff) {
> -            error_setg(errp, "PMSAv7 MPU #regions invalid %" PRIu32 "\n", nr);
> +            error_setg(errp, "PMSAv7 MPU #regions invalid %" PRIu32, nr);
>              return;
>          }
>  
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 36a0d15..b1e1418 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -337,11 +337,11 @@ const char *gicv3_class_name(void)
>          return "kvm-arm-gicv3";
>  #else
>          error_report("KVM GICv3 acceleration is not supported on this "
> -                     "platform\n");
> +                     "platform");
>  #endif
>      } else {
>          /* TODO: Software emulation is not implemented yet */
> -        error_report("KVM is currently required for GICv3 emulation\n");
> +        error_report("KVM is currently required for GICv3 emulation");
>      }
>  
>      exit(1);
> -- 
> 2.4.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)

[Cornelia Huck]

On Thu, 10 Dec 2015 18:23:49 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Commit 6daf194d, be62a2eb and 312fd5f got rid of a bunch, but they
> keep coming back.  Tracked down with the Coccinelle semantic patch
> from commit 312fd5f.
> 
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Peter Crosthwaite <crosthwaitepeter@gmail.com>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Cc: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Changchun Ouyang <changchun.ouyang@intel.com>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Cc: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/vmdk.c              |  4 ++--
>  hw/arm/xlnx-zynqmp.c      |  2 +-
>  hw/ppc/spapr.c            |  3 ++-
>  hw/s390x/ipl.c            |  8 ++++----
>  hw/s390x/s390-skeys-kvm.c |  2 +-
>  hw/s390x/s390-skeys.c     | 16 ++++++++--------
>  hw/tpm/tpm_tis.c          |  2 +-
>  migration/ram.c           |  2 +-
>  migration/savevm.c        |  4 ++--
>  net/vhost-user.c          |  6 +++---
>  qga/commands-posix.c      |  2 +-
>  target-arm/cpu.c          |  2 +-
>  target-arm/machine.c      |  4 ++--
>  13 files changed, 29 insertions(+), 28 deletions(-)
> 

s390x parts:

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Can somebody with Perl-fu extend checkpatch.pl if these keep coming
back?

Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)

[Hailiang Zhang]

On 2015/12/11 2:16, Cornelia Huck wrote:
> On Thu, 10 Dec 2015 18:23:49 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Commit 6daf194d, be62a2eb and 312fd5f got rid of a bunch, but they
>> keep coming back.  Tracked down with the Coccinelle semantic patch
>> from commit 312fd5f.
>>
>> Cc: Fam Zheng <famz@redhat.com>
>> Cc: Peter Crosthwaite <crosthwaitepeter@gmail.com>
>> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
>> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
>> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Cc: Jason J. Herne <jjherne@linux.vnet.ibm.com>
>> Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Changchun Ouyang <changchun.ouyang@intel.com>
>> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Cc: Pavel Fedin <p.fedin@samsung.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   block/vmdk.c              |  4 ++--
>>   hw/arm/xlnx-zynqmp.c      |  2 +-
>>   hw/ppc/spapr.c            |  3 ++-
>>   hw/s390x/ipl.c            |  8 ++++----
>>   hw/s390x/s390-skeys-kvm.c |  2 +-
>>   hw/s390x/s390-skeys.c     | 16 ++++++++--------
>>   hw/tpm/tpm_tis.c          |  2 +-
>>   migration/ram.c           |  2 +-
>>   migration/savevm.c        |  4 ++--
>>   net/vhost-user.c          |  6 +++---
>>   qga/commands-posix.c      |  2 +-
>>   target-arm/cpu.c          |  2 +-
>>   target-arm/machine.c      |  4 ++--
>>   13 files changed, 29 insertions(+), 28 deletions(-)
>>
>
> s390x parts:
>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>
> Can somebody with Perl-fu extend checkpatch.pl if these keep coming
> back?
>
>

Ha, that's really a good idea ...

> .
>

Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)

[Bharata B Rao]

On Thu, Dec 10, 2015 at 06:23:49PM +0100, Markus Armbruster wrote:
> Commit 6daf194d, be62a2eb and 312fd5f got rid of a bunch, but they
> keep coming back.  Tracked down with the Coccinelle semantic patch
> from commit 312fd5f.
> 
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Peter Crosthwaite <crosthwaitepeter@gmail.com>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Cc: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Cc: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Changchun Ouyang <changchun.ouyang@intel.com>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Cc: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/vmdk.c              |  4 ++--
>  hw/arm/xlnx-zynqmp.c      |  2 +-
>  hw/ppc/spapr.c            |  3 ++-

For spapr bits, Acked-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)

[Fam Zheng]

On Thu, 12/10 18:23, Markus Armbruster wrote:
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 6f819e4..b4a224e 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1494,8 +1494,8 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
>  
>      if (sector_num > bs->total_sectors) {
>          error_report("Wrong offset: sector_num=0x%" PRIx64
> -                " total_sectors=0x%" PRIx64 "\n",
> -                sector_num, bs->total_sectors);
> +                     " total_sectors=0x%" PRIx64,
> +                     sector_num, bs->total_sectors);
>          return -EIO;
>      }
>  

For VMDK change:

Acked-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1

[Markus Armbruster]

The arguments of error_report() should yield a short error string
without newlines.

A few places try to print additional help after the error message by
embedding newlines in the error string.  That's nice, but let's do it
the right way.  Commit 474c213 cleaned up some, but they keep comint
back.  Offenders tracked down with the Coccinelle semantic patch from
commit 312fd5f.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/pc.c | 4 ++--
 kvm-all.c    | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5e20e07..2c89ed1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -418,8 +418,8 @@ static void pc_cmos_init_late(void *opaque)
 
     if (state.multiple) {
         error_report("warning: multiple floppy disk controllers with "
-                     "iobase=0x3f0 have been found;\n"
-                     "the one being picked for CMOS setup might not reflect "
+                     "iobase=0x3f0 have been found");
+        error_printf("the one being picked for CMOS setup might not reflect "
                      "your intent");
     }
     pc_cmos_init_floppy(s, state.floppy);
diff --git a/kvm-all.c b/kvm-all.c
index c648b81..9a7dd21 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2020,9 +2020,9 @@ void kvm_device_access(int fd, int group, uint64_t attr,
                            write ? KVM_SET_DEVICE_ATTR : KVM_GET_DEVICE_ATTR,
                            &kvmattr);
     if (err < 0) {
-        error_report("KVM_%s_DEVICE_ATTR failed: %s\n"
-                     "Group %d attr 0x%016" PRIx64, write ? "SET" : "GET",
-                     strerror(-err), group, attr);
+        error_report("KVM_%s_DEVICE_ATTR failed: %s",
+                     write ? "SET" : "GET", strerror(-err));
+        error_printf("Group %d attr 0x%016" PRIx64, group, attr);
         abort();
     }
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1

[Laszlo Ersek]

On 12/10/15 18:23, Markus Armbruster wrote:
> The arguments of error_report() should yield a short error string
> without newlines.
> 
> A few places try to print additional help after the error message by
> embedding newlines in the error string.  That's nice, but let's do it
> the right way.  Commit 474c213 cleaned up some, but they keep comint

s/comint/coming/, you've been reading too many spy stories! :)

> back.  Offenders tracked down with the Coccinelle semantic patch from
> commit 312fd5f.

My share of the blame is b86f46132 here... But you saw that patch! ;)

Anyway, this cleanup is consistent with your advice in
<http://thread.gmane.org/gmane.comp.emulators.qemu/374410/focus=374855>.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/pc.c | 4 ++--
>  kvm-all.c    | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5e20e07..2c89ed1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -418,8 +418,8 @@ static void pc_cmos_init_late(void *opaque)
>  
>      if (state.multiple) {
>          error_report("warning: multiple floppy disk controllers with "
> -                     "iobase=0x3f0 have been found;\n"
> -                     "the one being picked for CMOS setup might not reflect "
> +                     "iobase=0x3f0 have been found");
> +        error_printf("the one being picked for CMOS setup might not reflect "
>                       "your intent");
>      }
>      pc_cmos_init_floppy(s, state.floppy);
> diff --git a/kvm-all.c b/kvm-all.c
> index c648b81..9a7dd21 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -2020,9 +2020,9 @@ void kvm_device_access(int fd, int group, uint64_t attr,
>                             write ? KVM_SET_DEVICE_ATTR : KVM_GET_DEVICE_ATTR,
>                             &kvmattr);
>      if (err < 0) {
> -        error_report("KVM_%s_DEVICE_ATTR failed: %s\n"
> -                     "Group %d attr 0x%016" PRIx64, write ? "SET" : "GET",
> -                     strerror(-err), group, attr);
> +        error_report("KVM_%s_DEVICE_ATTR failed: %s",
> +                     write ? "SET" : "GET", strerror(-err));
> +        error_printf("Group %d attr 0x%016" PRIx64, group, attr);
>          abort();
>      }
>  }
> 

Re: [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 12/10/15 18:23, Markus Armbruster wrote:
>> The arguments of error_report() should yield a short error string
>> without newlines.
>> 
>> A few places try to print additional help after the error message by
>> embedding newlines in the error string.  That's nice, but let's do it
>> the right way.  Commit 474c213 cleaned up some, but they keep comint
>
> s/comint/coming/, you've been reading too many spy stories! :)

Unfortunately, my demand for spy stories has been fully satisfied by the
news.

>> back.  Offenders tracked down with the Coccinelle semantic patch from
>> commit 312fd5f.
>
> My share of the blame is b86f46132 here... But you saw that patch! ;)

Yes, but it never made it to the front of my review queue...

> Anyway, this cleanup is consistent with your advice in
> <http://thread.gmane.org/gmane.comp.emulators.qemu/374410/focus=374855>.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

[Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2

[Markus Armbruster]

The arguments of error_setg() & friends should yield a short error
string without newlines.

A few places try to append additional help to the error message by
embedding newlines in the error string.  That's nice, but let's do it
the right way, with error_append_hint().  Offenders tracked down with
the Coccinelle semantic patch from commit 312fd5f.

Cc: Jeff Cody <jcody@redhat.com>
Cc: Fam Zheng <famz@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vhdx-log.c         |  9 +++++----
 block/vmdk.c             |  9 ++++++---
 hw/i386/kvm/pci-assign.c | 12 ++++++------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 47ae4b1..2ac8693 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
             ret = -EPERM;
             error_setg_errno(errp, EPERM,
                              "VHDX image file '%s' opened read-only, but "
-                             "contains a log that needs to be replayed.  To "
-                             "replay the log, execute:\n qemu-img check -r "
-                             "all '%s'",
-                             bs->filename, bs->filename);
+                             "contains a log that needs to be replayed",
+                             bs->filename);
+            error_append_hint(errp,  "To replay the log, run:\n"
+                              "qemu-img check -r all '%s'\n",
+                              bs->filename);
             goto exit;
         }
         /* now flush the log */
diff --git a/block/vmdk.c b/block/vmdk.c
index b4a224e..3a4c4ed 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             goto next_line;
         } else if (!strcmp(type, "FLAT")) {
             if (matches != 5 || flat_offset < 0) {
-                error_setg(errp, "Invalid extent lines: \n%s", p);
+                error_setg(errp, "Invalid extent lines");
+                error_append_hint(errp, "%s", p);
                 return -EINVAL;
             }
         } else if (!strcmp(type, "VMFS")) {
             if (matches == 4) {
                 flat_offset = 0;
             } else {
-                error_setg(errp, "Invalid extent lines:\n%s", p);
+                error_setg(errp, "Invalid extent lines");
+                error_append_hint(errp, "%s", p);
                 return -EINVAL;
             }
         } else if (matches != 4) {
-            error_setg(errp, "Invalid extent lines:\n%s", p);
+            error_setg(errp, "Invalid extent lines");
+            error_append_hint(errp, "%s", p);
             return -EINVAL;
         }
 
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 0fd6923..68622ff 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -812,8 +812,9 @@ static void assign_device(AssignedDevice *dev, Error **errp)
             char *cause;
 
             cause = assign_failed_examine(dev);
-            error_setg_errno(errp, -r, "Failed to assign device \"%s\"\n%s",
-                             dev->dev.qdev.id, cause);
+            error_setg_errno(errp, -r, "Failed to assign device \"%s\"",
+                             dev->dev.qdev.id);
+            error_append_hint(errp, "%s", cause);
             g_free(cause);
             break;
         }
@@ -912,11 +913,10 @@ retry:
             dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK;
             goto retry;
         }
-        error_setg_errno(errp, -r,
-                         "Failed to assign irq for \"%s\"\n"
-                         "Perhaps you are assigning a device "
-                         "that shares an IRQ with another device?",
+        error_setg_errno(errp, -r, "Failed to assign irq for \"%s\"",
                          dev->dev.qdev.id);
+        error_append_hint(errp, "Perhaps you are assigning a device "
+                          "that shares an IRQ with another device?");
         return r;
     }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2

[Laszlo Ersek]

On 12/10/15 18:23, Markus Armbruster wrote:
> The arguments of error_setg() & friends should yield a short error
> string without newlines.
> 
> A few places try to append additional help to the error message by
> embedding newlines in the error string.  That's nice, but let's do it
> the right way, with error_append_hint().  Offenders tracked down with
> the Coccinelle semantic patch from commit 312fd5f.
> 
> Cc: Jeff Cody <jcody@redhat.com>
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/vhdx-log.c         |  9 +++++----
>  block/vmdk.c             |  9 ++++++---
>  hw/i386/kvm/pci-assign.c | 12 ++++++------
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index 47ae4b1..2ac8693 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
>              ret = -EPERM;
>              error_setg_errno(errp, EPERM,
>                               "VHDX image file '%s' opened read-only, but "
> -                             "contains a log that needs to be replayed.  To "
> -                             "replay the log, execute:\n qemu-img check -r "
> -                             "all '%s'",
> -                             bs->filename, bs->filename);
> +                             "contains a log that needs to be replayed",
> +                             bs->filename);
> +            error_append_hint(errp,  "To replay the log, run:\n"
> +                              "qemu-img check -r all '%s'\n",
> +                              bs->filename);

This doesn't seem right. In error_report_err(), the hint is printed
("unless QMP") with an additional \n:

void error_report_err(Error *err)
{
    error_report("%s", error_get_pretty(err));
    if (err->hint) {
        error_printf_unless_qmp("%s\n", err->hint->str);
    }
    error_free(err);
}

Hence we shouldn't add the final \n to the hint.


>              goto exit;
>          }
>          /* now flush the log */
> diff --git a/block/vmdk.c b/block/vmdk.c
> index b4a224e..3a4c4ed 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>              goto next_line;
>          } else if (!strcmp(type, "FLAT")) {
>              if (matches != 5 || flat_offset < 0) {
> -                error_setg(errp, "Invalid extent lines: \n%s", p);
> +                error_setg(errp, "Invalid extent lines");
> +                error_append_hint(errp, "%s", p);

Looks good.

>                  return -EINVAL;
>              }
>          } else if (!strcmp(type, "VMFS")) {
>              if (matches == 4) {
>                  flat_offset = 0;
>              } else {
> -                error_setg(errp, "Invalid extent lines:\n%s", p);
> +                error_setg(errp, "Invalid extent lines");
> +                error_append_hint(errp, "%s", p);
>                  return -EINVAL;
>              }
>          } else if (matches != 4) {
> -            error_setg(errp, "Invalid extent lines:\n%s", p);
> +            error_setg(errp, "Invalid extent lines");
> +            error_append_hint(errp, "%s", p);
>              return -EINVAL;
>          }

These appear fine as well.


>  
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index 0fd6923..68622ff 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -812,8 +812,9 @@ static void assign_device(AssignedDevice *dev, Error **errp)
>              char *cause;
>  
>              cause = assign_failed_examine(dev);
> -            error_setg_errno(errp, -r, "Failed to assign device \"%s\"\n%s",
> -                             dev->dev.qdev.id, cause);
> +            error_setg_errno(errp, -r, "Failed to assign device \"%s\"",
> +                             dev->dev.qdev.id);
> +            error_append_hint(errp, "%s", cause);
>              g_free(cause);
>              break;
>          }
> @@ -912,11 +913,10 @@ retry:
>              dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK;
>              goto retry;
>          }
> -        error_setg_errno(errp, -r,
> -                         "Failed to assign irq for \"%s\"\n"
> -                         "Perhaps you are assigning a device "
> -                         "that shares an IRQ with another device?",
> +        error_setg_errno(errp, -r, "Failed to assign irq for \"%s\"",
>                           dev->dev.qdev.id);
> +        error_append_hint(errp, "Perhaps you are assigning a device "
> +                          "that shares an IRQ with another device?");
>          return r;
>      }
>  
> 

If you remove the extraneous \n from vhdx_parse_log(), you can add

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 12/10/15 18:23, Markus Armbruster wrote:
>> The arguments of error_setg() & friends should yield a short error
>> string without newlines.
>> 
>> A few places try to append additional help to the error message by
>> embedding newlines in the error string.  That's nice, but let's do it
>> the right way, with error_append_hint().  Offenders tracked down with
>> the Coccinelle semantic patch from commit 312fd5f.
>> 
>> Cc: Jeff Cody <jcody@redhat.com>
>> Cc: Fam Zheng <famz@redhat.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/vhdx-log.c         |  9 +++++----
>>  block/vmdk.c             |  9 ++++++---
>>  hw/i386/kvm/pci-assign.c | 12 ++++++------
>>  3 files changed, 17 insertions(+), 13 deletions(-)
>> 
>> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
>> index 47ae4b1..2ac8693 100644
>> --- a/block/vhdx-log.c
>> +++ b/block/vhdx-log.c
>> @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
>>              ret = -EPERM;
>>              error_setg_errno(errp, EPERM,
>>                               "VHDX image file '%s' opened read-only, but "
>> -                             "contains a log that needs to be replayed.  To "
>> -                             "replay the log, execute:\n qemu-img check -r "
>> -                             "all '%s'",
>> -                             bs->filename, bs->filename);
>> +                             "contains a log that needs to be replayed",
>> +                             bs->filename);
>> +            error_append_hint(errp,  "To replay the log, run:\n"
>> +                              "qemu-img check -r all '%s'\n",
>> +                              bs->filename);
>
> This doesn't seem right. In error_report_err(), the hint is printed
> ("unless QMP") with an additional \n:
>
> void error_report_err(Error *err)
> {
>     error_report("%s", error_get_pretty(err));
>     if (err->hint) {
>         error_printf_unless_qmp("%s\n", err->hint->str);
>     }
>     error_free(err);
> }
>
> Hence we shouldn't add the final \n to the hint.

You're right.

>
>>              goto exit;
>>          }
>>          /* now flush the log */
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index b4a224e..3a4c4ed 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>>              goto next_line;
>>          } else if (!strcmp(type, "FLAT")) {
>>              if (matches != 5 || flat_offset < 0) {
>> -                error_setg(errp, "Invalid extent lines: \n%s", p);
>> +                error_setg(errp, "Invalid extent lines");
>> +                error_append_hint(errp, "%s", p);
>
> Looks good.

Unless @p ends with a newline.

error_report_err() would report this error as

    [TIMESTAMP:][LOCATION: ]Invalid extent lines
    <first line that doesn't parse>
    <remaining text that may or may not parse, if any>
    <newline>

I figure this would make more sense:

    [TIMESTAMP:][LOCATION: ]Invalid extent line: <first line that doesn't parse>

Regardless, error_report_err()'s contract isn't clear on whether the
caller is supposed to end the hint with a newline or not.  It could be
made more tolerant and append a newline only when there isn't one
already.

What do you think?

>>                  return -EINVAL;
>>              }
>>          } else if (!strcmp(type, "VMFS")) {
>>              if (matches == 4) {
>>                  flat_offset = 0;
>>              } else {
>> -                error_setg(errp, "Invalid extent lines:\n%s", p);
>> +                error_setg(errp, "Invalid extent lines");
>> +                error_append_hint(errp, "%s", p);
>>                  return -EINVAL;
>>              }
>>          } else if (matches != 4) {
>> -            error_setg(errp, "Invalid extent lines:\n%s", p);
>> +            error_setg(errp, "Invalid extent lines");
>> +            error_append_hint(errp, "%s", p);
>>              return -EINVAL;
>>          }
>
> These appear fine as well.
>
>
>>  
>> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
>> index 0fd6923..68622ff 100644
>> --- a/hw/i386/kvm/pci-assign.c
>> +++ b/hw/i386/kvm/pci-assign.c
>> @@ -812,8 +812,9 @@ static void assign_device(AssignedDevice *dev, Error **errp)
>>              char *cause;
>>  
>>              cause = assign_failed_examine(dev);
>> -            error_setg_errno(errp, -r, "Failed to assign device \"%s\"\n%s",
>> -                             dev->dev.qdev.id, cause);
>> +            error_setg_errno(errp, -r, "Failed to assign device \"%s\"",
>> +                             dev->dev.qdev.id);
>> +            error_append_hint(errp, "%s", cause);
>>              g_free(cause);
>>              break;
>>          }
>> @@ -912,11 +913,10 @@ retry:
>>              dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK;
>>              goto retry;
>>          }
>> -        error_setg_errno(errp, -r,
>> -                         "Failed to assign irq for \"%s\"\n"
>> -                         "Perhaps you are assigning a device "
>> -                         "that shares an IRQ with another device?",
>> +        error_setg_errno(errp, -r, "Failed to assign irq for \"%s\"",
>>                           dev->dev.qdev.id);
>> +        error_append_hint(errp, "Perhaps you are assigning a device "
>> +                          "that shares an IRQ with another device?");
>>          return r;
>>      }
>>  
>> 
>
> If you remove the extraneous \n from vhdx_parse_log(), you can add
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2

[Fam Zheng]

On Mon, 12/14 10:42, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
> > On 12/10/15 18:23, Markus Armbruster wrote:
> >> The arguments of error_setg() & friends should yield a short error
> >> string without newlines.
> >> 
> >> A few places try to append additional help to the error message by
> >> embedding newlines in the error string.  That's nice, but let's do it
> >> the right way, with error_append_hint().  Offenders tracked down with
> >> the Coccinelle semantic patch from commit 312fd5f.
> >> 
> >> Cc: Jeff Cody <jcody@redhat.com>
> >> Cc: Fam Zheng <famz@redhat.com>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  block/vhdx-log.c         |  9 +++++----
> >>  block/vmdk.c             |  9 ++++++---
> >>  hw/i386/kvm/pci-assign.c | 12 ++++++------
> >>  3 files changed, 17 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> >> index 47ae4b1..2ac8693 100644
> >> --- a/block/vhdx-log.c
> >> +++ b/block/vhdx-log.c
> >> @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
> >>              ret = -EPERM;
> >>              error_setg_errno(errp, EPERM,
> >>                               "VHDX image file '%s' opened read-only, but "
> >> -                             "contains a log that needs to be replayed.  To "
> >> -                             "replay the log, execute:\n qemu-img check -r "
> >> -                             "all '%s'",
> >> -                             bs->filename, bs->filename);
> >> +                             "contains a log that needs to be replayed",
> >> +                             bs->filename);
> >> +            error_append_hint(errp,  "To replay the log, run:\n"
> >> +                              "qemu-img check -r all '%s'\n",
> >> +                              bs->filename);
> >
> > This doesn't seem right. In error_report_err(), the hint is printed
> > ("unless QMP") with an additional \n:
> >
> > void error_report_err(Error *err)
> > {
> >     error_report("%s", error_get_pretty(err));
> >     if (err->hint) {
> >         error_printf_unless_qmp("%s\n", err->hint->str);
> >     }
> >     error_free(err);
> > }
> >
> > Hence we shouldn't add the final \n to the hint.
> 
> You're right.
> 
> >
> >>              goto exit;
> >>          }
> >>          /* now flush the log */
> >> diff --git a/block/vmdk.c b/block/vmdk.c
> >> index b4a224e..3a4c4ed 100644
> >> --- a/block/vmdk.c
> >> +++ b/block/vmdk.c
> >> @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> >>              goto next_line;
> >>          } else if (!strcmp(type, "FLAT")) {
> >>              if (matches != 5 || flat_offset < 0) {
> >> -                error_setg(errp, "Invalid extent lines: \n%s", p);
> >> +                error_setg(errp, "Invalid extent lines");
> >> +                error_append_hint(errp, "%s", p);
> >
> > Looks good.
> 
> Unless @p ends with a newline.
> 
> error_report_err() would report this error as
> 
>     [TIMESTAMP:][LOCATION: ]Invalid extent lines
>     <first line that doesn't parse>
>     <remaining text that may or may not parse, if any>
>     <newline>
> 
> I figure this would make more sense:
> 
>     [TIMESTAMP:][LOCATION: ]Invalid extent line: <first line that doesn't parse>

Yes, it's better in every way!

Fam

> 
> Regardless, error_report_err()'s contract isn't clear on whether the
> caller is supposed to end the hint with a newline or not.  It could be
> made more tolerant and append a newline only when there isn't one
> already.
> 
> What do you think?

Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 12/14 10:42, Markus Armbruster wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>> 
>> > On 12/10/15 18:23, Markus Armbruster wrote:
>> >> The arguments of error_setg() & friends should yield a short error
>> >> string without newlines.
>> >> 
>> >> A few places try to append additional help to the error message by
>> >> embedding newlines in the error string.  That's nice, but let's do it
>> >> the right way, with error_append_hint().  Offenders tracked down with
>> >> the Coccinelle semantic patch from commit 312fd5f.
>> >> 
>> >> Cc: Jeff Cody <jcody@redhat.com>
>> >> Cc: Fam Zheng <famz@redhat.com>
>> >> Cc: Laszlo Ersek <lersek@redhat.com>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  block/vhdx-log.c         |  9 +++++----
>> >>  block/vmdk.c             |  9 ++++++---
>> >>  hw/i386/kvm/pci-assign.c | 12 ++++++------
>> >>  3 files changed, 17 insertions(+), 13 deletions(-)
>> >> 
>> >> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
>> >> index 47ae4b1..2ac8693 100644
>> >> --- a/block/vhdx-log.c
>> >> +++ b/block/vhdx-log.c
>> >> @@ -786,10 +786,11 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
>> >>              ret = -EPERM;
>> >>              error_setg_errno(errp, EPERM,
>> >>                               "VHDX image file '%s' opened read-only, but "
>> >> -                             "contains a log that needs to be replayed.  To "
>> >> -                             "replay the log, execute:\n qemu-img check -r "
>> >> -                             "all '%s'",
>> >> -                             bs->filename, bs->filename);
>> >> +                             "contains a log that needs to be replayed",
>> >> +                             bs->filename);
>> >> +            error_append_hint(errp,  "To replay the log, run:\n"
>> >> +                              "qemu-img check -r all '%s'\n",
>> >> +                              bs->filename);
>> >
>> > This doesn't seem right. In error_report_err(), the hint is printed
>> > ("unless QMP") with an additional \n:
>> >
>> > void error_report_err(Error *err)
>> > {
>> >     error_report("%s", error_get_pretty(err));
>> >     if (err->hint) {
>> >         error_printf_unless_qmp("%s\n", err->hint->str);
>> >     }
>> >     error_free(err);
>> > }
>> >
>> > Hence we shouldn't add the final \n to the hint.
>> 
>> You're right.
>> 
>> >
>> >>              goto exit;
>> >>          }
>> >>          /* now flush the log */
>> >> diff --git a/block/vmdk.c b/block/vmdk.c
>> >> index b4a224e..3a4c4ed 100644
>> >> --- a/block/vmdk.c
>> >> +++ b/block/vmdk.c
>> >> @@ -794,18 +794,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>> >>              goto next_line;
>> >>          } else if (!strcmp(type, "FLAT")) {
>> >>              if (matches != 5 || flat_offset < 0) {
>> >> -                error_setg(errp, "Invalid extent lines: \n%s", p);
>> >> +                error_setg(errp, "Invalid extent lines");
>> >> +                error_append_hint(errp, "%s", p);
>> >
>> > Looks good.
>> 
>> Unless @p ends with a newline.
>> 
>> error_report_err() would report this error as
>> 
>>     [TIMESTAMP:][LOCATION: ]Invalid extent lines
>>     <first line that doesn't parse>
>>     <remaining text that may or may not parse, if any>
>>     <newline>
>> 
>> I figure this would make more sense:
>> 
>>     [TIMESTAMP:][LOCATION: ]Invalid extent line: <first line that
>> doesn't parse>
>
> Yes, it's better in every way!

Okay, I'll try to do this for v2.

[...]

[Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err

[Markus Armbruster]

Let's follow established naming practice here as well.

Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/s390x/ipl.c  | 12 ++++++------
 hw/s390x/sclp.c | 14 +++++++-------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index e100428..9c01be5 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -76,7 +76,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
     S390IPLState *ipl = S390_IPL(dev);
     uint64_t pentry = KERN_IMAGE_START;
     int kernel_size;
-    Error *l_err = NULL;
+    Error *err = NULL;
 
     int bios_size;
     char *bios_filename;
@@ -94,7 +94,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
 
         bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
         if (bios_filename == NULL) {
-            error_setg(&l_err, "could not find stage1 bootloader");
+            error_setg(&err, "could not find stage1 bootloader");
             goto error;
         }
 
@@ -113,7 +113,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
         g_free(bios_filename);
 
         if (bios_size == -1) {
-            error_setg(&l_err, "could not load bootloader '%s'", bios_name);
+            error_setg(&err, "could not load bootloader '%s'", bios_name);
             goto error;
         }
 
@@ -128,7 +128,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
             kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
         }
         if (kernel_size < 0) {
-            error_setg(&l_err, "could not load kernel '%s'", ipl->kernel);
+            error_setg(&err, "could not load kernel '%s'", ipl->kernel);
             goto error;
         }
         /*
@@ -156,7 +156,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
             initrd_size = load_image_targphys(ipl->initrd, initrd_offset,
                                               ram_size - initrd_offset);
             if (initrd_size == -1) {
-                error_setg(&l_err, "could not load initrd '%s'", ipl->initrd);
+                error_setg(&err, "could not load initrd '%s'", ipl->initrd);
                 goto error;
             }
 
@@ -170,7 +170,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
     }
     qemu_register_reset(qdev_reset_all_fn, dev);
 error:
-    error_propagate(errp, l_err);
+    error_propagate(errp, err);
 }
 
 static Property s390_ipl_properties[] = {
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index a061b49..9a117c9 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -456,29 +456,29 @@ static void sclp_realize(DeviceState *dev, Error **errp)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
     SCLPDevice *sclp = SCLP(dev);
-    Error *l_err = NULL;
+    Error *err = NULL;
     uint64_t hw_limit;
     int ret;
 
     object_property_set_bool(OBJECT(sclp->event_facility), true, "realized",
-                             &l_err);
-    if (l_err) {
+                             &err);
+    if (err) {
         goto error;
     }
 
     ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
     if (ret == -E2BIG) {
-        error_setg(&l_err, "qemu: host supports a maximum of %" PRIu64 " GB",
+        error_setg(&err, "qemu: host supports a maximum of %" PRIu64 " GB",
                    hw_limit >> 30);
         goto error;
     } else if (ret) {
-        error_setg(&l_err, "qemu: setting the guest size failed");
+        error_setg(&err, "qemu: setting the guest size failed");
         goto error;
     }
     return;
 error:
-    assert(l_err);
-    error_propagate(errp, l_err);
+    assert(err);
+    error_propagate(errp, err);
 }
 
 static void sclp_memory_init(SCLPDevice *sclp)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err

[Cornelia Huck]

On Thu, 10 Dec 2015 18:23:52 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Let's follow established naming practice here as well.
> 
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/s390x/ipl.c  | 12 ++++++------
>  hw/s390x/sclp.c | 14 +++++++-------
>  2 files changed, 13 insertions(+), 13 deletions(-)

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err

[David Hildenbrand]

> 
>  static Property s390_ipl_properties[] = {
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index a061b49..9a117c9 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -456,29 +456,29 @@ static void sclp_realize(DeviceState *dev, Error **errp)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
>      SCLPDevice *sclp = SCLP(dev);
> -    Error *l_err = NULL;
> +    Error *err = NULL;
>      uint64_t hw_limit;
>      int ret;
> 
>      object_property_set_bool(OBJECT(sclp->event_facility), true, "realized",
> -                             &l_err);
> -    if (l_err) {
> +                             &err);
> +    if (err) {
>          goto error;
>      }
> 
>      ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
>      if (ret == -E2BIG) {
> -        error_setg(&l_err, "qemu: host supports a maximum of %" PRIu64 " GB",
> +        error_setg(&err, "qemu: host supports a maximum of %" PRIu64 " GB",
>                     hw_limit >> 30);
>          goto error;
>      } else if (ret) {
> -        error_setg(&l_err, "qemu: setting the guest size failed");
> +        error_setg(&err, "qemu: setting the guest size failed");
>          goto error;
>      }
>      return;
>  error:
> -    assert(l_err);
> -    error_propagate(errp, l_err);
> +    assert(err);

We could also get rid of that assert and remove the return; above (naming the
label "out").

> +    error_propagate(errp, err);
>  }
> 
>  static void sclp_memory_init(SCLPDevice *sclp)

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

David

Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err

[Markus Armbruster]

David Hildenbrand <dahi@linux.vnet.ibm.com> writes:

>> 
>>  static Property s390_ipl_properties[] = {
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index a061b49..9a117c9 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -456,29 +456,29 @@ static void sclp_realize(DeviceState *dev, Error **errp)
>>  {
>>      MachineState *machine = MACHINE(qdev_get_machine());
>>      SCLPDevice *sclp = SCLP(dev);
>> -    Error *l_err = NULL;
>> +    Error *err = NULL;
>>      uint64_t hw_limit;
>>      int ret;
>> 
>>      object_property_set_bool(OBJECT(sclp->event_facility), true, "realized",
>> -                             &l_err);
>> -    if (l_err) {
>> +                             &err);
>> +    if (err) {
>>          goto error;
>>      }
>> 
>>      ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
>>      if (ret == -E2BIG) {
>> -        error_setg(&l_err, "qemu: host supports a maximum of %" PRIu64 " GB",
>> +        error_setg(&err, "qemu: host supports a maximum of %" PRIu64 " GB",
>>                     hw_limit >> 30);
>>          goto error;
>>      } else if (ret) {
>> -        error_setg(&l_err, "qemu: setting the guest size failed");
>> +        error_setg(&err, "qemu: setting the guest size failed");
>>          goto error;
>>      }
>>      return;
>>  error:
>> -    assert(l_err);
>> -    error_propagate(errp, l_err);
>> +    assert(err);
>
> We could also get rid of that assert and remove the return; above (naming the
> label "out").

Separate patch, like this:

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 9a117c9..74f2b40 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -463,21 +463,18 @@ static void sclp_realize(DeviceState *dev, Error **errp)
     object_property_set_bool(OBJECT(sclp->event_facility), true, "realized",
                              &err);
     if (err) {
-        goto error;
+        goto out;
     }
 
     ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
     if (ret == -E2BIG) {
         error_setg(&err, "qemu: host supports a maximum of %" PRIu64 " GB",
                    hw_limit >> 30);
-        goto error;
     } else if (ret) {
         error_setg(&err, "qemu: setting the guest size failed");
-        goto error;
     }
-    return;
-error:
-    assert(err);
+
+out:
     error_propagate(errp, err);
 }
 
If you like it, I'll stick it into my respin.

>> +    error_propagate(errp, err);
>>  }
>> 
>>  static void sclp_memory_init(SCLPDevice *sclp)
>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

Thanks!

Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err

[Cornelia Huck]

On Mon, 14 Dec 2015 10:59:36 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Separate patch, like this:
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 9a117c9..74f2b40 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -463,21 +463,18 @@ static void sclp_realize(DeviceState *dev, Error **errp)
>      object_property_set_bool(OBJECT(sclp->event_facility), true, "realized",
>                               &err);
>      if (err) {
> -        goto error;
> +        goto out;
>      }
> 
>      ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
>      if (ret == -E2BIG) {
>          error_setg(&err, "qemu: host supports a maximum of %" PRIu64 " GB",
>                     hw_limit >> 30);
> -        goto error;
>      } else if (ret) {
>          error_setg(&err, "qemu: setting the guest size failed");
> -        goto error;
>      }
> -    return;
> -error:
> -    assert(err);
> +
> +out:
>      error_propagate(errp, err);
>  }
> 
> If you like it, I'll stick it into my respin.

Not speaking for David, but this would get my Ack.

Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err

[David Hildenbrand]

> On Mon, 14 Dec 2015 10:59:36 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Separate patch, like this:
> > 
> > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> > index 9a117c9..74f2b40 100644
> > --- a/hw/s390x/sclp.c
> > +++ b/hw/s390x/sclp.c
> > @@ -463,21 +463,18 @@ static void sclp_realize(DeviceState *dev, Error **errp)
> >      object_property_set_bool(OBJECT(sclp->event_facility), true, "realized",
> >                               &err);
> >      if (err) {
> > -        goto error;
> > +        goto out;
> >      }
> > 
> >      ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
> >      if (ret == -E2BIG) {
> >          error_setg(&err, "qemu: host supports a maximum of %" PRIu64 " GB",
> >                     hw_limit >> 30);
> > -        goto error;
> >      } else if (ret) {
> >          error_setg(&err, "qemu: setting the guest size failed");
> > -        goto error;
> >      }
> > -    return;
> > -error:
> > -    assert(err);
> > +
> > +out:
> >      error_propagate(errp, err);
> >  }
> > 
> > If you like it, I'll stick it into my respin.
> 
> Not speaking for David, but this would get my Ack.

Yes, this looks good to me!

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

Thanks!

David

[Qemu-devel] [PATCH 0/4] error: Newline fixes

[Markus Armbruster]

Markus Armbruster (4):
  error: Strip trailing '\n' from error string arguments (again)
  checkpatch: Fix newline detection in error_setg() & friends
  vfio: Use error_report() instead of error_printf() for errors
  error: Fix error_printf() calls lacking newlines

 hw/i386/pc.c                             |  2 +-
 hw/mips/cps.c                            |  2 +-
 hw/nvram/fw_cfg.c                        |  2 +-
 hw/ppc/spapr_cpu_core.c                  |  2 +-
 hw/vfio/platform.c                       |  2 +-
 kvm-all.c                                |  2 +-
 qemu-img.c                               |  4 ++--
 scripts/checkpatch.pl                    |  5 ++++-
 scripts/coccinelle/err-bad-newline.cocci | 29 +++++++++++++++++++++++++++++
 slirp/slirp.c                            |  8 ++++----
 target-i386/kvm.c                        |  2 +-
 ui/vnc.c                                 |  2 +-
 12 files changed, 47 insertions(+), 15 deletions(-)
 create mode 100644 scripts/coccinelle/err-bad-newline.cocci

-- 
2.5.5

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

[Markus Armbruster]

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

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

Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)

[Markus Armbruster]

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!