* [Qemu-devel] [PATCH 0/4] Error reporting cleanups
@ 2015-12-10 17:23 Markus Armbruster
2015-12-10 17:23 ` [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
` (3 more replies)
0 siblings, 4 replies; 21+ messages in thread
From: Markus Armbruster @ 2015-12-10 17:23 UTC (permalink / raw)
To: qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)
2015-12-10 17:23 [Qemu-devel] [PATCH 0/4] Error reporting cleanups Markus Armbruster
@ 2015-12-10 17:23 ` Markus Armbruster
2015-12-10 17:31 ` Dr. David Alan Gilbert
` (3 more replies)
2015-12-10 17:23 ` [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1 Markus Armbruster
` (2 subsequent siblings)
3 siblings, 4 replies; 21+ messages in thread
From: Markus Armbruster @ 2015-12-10 17:23 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, zhanghailiang, Stefan Berger, Pavel Fedin,
Dr. David Alan Gilbert, Dominik Dingel, David Hildenbrand,
Peter Crosthwaite, Jason J. Herne, Bharata B Rao,
Changchun Ouyang
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1
2015-12-10 17:23 [Qemu-devel] [PATCH 0/4] Error reporting cleanups Markus Armbruster
2015-12-10 17:23 ` [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
@ 2015-12-10 17:23 ` Markus Armbruster
2015-12-10 17:36 ` Laszlo Ersek
2015-12-10 17:23 ` [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2 Markus Armbruster
2015-12-10 17:23 ` [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
3 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2015-12-10 17:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Fedin, Laszlo Ersek
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2
2015-12-10 17:23 [Qemu-devel] [PATCH 0/4] Error reporting cleanups Markus Armbruster
2015-12-10 17:23 ` [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
2015-12-10 17:23 ` [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1 Markus Armbruster
@ 2015-12-10 17:23 ` Markus Armbruster
2015-12-10 17:48 ` Laszlo Ersek
2015-12-10 17:23 ` [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
3 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2015-12-10 17:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Laszlo Ersek, Jeff Cody, Fam Zheng
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err
2015-12-10 17:23 [Qemu-devel] [PATCH 0/4] Error reporting cleanups Markus Armbruster
` (2 preceding siblings ...)
2015-12-10 17:23 ` [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2 Markus Armbruster
@ 2015-12-10 17:23 ` Markus Armbruster
2015-12-10 18:18 ` Cornelia Huck
2015-12-11 8:07 ` David Hildenbrand
3 siblings, 2 replies; 21+ messages in thread
From: Markus Armbruster @ 2015-12-10 17:23 UTC (permalink / raw)
To: qemu-devel; +Cc: David Hildenbrand
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)
2015-12-10 17:23 ` [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
@ 2015-12-10 17:31 ` Dr. David Alan Gilbert
2015-12-10 18:16 ` Cornelia Huck
` (2 subsequent siblings)
3 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2015-12-10 17:31 UTC (permalink / raw)
To: Markus Armbruster
Cc: Fam Zheng, zhanghailiang, Stefan Berger, Pavel Fedin, qemu-devel,
Dominik Dingel, David Hildenbrand, Peter Crosthwaite,
Jason J. Herne, Bharata B Rao, Changchun Ouyang
* 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1
2015-12-10 17:23 ` [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1 Markus Armbruster
@ 2015-12-10 17:36 ` Laszlo Ersek
2015-12-14 9:27 ` Markus Armbruster
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2015-12-10 17:36 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Pavel Fedin
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();
> }
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2
2015-12-10 17:23 ` [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2 Markus Armbruster
@ 2015-12-10 17:48 ` Laszlo Ersek
2015-12-14 9:42 ` Markus Armbruster
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2015-12-10 17:48 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Jeff Cody, Fam Zheng
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)
2015-12-10 17:23 ` [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
2015-12-10 17:31 ` Dr. David Alan Gilbert
@ 2015-12-10 18:16 ` Cornelia Huck
2015-12-11 9:01 ` Hailiang Zhang
2015-12-11 3:09 ` Bharata B Rao
2015-12-11 5:26 ` Fam Zheng
3 siblings, 1 reply; 21+ messages in thread
From: Cornelia Huck @ 2015-12-10 18:16 UTC (permalink / raw)
To: Markus Armbruster
Cc: Fam Zheng, zhanghailiang, Stefan Berger, Pavel Fedin, qemu-devel,
Dominik Dingel, Dr. David Alan Gilbert, David Hildenbrand,
Peter Crosthwaite, Jason J. Herne, Bharata B Rao,
Changchun Ouyang
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?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err
2015-12-10 17:23 ` [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
@ 2015-12-10 18:18 ` Cornelia Huck
2015-12-11 8:07 ` David Hildenbrand
1 sibling, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2015-12-10 18:18 UTC (permalink / raw)
To: Markus Armbruster; +Cc: David Hildenbrand, qemu-devel
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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)
2015-12-10 17:23 ` [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
2015-12-10 17:31 ` Dr. David Alan Gilbert
2015-12-10 18:16 ` Cornelia Huck
@ 2015-12-11 3:09 ` Bharata B Rao
2015-12-11 5:26 ` Fam Zheng
3 siblings, 0 replies; 21+ messages in thread
From: Bharata B Rao @ 2015-12-11 3:09 UTC (permalink / raw)
To: Markus Armbruster
Cc: Fam Zheng, zhanghailiang, Stefan Berger, Pavel Fedin, qemu-devel,
Dominik Dingel, Dr. David Alan Gilbert, David Hildenbrand,
Peter Crosthwaite, Jason J. Herne, Changchun Ouyang
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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)
2015-12-10 17:23 ` [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
` (2 preceding siblings ...)
2015-12-11 3:09 ` Bharata B Rao
@ 2015-12-11 5:26 ` Fam Zheng
3 siblings, 0 replies; 21+ messages in thread
From: Fam Zheng @ 2015-12-11 5:26 UTC (permalink / raw)
To: Markus Armbruster
Cc: zhanghailiang, Stefan Berger, Pavel Fedin, qemu-devel,
Dominik Dingel, Dr. David Alan Gilbert, David Hildenbrand,
Peter Crosthwaite, Jason J. Herne, Bharata B Rao,
Changchun Ouyang
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>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err
2015-12-10 17:23 ` [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
2015-12-10 18:18 ` Cornelia Huck
@ 2015-12-11 8:07 ` David Hildenbrand
2015-12-14 9:59 ` Markus Armbruster
1 sibling, 1 reply; 21+ messages in thread
From: David Hildenbrand @ 2015-12-11 8:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
>
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again)
2015-12-10 18:16 ` Cornelia Huck
@ 2015-12-11 9:01 ` Hailiang Zhang
0 siblings, 0 replies; 21+ messages in thread
From: Hailiang Zhang @ 2015-12-11 9:01 UTC (permalink / raw)
To: Cornelia Huck, Markus Armbruster
Cc: Fam Zheng, Stefan Berger, qemu-devel, Pavel Fedin,
peter.huangpeng, Dominik Dingel, Dr. David Alan Gilbert,
David Hildenbrand, Peter Crosthwaite, Jason J. Herne,
Bharata B Rao, Changchun Ouyang
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 ...
> .
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1
2015-12-10 17:36 ` Laszlo Ersek
@ 2015-12-14 9:27 ` Markus Armbruster
0 siblings, 0 replies; 21+ messages in thread
From: Markus Armbruster @ 2015-12-14 9:27 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Pavel Fedin, qemu-devel
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!
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2
2015-12-10 17:48 ` Laszlo Ersek
@ 2015-12-14 9:42 ` Markus Armbruster
2015-12-15 2:03 ` Fam Zheng
0 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2015-12-14 9:42 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Jeff Cody, Fam Zheng, qemu-devel
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!
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err
2015-12-11 8:07 ` David Hildenbrand
@ 2015-12-14 9:59 ` Markus Armbruster
2015-12-14 10:15 ` Cornelia Huck
0 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2015-12-14 9:59 UTC (permalink / raw)
To: David Hildenbrand; +Cc: qemu-devel
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!
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err
2015-12-14 9:59 ` Markus Armbruster
@ 2015-12-14 10:15 ` Cornelia Huck
2015-12-14 10:28 ` David Hildenbrand
0 siblings, 1 reply; 21+ messages in thread
From: Cornelia Huck @ 2015-12-14 10:15 UTC (permalink / raw)
To: Markus Armbruster; +Cc: David Hildenbrand, qemu-devel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err
2015-12-14 10:15 ` Cornelia Huck
@ 2015-12-14 10:28 ` David Hildenbrand
0 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2015-12-14 10:28 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Markus Armbruster, qemu-devel
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2
2015-12-14 9:42 ` Markus Armbruster
@ 2015-12-15 2:03 ` Fam Zheng
2015-12-15 7:59 ` Markus Armbruster
0 siblings, 1 reply; 21+ messages in thread
From: Fam Zheng @ 2015-12-15 2:03 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Jeff Cody, Laszlo Ersek, qemu-devel
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?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2
2015-12-15 2:03 ` Fam Zheng
@ 2015-12-15 7:59 ` Markus Armbruster
0 siblings, 0 replies; 21+ messages in thread
From: Markus Armbruster @ 2015-12-15 7:59 UTC (permalink / raw)
To: Fam Zheng; +Cc: Jeff Cody, Laszlo Ersek, qemu-devel
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.
[...]
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2015-12-15 7:59 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-10 17:23 [Qemu-devel] [PATCH 0/4] Error reporting cleanups Markus Armbruster
2015-12-10 17:23 ` [Qemu-devel] [PATCH 1/4] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
2015-12-10 17:31 ` Dr. David Alan Gilbert
2015-12-10 18:16 ` Cornelia Huck
2015-12-11 9:01 ` Hailiang Zhang
2015-12-11 3:09 ` Bharata B Rao
2015-12-11 5:26 ` Fam Zheng
2015-12-10 17:23 ` [Qemu-devel] [PATCH 2/4] error: Clean up errors with embedded newlines (again), part 1 Markus Armbruster
2015-12-10 17:36 ` Laszlo Ersek
2015-12-14 9:27 ` Markus Armbruster
2015-12-10 17:23 ` [Qemu-devel] [PATCH 3/4] error: Clean up errors with embedded newlines (again), part 2 Markus Armbruster
2015-12-10 17:48 ` Laszlo Ersek
2015-12-14 9:42 ` Markus Armbruster
2015-12-15 2:03 ` Fam Zheng
2015-12-15 7:59 ` Markus Armbruster
2015-12-10 17:23 ` [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
2015-12-10 18:18 ` Cornelia Huck
2015-12-11 8:07 ` David Hildenbrand
2015-12-14 9:59 ` Markus Armbruster
2015-12-14 10:15 ` Cornelia Huck
2015-12-14 10:28 ` David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.