* [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes
@ 2015-12-17 16:49 Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report() Markus Armbruster
` (23 more replies)
0 siblings, 24 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Two topics:
1. New in v2: Avoid error_get_pretty() when all you want is extend an
error message. Doing that with error_get_pretty() loses the error
hint. Provide that don't lose it, and are more convenient to use.
2. Newlines in error messages. Jason Herne's "[PATCH v2] checkpatch:
Detect newlines in error_report and other error functions" will catch
at least some instances of this kind of mistake in the future.
Based on my "[PATCH v3 00/13] Clean up some hw_error() misuse".
I intend to take these patches through my tree.
v2:
* 19 new patches...
* PATCH 1 became PATCH 16
* PATCH 2 became PATCH 21, with commit message typo fixed [László]
* PATCH 3 split into PATCH 17-20, partly rewritten
* PATCH 4 became PATCH 22
Markus Armbruster (23):
qemu-nbd: Replace BSDism <err.h> by error_report()
error: Use error_report_err() where appropriate (again)
error: Use error_report_err() instead of monitor_printf()
error: Use error_report_err() instead of ad hoc prints
error: Improve documentation around error_append_hint()
block: Clean up "Could not create temporary overlay" error message
qemu-nbd: Clean up "Failed to load snapshot" error message
test-throttle: Simplify qemu_init_main_loop() error handling
error: New error_prepend(), error_reportf_err()
error: Don't decorate original error message when adding to it
error: Use error_reportf_err() where it makes obvious sense
error: Use error_prepend() where it makes obvious sense
spapr: Use error_reportf_err()
migration: Use error_reportf_err() instead of monitor_printf()
qemu-io qemu-nbd: Use error_report() etc. instead of fprintf()
error: Strip trailing '\n' from error string arguments (again)
vmdk: Clean up control flow in vmdk_parse_extents() a bit
vmdk: Clean up "Invalid extent lines" error message
pci-assign: Clean up "Failed to assign" error messages
vhdx: Fix "log that needs to be replayed" error message
error: Clean up errors with embedded newlines (again)
hw/s390x: Rename local variables Error *l_err to just err
s390/sclp: Simplify control flow in sclp_realize()
arch_init.c | 4 +-
block.c | 20 +++----
block/qcow2.c | 5 +-
block/qed.c | 5 +-
block/sheepdog.c | 8 +--
block/vhdx-log.c | 13 ++--
block/vmdk.c | 49 +++++++++-------
blockdev.c | 11 ++--
contrib/ivshmem-server/main.c | 4 +-
hmp.c | 29 +++------
hw/arm/cubieboard.c | 9 ++-
hw/arm/digic_boards.c | 3 +-
hw/arm/imx25_pdk.c | 2 +-
hw/arm/kzm.c | 2 +-
hw/arm/netduino2.c | 2 +-
hw/arm/xlnx-ep108.c | 2 +-
hw/arm/xlnx-zynqmp.c | 2 +-
hw/block/dataplane/virtio-blk.c | 8 +--
hw/core/qdev-properties.c | 6 +-
hw/core/qdev.c | 5 +-
hw/i386/kvm/pci-assign.c | 16 ++---
hw/i386/pc.c | 9 ++-
hw/ppc/e500.c | 4 +-
hw/ppc/spapr.c | 10 ++--
hw/ppc/spapr_drc.c | 6 +-
hw/s390x/ipl.c | 12 ++--
hw/s390x/s390-skeys-kvm.c | 2 +-
hw/s390x/s390-skeys.c | 19 +++---
hw/s390x/sclp.c | 21 +++----
hw/scsi/vhost-scsi.c | 6 +-
hw/tpm/tpm_tis.c | 2 +-
hw/usb/bus.c | 11 ++--
include/qapi/error.h | 50 ++++++++++++++--
kvm-all.c | 6 +-
migration/ram.c | 2 +-
migration/savevm.c | 21 +++----
monitor.c | 6 +-
net/vhost-user.c | 6 +-
qdev-monitor.c | 3 +-
qemu-img.c | 33 ++++-------
qemu-io.c | 8 +--
qemu-nbd.c | 127 ++++++++++++++++++++++++----------------
qga/commands-posix.c | 2 +-
replay/replay.c | 3 +-
target-arm/cpu.c | 2 +-
target-arm/machine.c | 4 +-
tests/qemu-iotests/059.out | 12 ++--
tests/qemu-iotests/060.out | 2 +-
tests/qemu-iotests/069.out | 2 +-
tests/qemu-iotests/070.out | 5 +-
tests/qemu-iotests/075.out | 14 ++---
tests/qemu-iotests/076.out | 6 +-
tests/qemu-iotests/078.out | 12 ++--
tests/qemu-iotests/080.out | 36 ++++++------
tests/qemu-iotests/083.out | 34 +++++------
tests/qemu-iotests/088.out | 12 ++--
tests/qemu-iotests/092.out | 24 ++++----
tests/qemu-iotests/103.out | 8 +--
tests/qemu-iotests/114.out | 2 +-
tests/qemu-iotests/116.out | 14 ++---
tests/qemu-iotests/131.out | 2 +-
tests/test-aio.c | 4 +-
tests/test-thread-pool.c | 4 +-
tests/test-throttle.c | 15 +----
ui/vnc.c | 4 +-
util/error.c | 35 ++++++++++-
vl.c | 8 +--
67 files changed, 431 insertions(+), 404 deletions(-)
--
2.4.3
^ permalink raw reply [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report()
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 17:17 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 02/23] error: Use error_report_err() where appropriate (again) Markus Armbruster
` (22 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Coccinelle semantic patch
@@
expression E;
expression list ARGS;
@@
- errx(E, ARGS);
+ error_report(ARGS);
+ exit(E);
@@
expression E, FMT;
expression list ARGS;
@@
- err(E, FMT, ARGS);
+ error_report(FMT /*": %s"*/, ARGS, strerror(errno));
+ exit(E);
followed by a replace of '"/*": %s"*/' by ' : %s"'.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-nbd.c | 119 ++++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 75 insertions(+), 44 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 3afec76..0d47f14 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -30,7 +30,6 @@
#include <stdarg.h>
#include <stdio.h>
#include <getopt.h>
-#include <err.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
@@ -158,7 +157,8 @@ static int find_partition(BlockBackend *blk, int partition,
if ((ret = blk_read(blk, 0, data, 1)) < 0) {
errno = -ret;
- err(EXIT_FAILURE, "error while reading");
+ error_report("error while reading: %s", strerror(errno));
+ exit(EXIT_FAILURE);
}
if (data[510] != 0x55 || data[511] != 0xaa) {
@@ -179,7 +179,8 @@ static int find_partition(BlockBackend *blk, int partition,
if ((ret = blk_read(blk, mbr[i].start_sector_abs, data1, 1)) < 0) {
errno = -ret;
- err(EXIT_FAILURE, "error while reading");
+ error_report("error while reading: %s", strerror(errno));
+ exit(EXIT_FAILURE);
}
for (j = 0; j < 4; j++) {
@@ -454,16 +455,19 @@ int main(int argc, char **argv)
/* fallthrough */
case QEMU_NBD_OPT_CACHE:
if (seen_cache) {
- errx(EXIT_FAILURE, "-n and --cache can only be specified once");
+ error_report("-n and --cache can only be specified once");
+ exit(EXIT_FAILURE);
}
seen_cache = true;
if (bdrv_parse_cache_flags(optarg, &flags) == -1) {
- errx(EXIT_FAILURE, "Invalid cache mode `%s'", optarg);
+ error_report("Invalid cache mode `%s'", optarg);
+ exit(EXIT_FAILURE);
}
break;
case QEMU_NBD_OPT_AIO:
if (seen_aio) {
- errx(EXIT_FAILURE, "--aio can only be specified once");
+ error_report("--aio can only be specified once");
+ exit(EXIT_FAILURE);
}
seen_aio = true;
if (!strcmp(optarg, "native")) {
@@ -471,16 +475,19 @@ int main(int argc, char **argv)
} else if (!strcmp(optarg, "threads")) {
/* this is the default */
} else {
- errx(EXIT_FAILURE, "invalid aio mode `%s'", optarg);
+ error_report("invalid aio mode `%s'", optarg);
+ exit(EXIT_FAILURE);
}
break;
case QEMU_NBD_OPT_DISCARD:
if (seen_discard) {
- errx(EXIT_FAILURE, "--discard can only be specified once");
+ error_report("--discard can only be specified once");
+ exit(EXIT_FAILURE);
}
seen_discard = true;
if (bdrv_parse_discard_flags(optarg, &flags) == -1) {
- errx(EXIT_FAILURE, "Invalid discard mode `%s'", optarg);
+ error_report("Invalid discard mode `%s'", optarg);
+ exit(EXIT_FAILURE);
}
break;
case QEMU_NBD_OPT_DETECT_ZEROES:
@@ -491,13 +498,14 @@ int main(int argc, char **argv)
BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
&local_err);
if (local_err) {
- errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s",
- error_get_pretty(local_err));
+ error_report("Failed to parse detect_zeroes mode: %s",
+ error_get_pretty(local_err));
+ exit(EXIT_FAILURE);
}
if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
!(flags & BDRV_O_UNMAP)) {
- errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
- "without setting discard operation to unmap");
+ error_report("setting detect-zeroes to unmap is not allowed " "without setting discard operation to unmap");
+ exit(EXIT_FAILURE);
}
break;
case 'b':
@@ -509,10 +517,12 @@ int main(int argc, char **argv)
case 'o':
dev_offset = strtoll (optarg, &end, 0);
if (*end) {
- errx(EXIT_FAILURE, "Invalid offset `%s'", optarg);
+ error_report("Invalid offset `%s'", optarg);
+ exit(EXIT_FAILURE);
}
if (dev_offset < 0) {
- errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
+ error_report("Offset must be positive `%s'", optarg);
+ exit(EXIT_FAILURE);
}
break;
case 'l':
@@ -520,8 +530,9 @@ int main(int argc, char **argv)
sn_opts = qemu_opts_parse_noisily(&internal_snapshot_opts,
optarg, false);
if (!sn_opts) {
- errx(EXIT_FAILURE, "Failed in parsing snapshot param `%s'",
- optarg);
+ error_report("Failed in parsing snapshot param `%s'",
+ optarg);
+ exit(EXIT_FAILURE);
}
} else {
sn_id_or_name = optarg;
@@ -534,16 +545,19 @@ int main(int argc, char **argv)
case 'P':
partition = strtol(optarg, &end, 0);
if (*end) {
- errx(EXIT_FAILURE, "Invalid partition `%s'", optarg);
+ error_report("Invalid partition `%s'", optarg);
+ exit(EXIT_FAILURE);
}
if (partition < 1 || partition > 8) {
- errx(EXIT_FAILURE, "Invalid partition %d", partition);
+ error_report("Invalid partition %d", partition);
+ exit(EXIT_FAILURE);
}
break;
case 'k':
sockpath = optarg;
if (sockpath[0] != '/') {
- errx(EXIT_FAILURE, "socket path must be absolute\n");
+ error_report("socket path must be absolute\n");
+ exit(EXIT_FAILURE);
}
break;
case 'd':
@@ -555,10 +569,12 @@ int main(int argc, char **argv)
case 'e':
shared = strtol(optarg, &end, 0);
if (*end) {
- errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg);
+ error_report("Invalid shared device number '%s'", optarg);
+ exit(EXIT_FAILURE);
}
if (shared < 1) {
- errx(EXIT_FAILURE, "Shared device number must be greater than 0\n");
+ error_report("Shared device number must be greater than 0\n");
+ exit(EXIT_FAILURE);
}
break;
case 'f':
@@ -579,21 +595,23 @@ int main(int argc, char **argv)
exit(0);
break;
case '?':
- errx(EXIT_FAILURE, "Try `%s --help' for more information.",
- argv[0]);
+ error_report("Try `%s --help' for more information.", argv[0]);
+ exit(EXIT_FAILURE);
}
}
if ((argc - optind) != 1) {
- errx(EXIT_FAILURE, "Invalid number of argument.\n"
- "Try `%s --help' for more information.",
- argv[0]);
+ error_report("Invalid number of argument.\n" "Try `%s --help' for more information.",
+ argv[0]);
+ exit(EXIT_FAILURE);
}
if (disconnect) {
fd = open(argv[optind], O_RDWR);
if (fd < 0) {
- err(EXIT_FAILURE, "Cannot open %s", argv[optind]);
+ error_report("Cannot open %s: %s", argv[optind],
+ strerror(errno));
+ exit(EXIT_FAILURE);
}
nbd_disconnect(fd);
@@ -610,7 +628,9 @@ int main(int argc, char **argv)
int ret;
if (qemu_pipe(stderr_fd) < 0) {
- err(EXIT_FAILURE, "Error setting up communication pipe");
+ error_report("Error setting up communication pipe: %s",
+ strerror(errno));
+ exit(EXIT_FAILURE);
}
/* Now daemonize, but keep a communication channel open to
@@ -618,7 +638,8 @@ int main(int argc, char **argv)
*/
pid = fork();
if (pid < 0) {
- err(EXIT_FAILURE, "Failed to fork");
+ error_report("Failed to fork: %s", strerror(errno));
+ exit(EXIT_FAILURE);
} else if (pid == 0) {
close(stderr_fd[0]);
ret = qemu_daemon(1, 0);
@@ -626,7 +647,8 @@ int main(int argc, char **argv)
/* Temporarily redirect stderr to the parent's pipe... */
dup2(stderr_fd[1], STDERR_FILENO);
if (ret < 0) {
- err(EXIT_FAILURE, "Failed to daemonize");
+ error_report("Failed to daemonize: %s", strerror(errno));
+ exit(EXIT_FAILURE);
}
/* ... close the descriptor we inherited and go on. */
@@ -648,7 +670,9 @@ int main(int argc, char **argv)
}
}
if (ret < 0) {
- err(EXIT_FAILURE, "Cannot read from daemon");
+ error_report("Cannot read from daemon: %s",
+ strerror(errno));
+ exit(EXIT_FAILURE);
}
/* Usually the daemon should not print any message.
@@ -680,8 +704,9 @@ int main(int argc, char **argv)
srcpath = argv[optind];
blk = blk_new_open("hda", srcpath, NULL, options, flags, &local_err);
if (!blk) {
- errx(EXIT_FAILURE, "Failed to blk_new_open '%s': %s", argv[optind],
- error_get_pretty(local_err));
+ error_report("Failed to blk_new_open '%s': %s", argv[optind],
+ error_get_pretty(local_err));
+ exit(EXIT_FAILURE);
}
bs = blk_bs(blk);
@@ -696,30 +721,34 @@ int main(int argc, char **argv)
}
if (ret < 0) {
errno = -ret;
- err(EXIT_FAILURE,
- "Failed to load snapshot: %s",
- error_get_pretty(local_err));
+ error_report("Failed to load snapshot: %s: %s",
+ error_get_pretty(local_err), strerror(errno));
+ exit(EXIT_FAILURE);
}
bs->detect_zeroes = detect_zeroes;
fd_size = blk_getlength(blk);
if (fd_size < 0) {
- errx(EXIT_FAILURE, "Failed to determine the image length: %s",
- strerror(-fd_size));
+ error_report("Failed to determine the image length: %s",
+ strerror(-fd_size));
+ exit(EXIT_FAILURE);
}
if (partition != -1) {
ret = find_partition(blk, partition, &dev_offset, &fd_size);
if (ret < 0) {
errno = -ret;
- err(EXIT_FAILURE, "Could not find partition %d", partition);
+ error_report("Could not find partition %d: %s", partition,
+ strerror(errno));
+ exit(EXIT_FAILURE);
}
}
exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed,
&local_err);
if (!exp) {
- errx(EXIT_FAILURE, "%s", error_get_pretty(local_err));
+ error_report("%s", error_get_pretty(local_err));
+ exit(EXIT_FAILURE);
}
fd = socket_listen(saddr, &local_err);
@@ -733,8 +762,8 @@ int main(int argc, char **argv)
ret = pthread_create(&client_thread, NULL, nbd_client_thread, device);
if (ret != 0) {
- errx(EXIT_FAILURE, "Failed to create client thread: %s",
- strerror(ret));
+ error_report("Failed to create client thread: %s", strerror(ret));
+ exit(EXIT_FAILURE);
}
} else {
/* Shut up GCC warnings. */
@@ -747,7 +776,9 @@ int main(int argc, char **argv)
/* now when the initialization is (almost) complete, chdir("/")
* to free any busy filesystems */
if (chdir("/") < 0) {
- err(EXIT_FAILURE, "Could not chdir to root directory");
+ error_report("Could not chdir to root directory: %s",
+ strerror(errno));
+ exit(EXIT_FAILURE);
}
state = RUNNING;
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 02/23] error: Use error_report_err() where appropriate (again)
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report() Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 17:33 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 03/23] error: Use error_report_err() instead of monitor_printf() Markus Armbruster
` (21 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Same Coccinelle semantic patch as in commit 565f65d.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/sheepdog.c | 3 +--
hw/arm/imx25_pdk.c | 2 +-
hw/arm/kzm.c | 2 +-
hw/arm/netduino2.c | 2 +-
hw/arm/xlnx-ep108.c | 2 +-
hw/ppc/spapr_drc.c | 6 ++----
qemu-nbd.c | 2 +-
vl.c | 2 +-
8 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index d80e4ed..dd8301b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1861,8 +1861,7 @@ static int sd_create(const char *filename, QemuOpts *opts,
fd = connect_to_sdog(s, &local_err);
if (fd < 0) {
- error_report("%s", error_get_pretty(local_err));
- error_free(local_err);
+ error_report_err(local_err);
ret = -EIO;
goto out;
}
diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index 59a4c11..039f0eb 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -75,7 +75,7 @@ static void imx25_pdk_init(MachineState *machine)
object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
if (err != NULL) {
- error_report("%s", error_get_pretty(err));
+ error_report_err(err);
exit(1);
}
diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index eff6f46..f4b463a 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -74,7 +74,7 @@ static void kzm_init(MachineState *machine)
object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
if (err != NULL) {
- error_report("%s", error_get_pretty(err));
+ error_report_err(err);
exit(1);
}
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index a3b9e82..3ab83a1 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -38,7 +38,7 @@ static void netduino2_init(MachineState *machine)
qdev_prop_set_string(dev, "cpu-model", "cortex-m3");
object_property_set_bool(OBJECT(dev), true, "realized", &err);
if (err != NULL) {
- error_report("%s", error_get_pretty(err));
+ error_report_err(err);
exit(1);
}
}
diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index 85b978f..73e6087 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -41,7 +41,7 @@ static void xlnx_ep108_init(MachineState *machine)
object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
if (err) {
- error_report("%s", error_get_pretty(err));
+ error_report_err(err);
exit(1);
}
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 8be62c3..4fb86a6 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -465,8 +465,7 @@ static void realize(DeviceState *d, Error **errp)
object_property_add_alias(root_container, link_name,
drc->owner, child_name, &err);
if (err) {
- error_report("%s", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
object_unref(OBJECT(drc));
}
g_free(child_name);
@@ -486,8 +485,7 @@ static void unrealize(DeviceState *d, Error **errp)
snprintf(name, sizeof(name), "%x", drck->get_index(drc));
object_property_del(root_container, name, &err);
if (err) {
- error_report("%s", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
object_unref(OBJECT(drc));
}
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0d47f14..2881d84 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -747,7 +747,7 @@ int main(int argc, char **argv)
exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed,
&local_err);
if (!exp) {
- error_report("%s", error_get_pretty(local_err));
+ error_report_err(local_err);
exit(EXIT_FAILURE);
}
diff --git a/vl.c b/vl.c
index 5cc6bfd..5a395e9 100644
--- a/vl.c
+++ b/vl.c
@@ -4553,7 +4553,7 @@ int main(int argc, char **argv, char **envp)
Error *local_err = NULL;
qemu_boot_set(boot_once, &local_err);
if (local_err) {
- error_report("%s", error_get_pretty(local_err));
+ error_report_err(local_err);
exit(1);
}
qemu_register_reset(restore_boot_order, g_strdup(boot_order));
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 03/23] error: Use error_report_err() instead of monitor_printf()
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report() Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 02/23] error: Use error_report_err() where appropriate (again) Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 18:06 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints Markus Armbruster
` (20 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Using error_report_err() instead of monitor_printf() makes no
difference when monitor_printf() is used correctly, i.e. within an HMP
monitor. Elsewhere, monitor_printf() does nothing, while
error_report_err() reports to stderr.
Most changed functions are HMP command handlers. These should only
run within an HMP monitor. The one exception is bdrv_password_cb(),
which should also only run within an HMP monitor.
Four command handlers prefix the error message with the command name:
balloon, migrate_set_capability, migrate_set_parameter, migrate.
Pointless, drop.
Coccinelle semantic patch:
@@
expression M, E;
@@
- monitor_printf(M, "%s\n", error_get_pretty(E));
- error_free(E);
+ error_report_err(E);
@r1@
expression M, E;
format F;
position p;
@@
- monitor_printf(M, "...%@F@\n", error_get_pretty(E));@p
- error_free(E);
+ error_report_err(E);
@script:python@
p << r1.p;
@@
print "%s:%s:%s: prefix dropped" % (p[0].file, p[0].line, p[0].column)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hmp.c | 29 +++++++++--------------------
hw/s390x/s390-skeys.c | 3 +--
migration/savevm.c | 3 +--
monitor.c | 6 ++----
4 files changed, 13 insertions(+), 28 deletions(-)
diff --git a/hmp.c b/hmp.c
index 2140605..96c7c7f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -41,8 +41,7 @@ static void hmp_handle_error(Monitor *mon, Error **errp)
{
assert(errp);
if (*errp) {
- monitor_printf(mon, "%s\n", error_get_pretty(*errp));
- error_free(*errp);
+ error_report_err(*errp);
}
}
@@ -548,8 +547,7 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
info = qmp_query_vnc(&err);
if (err) {
- monitor_printf(mon, "%s\n", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
return;
}
@@ -671,8 +669,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict)
info = qmp_query_balloon(&err);
if (err) {
- monitor_printf(mon, "%s\n", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
return;
}
@@ -940,8 +937,7 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict)
data = qmp_ringbuf_read(chardev, size, false, 0, &err);
if (err) {
- monitor_printf(mon, "%s\n", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
return;
}
@@ -1034,8 +1030,7 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
qmp_balloon(value, &err);
if (err) {
- monitor_printf(mon, "balloon: %s\n", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
}
}
@@ -1183,8 +1178,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
qmp_migrate_set_cache_size(value, &err);
if (err) {
- monitor_printf(mon, "%s\n", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
return;
}
}
@@ -1221,9 +1215,7 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
qapi_free_MigrationCapabilityStatusList(caps);
if (err) {
- monitor_printf(mon, "migrate_set_capability: %s\n",
- error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
}
}
@@ -1273,9 +1265,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
}
if (err) {
- monitor_printf(mon, "migrate_set_parameter: %s\n",
- error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
}
}
@@ -1536,8 +1526,7 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
if (err) {
- monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
return;
}
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 539ef6d..4af1558 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -100,8 +100,7 @@ void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
qmp_dump_skeys(filename, &err);
if (err) {
- monitor_printf(mon, "%s\n", error_get_pretty(err));
- error_free(err);
+ error_report_err(err);
}
}
diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..e277b72 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1984,8 +1984,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
vm_state_size = qemu_ftell(f);
qemu_fclose(f);
if (ret < 0) {
- monitor_printf(mon, "%s\n", error_get_pretty(local_err));
- error_free(local_err);
+ error_report_err(local_err);
goto the_end;
}
diff --git a/monitor.c b/monitor.c
index 9a35d72..0843270 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1464,8 +1464,7 @@ static void hmp_boot_set(Monitor *mon, const QDict *qdict)
qemu_boot_set(bootdevice, &local_err);
if (local_err) {
- monitor_printf(mon, "%s\n", error_get_pretty(local_err));
- error_free(local_err);
+ error_report_err(local_err);
} else {
monitor_printf(mon, "boot device list now set to %s\n", bootdevice);
}
@@ -4149,8 +4148,7 @@ static void bdrv_password_cb(void *opaque, const char *password,
bdrv_add_key(bs, password, &local_err);
if (local_err) {
- monitor_printf(mon, "%s\n", error_get_pretty(local_err));
- error_free(local_err);
+ error_report_err(local_err);
ret = -EPERM;
}
if (mon->password_completion_cb)
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (2 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 03/23] error: Use error_report_err() instead of monitor_printf() Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:10 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint() Markus Armbruster
` (19 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
contrib/ivshmem-server/main.c | 4 +---
qdev-monitor.c | 3 +--
qemu-nbd.c | 3 +--
3 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
index 54ff001..00508b5 100644
--- a/contrib/ivshmem-server/main.c
+++ b/contrib/ivshmem-server/main.c
@@ -106,9 +106,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
case 'l': /* shm_size */
parse_option_size("shm_size", optarg, &args->shm_size, &errp);
if (errp) {
- fprintf(stderr, "cannot parse shm size: %s\n",
- error_get_pretty(errp));
- error_free(errp);
+ error_report_err(errp);
ivshmem_server_usage(argv[0], 1);
}
break;
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 30936df..3ce4710 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -266,8 +266,7 @@ int qdev_device_help(QemuOpts *opts)
return 1;
error:
- error_printf("%s\n", error_get_pretty(local_err));
- error_free(local_err);
+ error_report_err(local_err);
return 1;
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2881d84..ac7ceef 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -251,8 +251,7 @@ static void *nbd_client_thread(void *arg)
&size, &local_error);
if (ret < 0) {
if (local_error) {
- fprintf(stderr, "%s\n", error_get_pretty(local_error));
- error_free(local_error);
+ error_report_err(local_error);
}
goto out_socket;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (3 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 16:59 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 06/23] block: Clean up "Could not create temporary overlay" error message Markus Armbruster
` (18 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
While there, tighten its assertion.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/error.h | 19 +++++++++++++++++--
util/error.c | 2 +-
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index b2362a5..048d947 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -18,6 +18,15 @@
* Create an error:
* error_setg(&err, "situation normal, all fouled up");
*
+ * Create an error and add additional explanation:
+ * error_setg(&err, "invalid quark");
+ * error_append_hint(&err, "Valid quarks are up, down, strange, "
+ * "charm, top, bottom\n");
+ *
+ * Do *not* contract this to
+ * error_setg(&err, "invalid quark\n"
+ * "Valid quarks are up, down, strange, charm, top, bottom");
+ *
* Report an error to stderr:
* error_report_err(err);
* This frees the error object.
@@ -26,6 +35,7 @@
* const char *msg = error_get_pretty(err);
* do with msg what needs to be done...
* error_free(err);
+ * Note that this loses hints added with error_append_hint().
*
* Handle an error without reporting it (just for completeness):
* error_free(err);
@@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err);
* If @errp is anything else, *@errp must be NULL.
* The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
* human-readable error message is made from printf-style @fmt, ...
+ * The resulting message should not contain newlines.
*/
#define error_setg(errp, fmt, ...) \
error_setg_internal((errp), __FILE__, __LINE__, __func__, \
@@ -184,7 +195,11 @@ void error_propagate(Error **dst_errp, Error *local_err);
/**
* Append a printf-style human-readable explanation to an existing error.
- * May be called multiple times, and safe if @errp is NULL.
+ * @errp may be NULL, but neither &error_fatal nor &error_abort.
+ * Trivially the case if you call it only after error_setg() or
+ * error_propagate().
+ * May be called multiple times. The resulting hint should end with a
+ * newline.
*/
void error_append_hint(Error **errp, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
@@ -218,7 +233,7 @@ void error_free_or_abort(Error **errp);
/*
* Convenience function to error_report() and free @err.
*/
-void error_report_err(Error *);
+void error_report_err(Error *err);
/*
* Just like error_setg(), except you get to specify the error class.
diff --git a/util/error.c b/util/error.c
index 9b27c45..ebfb74b 100644
--- a/util/error.c
+++ b/util/error.c
@@ -132,7 +132,7 @@ void error_append_hint(Error **errp, const char *fmt, ...)
return;
}
err = *errp;
- assert(err && errp != &error_abort);
+ assert(err && errp != &error_abort && errp != &error_fatal);
if (!err->hint) {
err->hint = g_string_new(NULL);
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 06/23] block: Clean up "Could not create temporary overlay" error message
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (4 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint() Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:16 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 07/23] qemu-nbd: Clean up "Failed to load snapshot" " Markus Armbruster
` (17 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
bdrv_create() sets an error and returns -errno on failure. When the
latter is interesting, the error is created with error_setg_errno().
bdrv_append_temp_snapshot() uses the error's message to create a new
one with error_setg_errno(). This adds a strerror() that is either
uninteresting or duplicate. Use error_setg() instead.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 3a7324b..4b0845e 100644
--- a/block.c
+++ b/block.c
@@ -1336,9 +1336,8 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, &local_err);
qemu_opts_del(opts);
if (ret < 0) {
- error_setg_errno(errp, -ret, "Could not create temporary overlay "
- "'%s': %s", tmp_filename,
- error_get_pretty(local_err));
+ error_setg(errp, "Could not create temporary overlay '%s': %s",
+ tmp_filename, error_get_pretty(local_err));
error_free(local_err);
goto out;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 07/23] qemu-nbd: Clean up "Failed to load snapshot" error message
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (5 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 06/23] block: Clean up "Could not create temporary overlay" error message Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:17 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 08/23] test-throttle: Simplify qemu_init_main_loop() error handling Markus Armbruster
` (16 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
bdrv_snapshot_load_tmp() sets an error and returns -errno on failure.
We report both even though the error message is self-contained. Drop
the redundant strerror().
While there: setting errno right before exit() is pointless, so drop
that, too.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-nbd.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index ac7ceef..b8ee2d0 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -156,8 +156,7 @@ static int find_partition(BlockBackend *blk, int partition,
int ret;
if ((ret = blk_read(blk, 0, data, 1)) < 0) {
- errno = -ret;
- error_report("error while reading: %s", strerror(errno));
+ error_report("error while reading: %s", strerror(-ret));
exit(EXIT_FAILURE);
}
@@ -178,8 +177,7 @@ static int find_partition(BlockBackend *blk, int partition,
int j;
if ((ret = blk_read(blk, mbr[i].start_sector_abs, data1, 1)) < 0) {
- errno = -ret;
- error_report("error while reading: %s", strerror(errno));
+ error_report("error while reading: %s", strerror(-ret));
exit(EXIT_FAILURE);
}
@@ -719,9 +717,8 @@ int main(int argc, char **argv)
&local_err);
}
if (ret < 0) {
- errno = -ret;
- error_report("Failed to load snapshot: %s: %s",
- error_get_pretty(local_err), strerror(errno));
+ error_report("Failed to load snapshot: %s",
+ error_get_pretty(local_err));
exit(EXIT_FAILURE);
}
@@ -736,9 +733,8 @@ int main(int argc, char **argv)
if (partition != -1) {
ret = find_partition(blk, partition, &dev_offset, &fd_size);
if (ret < 0) {
- errno = -ret;
error_report("Could not find partition %d: %s", partition,
- strerror(errno));
+ strerror(-ret));
exit(EXIT_FAILURE);
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 08/23] test-throttle: Simplify qemu_init_main_loop() error handling
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (6 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 07/23] qemu-nbd: Clean up "Failed to load snapshot" " Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:19 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 09/23] error: New error_prepend(), error_reportf_err() Markus Armbruster
` (15 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
The code looks like it tries to check for both qemu_init_main_loop()
and qemu_get_aio_context() failure in one conditional. In fact,
qemu_get_aio_context() can fail only after qemu_init_main_loop()
failed.
Simplify accordingly: check for qemu_init_main_loop() error directly,
without bothering to improve its error message. Call
qemu_get_aio_context() only when qemu_get_aio_context() succeeded. It
can't fail then, so no need to check.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-throttle.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 85c9b6c..a95039f 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -581,21 +581,8 @@ static void test_groups(void)
int main(int argc, char **argv)
{
- Error *local_error = NULL;
-
- qemu_init_main_loop(&local_error);
+ qemu_init_main_loop(&error_fatal);
ctx = qemu_get_aio_context();
-
- if (!ctx) {
- error_report("Failed to create AIO Context: '%s'",
- local_error ? error_get_pretty(local_error) :
- "Failed to initialize the QEMU main loop");
- if (local_error) {
- error_free(local_error);
- }
- exit(1);
- }
-
bdrv_init();
do {} while (g_main_context_iteration(NULL, false));
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 09/23] error: New error_prepend(), error_reportf_err()
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (7 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 08/23] test-throttle: Simplify qemu_init_main_loop() error handling Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:23 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 10/23] error: Don't decorate original error message when adding to it Markus Armbruster
` (14 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Instead of simply propagating an error verbatim, we sometimes want to
add to its message, like this:
frobnicate(arg, &err);
error_setg(errp, "Can't frobnicate %s: %s",
arg, error_get_pretty(err));
error_free(err);
This is suboptimal, because it loses err's hint (if any). Moreover,
when errp is &error_abort or is subsequently propagated to
&error_abort, the abort message points to the place where we last
added to the error, not to the place where it originated.
To avoid these issues, provide means to add to an error's message in
place:
frobnicate(arg, errp);
error_prepend(errp, "Can't frobnicate %s: ", arg);
Likewise, reporting an error like
frobnicate(arg, &err);
error_report("Can't frobnicate %s: %s", arg, error_get_pretty(err));
can lose err's hint. To avoid:
error_reportf_err(err, "Can't frobnicate %s: ", arg);
The next commits will put these functions to use.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/error.h | 31 +++++++++++++++++++++++++++++--
util/error.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 048d947..139d885 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -31,6 +31,9 @@
* error_report_err(err);
* This frees the error object.
*
+ * Report an error to stderr with additional text prepended:
+ * error_reportf_err(err, "Could not frobnicate '%s': ", name);
+ *
* Report an error somewhere else:
* const char *msg = error_get_pretty(err);
* do with msg what needs to be done...
@@ -48,6 +51,10 @@
* error_propagate(errp, err);
* where Error **errp is a parameter, by convention the last one.
*
+ * Pass an existing error to the caller with the message modified:
+ * error_propagate(errp, err);
+ * error_prepend(errp, "Could not frobnicate '%s': ", name);
+ *
* Create a new error and pass it to the caller:
* error_setg(errp, "situation normal, all fouled up");
*
@@ -108,9 +115,10 @@
#ifndef ERROR_H
#define ERROR_H
+#include <stdarg.h>
+#include <stdbool.h>
#include "qemu/compiler.h"
#include "qapi-types.h"
-#include <stdbool.h>
/*
* Opaque error object.
@@ -193,7 +201,20 @@ void error_setg_win32_internal(Error **errp,
*/
void error_propagate(Error **dst_errp, Error *local_err);
-/**
+/*
+ * Prepend some text to @errp's human-readable error message.
+ * The text is made by formatting @fmt, @ap like vprintf().
+ */
+void error_vprepend(Error **errp, const char *fmt, va_list ap);
+
+/*
+ * Prepend some text to @errp's human-readable error message.
+ * The text is made by formatting @fmt, ... like printf().
+ */
+void error_prepend(Error **errp, const char *fmt, ...)
+ GCC_FMT_ATTR(2, 3);
+
+/*
* Append a printf-style human-readable explanation to an existing error.
* @errp may be NULL, but neither &error_fatal nor &error_abort.
* Trivially the case if you call it only after error_setg() or
@@ -236,6 +257,12 @@ void error_free_or_abort(Error **errp);
void error_report_err(Error *err);
/*
+ * Convenience function to error_prepend(), error_report() and free @err.
+ */
+void error_reportf_err(Error *err, const char *fmt, ...)
+ GCC_FMT_ATTR(2, 3);
+
+/*
* Just like error_setg(), except you get to specify the error class.
* Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
* strongly discouraged.
diff --git a/util/error.c b/util/error.c
index ebfb74b..57303fd 100644
--- a/util/error.c
+++ b/util/error.c
@@ -122,6 +122,29 @@ void error_setg_file_open_internal(Error **errp,
"Could not open '%s'", filename);
}
+void error_vprepend(Error **errp, const char *fmt, va_list ap)
+{
+ GString *newmsg;
+
+ if (!errp) {
+ return;
+ }
+
+ newmsg = g_string_new(NULL);
+ g_string_vprintf(newmsg, fmt, ap);
+ g_string_append(newmsg, (*errp)->msg);
+ (*errp)->msg = g_string_free(newmsg, 0);
+}
+
+void error_prepend(Error **errp, const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ error_vprepend(errp, fmt, ap);
+ va_end(ap);
+}
+
void error_append_hint(Error **errp, const char *fmt, ...)
{
va_list ap;
@@ -209,6 +232,16 @@ void error_report_err(Error *err)
error_free(err);
}
+void error_reportf_err(Error *err, const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ error_vprepend(&err, fmt, ap);
+ va_end(ap);
+ error_report_err(err);
+}
+
void error_free(Error *err)
{
if (err) {
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 10/23] error: Don't decorate original error message when adding to it
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (8 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 09/23] error: New error_prepend(), error_reportf_err() Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:32 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 11/23] error: Use error_reportf_err() where it makes obvious sense Markus Armbruster
` (13 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Prepend the additional information, colon, space to the original
message without enclosing it in parenthesis or quotes, like we do
elsewhere.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/core/qdev-properties.c | 2 +-
qemu-img.c | 2 +-
tests/test-aio.c | 2 +-
tests/test-thread-pool.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 33e245e..fffb58e 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1063,7 +1063,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
if (err != NULL) {
assert(prop->user_provided);
- error_report("Warning: global %s.%s=%s ignored (%s)",
+ error_report("Warning: global %s.%s=%s ignored: %s",
prop->driver, prop->property, prop->value,
error_get_pretty(err));
error_free(err);
diff --git a/qemu-img.c b/qemu-img.c
index 033011c..ea8c84d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2436,7 +2436,7 @@ static int img_snapshot(int argc, char **argv)
case SNAPSHOT_DELETE:
bdrv_snapshot_delete_by_id_or_name(bs, snapshot_name, &err);
if (err) {
- error_report("Could not delete snapshot '%s': (%s)",
+ error_report("Could not delete snapshot '%s': %s",
snapshot_name, error_get_pretty(err));
error_free(err);
ret = 1;
diff --git a/tests/test-aio.c b/tests/test-aio.c
index e188d8c..f0b447e 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -832,7 +832,7 @@ int main(int argc, char **argv)
ctx = aio_context_new(&local_error);
if (!ctx) {
- error_report("Failed to create AIO Context: '%s'",
+ error_report("Failed to create AIO Context: %s",
error_get_pretty(local_error));
error_free(local_error);
exit(1);
diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
index 6a0b981..153b8f5 100644
--- a/tests/test-thread-pool.c
+++ b/tests/test-thread-pool.c
@@ -229,7 +229,7 @@ int main(int argc, char **argv)
ctx = aio_context_new(&local_error);
if (!ctx) {
- error_report("Failed to create AIO Context: '%s'",
+ error_report("Failed to create AIO Context: %s",
error_get_pretty(local_error));
error_free(local_error);
exit(1);
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 11/23] error: Use error_reportf_err() where it makes obvious sense
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (9 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 10/23] error: Don't decorate original error message when adding to it Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:39 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 12/23] error: Use error_prepend() " Markus Armbruster
` (12 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Done with this Coccinelle semantic patch
@@
expression FMT, E;
expression list ARGS;
@@
- error_report(FMT, ARGS, error_get_pretty(E));
+ error_reportf_err(E, FMT/*@@@*/, ARGS);
(
- error_free(E);
|
exit(S);
|
abort();
)
followed by a replace of '%s"/*@@@*/' by '"' and some line rewrapping,
because I can't figure out how to make Coccinelle transform strings.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
arch_init.c | 4 +---
block/sheepdog.c | 5 ++---
blockdev.c | 11 ++++-------
hw/arm/cubieboard.c | 9 ++++-----
hw/arm/digic_boards.c | 3 +--
hw/core/qdev-properties.c | 6 ++----
hw/core/qdev.c | 5 ++---
hw/i386/pc.c | 5 ++---
hw/ppc/e500.c | 4 ++--
hw/usb/bus.c | 5 ++---
qemu-img.c | 33 +++++++++++++--------------------
qemu-nbd.c | 11 +++++------
replay/replay.c | 3 +--
tests/test-aio.c | 4 +---
tests/test-thread-pool.c | 4 +---
ui/vnc.c | 4 +---
vl.c | 6 ++----
17 files changed, 46 insertions(+), 76 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 38f5fb9..d1383b3 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -258,9 +258,7 @@ void do_acpitable_option(const QemuOpts *opts)
acpi_table_add(opts, &err);
if (err) {
- error_report("Wrong acpi table provided: %s",
- error_get_pretty(err));
- error_free(err);
+ error_reportf_err(err, "Wrong acpi table provided: ");
exit(1);
}
#endif
diff --git a/block/sheepdog.c b/block/sheepdog.c
index dd8301b..6986be8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2405,9 +2405,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
ret = do_sd_create(s, &new_vid, 1, &local_err);
if (ret < 0) {
- error_report("failed to create inode for snapshot: %s",
- error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err,
+ "failed to create inode for snapshot: ");
goto cleanup;
}
diff --git a/blockdev.c b/blockdev.c
index 80932e8..928db2d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1583,13 +1583,10 @@ static void internal_snapshot_abort(BlkActionState *common)
}
if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
- error_report("Failed to delete snapshot with id '%s' and name '%s' on "
- "device '%s' in abort: %s",
- sn->id_str,
- sn->name,
- bdrv_get_device_name(bs),
- error_get_pretty(local_error));
- error_free(local_error);
+ error_reportf_err(local_error,
+ "Failed to delete snapshot with id '%s' and name '%s' on " "device '%s' in abort: ",
+ sn->id_str, sn->name,
+ bdrv_get_device_name(bs));
}
}
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index bf068cd..a71e43c 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -39,27 +39,26 @@ static void cubieboard_init(MachineState *machine)
object_property_set_int(OBJECT(&s->a10->emac), 1, "phy-addr", &err);
if (err != NULL) {
- error_report("Couldn't set phy address: %s", error_get_pretty(err));
+ error_reportf_err(err, "Couldn't set phy address: ");
exit(1);
}
object_property_set_int(OBJECT(&s->a10->timer), 32768, "clk0-freq", &err);
if (err != NULL) {
- error_report("Couldn't set clk0 frequency: %s", error_get_pretty(err));
+ error_reportf_err(err, "Couldn't set clk0 frequency: ");
exit(1);
}
object_property_set_int(OBJECT(&s->a10->timer), 24000000, "clk1-freq",
&err);
if (err != NULL) {
- error_report("Couldn't set clk1 frequency: %s", error_get_pretty(err));
+ error_reportf_err(err, "Couldn't set clk1 frequency: ");
exit(1);
}
object_property_set_bool(OBJECT(s->a10), true, "realized", &err);
if (err != NULL) {
- error_report("Couldn't realize Allwinner A10: %s",
- error_get_pretty(err));
+ error_reportf_err(err, "Couldn't realize Allwinner A10: ");
exit(1);
}
diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index 710045a..dfaed25 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -64,8 +64,7 @@ static void digic4_board_init(DigicBoard *board)
s->digic = DIGIC(object_new(TYPE_DIGIC));
object_property_set_bool(OBJECT(s->digic), true, "realized", &err);
if (err != NULL) {
- error_report("Couldn't realize DIGIC SoC: %s",
- error_get_pretty(err));
+ error_reportf_err(err, "Couldn't realize DIGIC SoC: ");
exit(1);
}
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index fffb58e..3572810 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1063,10 +1063,8 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
if (err != NULL) {
assert(prop->user_provided);
- error_report("Warning: global %s.%s=%s ignored: %s",
- prop->driver, prop->property, prop->value,
- error_get_pretty(err));
- error_free(err);
+ error_reportf_err(err, "Warning: global %s.%s=%s ignored: ",
+ prop->driver, prop->property, prop->value);
return;
}
}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index b3ad467..2a44ec5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -370,9 +370,8 @@ void qdev_init_nofail(DeviceState *dev)
object_property_set_bool(OBJECT(dev), true, "realized", &err);
if (err) {
- error_report("Initialization of device %s failed: %s",
- object_get_typename(OBJECT(dev)),
- error_get_pretty(err));
+ error_reportf_err(err, "Initialization of device %s failed: ",
+ object_get_typename(OBJECT(dev)));
exit(1);
}
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3025ca5..112361a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1252,9 +1252,8 @@ void pc_acpi_init(const char *default_dsdt)
acpi_table_add_builtin(opts, &err);
if (err) {
- error_report("WARNING: failed to load %s: %s", filename,
- error_get_pretty(err));
- error_free(err);
+ error_reportf_err(err, "WARNING: failed to load %s: ",
+ filename);
}
g_free(filename);
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index b3418db..bd7da47 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -751,8 +751,8 @@ static qemu_irq *ppce500_init_mpic(MachineState *machine, PPCE500Params *params,
dev = ppce500_init_mpic_kvm(params, irqs, &err);
}
if (machine_kernel_irqchip_required(machine) && !dev) {
- error_report("kernel_irqchip requested but unavailable: %s",
- error_get_pretty(err));
+ error_reportf_err(err,
+ "kernel_irqchip requested but unavailable: ");
exit(1);
}
}
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index ee6b43a..26ab67f 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -725,9 +725,8 @@ USBDevice *usbdevice_create(const char *cmdline)
}
object_property_set_bool(OBJECT(dev), true, "realized", &err);
if (err) {
- error_report("Failed to initialize USB device '%s': %s",
- f->name, error_get_pretty(err));
- error_free(err);
+ error_reportf_err(err, "Failed to initialize USB device '%s': ",
+ f->name);
object_unparent(OBJECT(dev));
return NULL;
}
diff --git a/qemu-img.c b/qemu-img.c
index ea8c84d..a4251cc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -213,9 +213,7 @@ static BlockBackend *img_open(const char *id, const char *filename,
blk = blk_new_open(id, filename, NULL, options, flags, &local_err);
if (!blk) {
- error_report("Could not open '%s': %s", filename,
- error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err, "Could not open '%s': ", filename);
goto fail;
}
@@ -360,8 +358,7 @@ static int img_create(int argc, char **argv)
bdrv_img_create(filename, fmt, base_filename, base_fmt,
options, img_size, BDRV_O_FLAGS, &local_err, quiet);
if (local_err) {
- error_report("%s: %s", filename, error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err, "%s: ", filename);
goto fail;
}
@@ -1711,9 +1708,7 @@ static int img_convert(int argc, char **argv)
bdrv_snapshot_load_tmp_by_id_or_name(bs[0], snapshot_name, &local_err);
}
if (local_err) {
- error_report("Failed to load snapshot: %s",
- error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err, "Failed to load snapshot: ");
ret = -1;
goto out;
}
@@ -1809,9 +1804,8 @@ static int img_convert(int argc, char **argv)
/* Create the new image */
ret = bdrv_create(drv, out_filename, opts, &local_err);
if (ret < 0) {
- error_report("%s: error while converting %s: %s",
- out_filename, out_fmt, error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err, "%s: error while converting %s: ",
+ out_filename, out_fmt);
goto out;
}
}
@@ -2436,9 +2430,8 @@ static int img_snapshot(int argc, char **argv)
case SNAPSHOT_DELETE:
bdrv_snapshot_delete_by_id_or_name(bs, snapshot_name, &err);
if (err) {
- error_report("Could not delete snapshot '%s': %s",
- snapshot_name, error_get_pretty(err));
- error_free(err);
+ error_reportf_err(err, "Could not delete snapshot '%s': ",
+ snapshot_name);
ret = 1;
}
break;
@@ -2571,9 +2564,9 @@ static int img_rebase(int argc, char **argv)
blk_old_backing = blk_new_open("old_backing", backing_name, NULL,
options, src_flags, &local_err);
if (!blk_old_backing) {
- error_report("Could not open old backing file '%s': %s",
- backing_name, error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err,
+ "Could not open old backing file '%s': ",
+ backing_name);
goto out;
}
@@ -2588,9 +2581,9 @@ static int img_rebase(int argc, char **argv)
blk_new_backing = blk_new_open("new_backing", out_baseimg, NULL,
options, src_flags, &local_err);
if (!blk_new_backing) {
- error_report("Could not open new backing file '%s': %s",
- out_baseimg, error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err,
+ "Could not open new backing file '%s': ",
+ out_baseimg);
goto out;
}
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b8ee2d0..390852f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -495,8 +495,8 @@ int main(int argc, char **argv)
BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
&local_err);
if (local_err) {
- error_report("Failed to parse detect_zeroes mode: %s",
- error_get_pretty(local_err));
+ error_reportf_err(local_err,
+ "Failed to parse detect_zeroes mode: ");
exit(EXIT_FAILURE);
}
if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
@@ -701,8 +701,8 @@ int main(int argc, char **argv)
srcpath = argv[optind];
blk = blk_new_open("hda", srcpath, NULL, options, flags, &local_err);
if (!blk) {
- error_report("Failed to blk_new_open '%s': %s", argv[optind],
- error_get_pretty(local_err));
+ error_reportf_err(local_err, "Failed to blk_new_open '%s': ",
+ argv[optind]);
exit(EXIT_FAILURE);
}
bs = blk_bs(blk);
@@ -717,8 +717,7 @@ int main(int argc, char **argv)
&local_err);
}
if (ret < 0) {
- error_report("Failed to load snapshot: %s",
- error_get_pretty(local_err));
+ error_reportf_err(local_err, "Failed to load snapshot: ");
exit(EXIT_FAILURE);
}
diff --git a/replay/replay.c b/replay/replay.c
index 0d33e82..e4673b3 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -291,8 +291,7 @@ void replay_start(void)
}
if (replay_blockers) {
- error_report("Record/replay: %s",
- error_get_pretty(replay_blockers->data));
+ error_reportf_err(replay_blockers->data, "Record/replay: ");
exit(1);
}
if (!use_icount) {
diff --git a/tests/test-aio.c b/tests/test-aio.c
index f0b447e..6ccea98 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -832,9 +832,7 @@ int main(int argc, char **argv)
ctx = aio_context_new(&local_error);
if (!ctx) {
- error_report("Failed to create AIO Context: %s",
- error_get_pretty(local_error));
- error_free(local_error);
+ error_reportf_err(local_error, "Failed to create AIO Context: ");
exit(1);
}
src = aio_get_g_source(ctx);
diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
index 153b8f5..ccdee39 100644
--- a/tests/test-thread-pool.c
+++ b/tests/test-thread-pool.c
@@ -229,9 +229,7 @@ int main(int argc, char **argv)
ctx = aio_context_new(&local_error);
if (!ctx) {
- error_report("Failed to create AIO Context: %s",
- error_get_pretty(local_error));
- error_free(local_error);
+ error_reportf_err(local_error, "Failed to create AIO Context: ");
exit(1);
}
pool = aio_get_thread_pool(ctx);
diff --git a/ui/vnc.c b/ui/vnc.c
index cbe4d33..9da54e0 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3925,9 +3925,7 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
vnc_display_init(id);
vnc_display_open(id, &local_err);
if (local_err != NULL) {
- error_report("Failed to start VNC server: %s",
- error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err, "Failed to start VNC server: ");
exit(1);
}
return 0;
diff --git a/vl.c b/vl.c
index 5a395e9..7c51ec4 100644
--- a/vl.c
+++ b/vl.c
@@ -3043,7 +3043,7 @@ int main(int argc, char **argv, char **envp)
runstate_init();
if (qcrypto_init(&err) < 0) {
- error_report("cannot initialize crypto: %s", error_get_pretty(err));
+ error_reportf_err(err, "cannot initialize crypto: ");
exit(1);
}
rtc_clock = QEMU_CLOCK_HOST;
@@ -4649,9 +4649,7 @@ int main(int argc, char **argv, char **envp)
Error *local_err = NULL;
qemu_start_incoming_migration(incoming, &local_err);
if (local_err) {
- error_report("-incoming %s: %s", incoming,
- error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err, "-incoming %s: ", incoming);
exit(1);
}
} else if (autostart) {
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 12/23] error: Use error_prepend() where it makes obvious sense
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (10 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 11/23] error: Use error_reportf_err() where it makes obvious sense Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:43 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 13/23] spapr: Use error_reportf_err() Markus Armbruster
` (11 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Done with this Coccinelle semantic patch
@@
expression FMT, E1, E2;
expression list ARGS;
@@
- error_setg(E1, FMT, ARGS, error_get_pretty(E2));
+ error_propagate(E1, E2);/*###*/
+ error_prepend(E1, FMT/*@@@*/, ARGS);
followed by manual cleanup, first because I can't figure out how to
make Coccinelle transform strings, and second to get rid of now
superfluous error_propagate().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block.c | 19 ++++++++-----------
block/qcow2.c | 5 ++---
block/qed.c | 5 ++---
hw/block/dataplane/virtio-blk.c | 8 ++------
hw/scsi/vhost-scsi.c | 6 ++----
hw/usb/bus.c | 6 +++---
6 files changed, 19 insertions(+), 30 deletions(-)
diff --git a/block.c b/block.c
index 4b0845e..3316949 100644
--- a/block.c
+++ b/block.c
@@ -1223,14 +1223,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
assert(bs->backing == NULL);
ret = bdrv_open_inherit(&backing_hd,
*backing_filename ? backing_filename : NULL,
- NULL, options, 0, bs, &child_backing, &local_err);
+ NULL, options, 0, bs, &child_backing, errp);
if (ret < 0) {
bdrv_unref(backing_hd);
backing_hd = NULL;
bs->open_flags |= BDRV_O_NO_BACKING;
- error_setg(errp, "Could not open backing file: %s",
- error_get_pretty(local_err));
- error_free(local_err);
+ error_prepend(errp, "Could not open backing file: ");
goto free_exit;
}
@@ -1333,12 +1331,11 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
opts = qemu_opts_create(bdrv_qcow2.create_opts, NULL, 0,
&error_abort);
qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, &error_abort);
- ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, &local_err);
+ ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, errp);
qemu_opts_del(opts);
if (ret < 0) {
- error_setg(errp, "Could not create temporary overlay '%s': %s",
- tmp_filename, error_get_pretty(local_err));
- error_free(local_err);
+ error_prepend(errp, "Could not create temporary overlay '%s': ",
+ tmp_filename);
goto out;
}
@@ -3482,9 +3479,9 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
if (!QLIST_EMPTY(&bs->op_blockers[op])) {
blocker = QLIST_FIRST(&bs->op_blockers[op]);
if (errp) {
- error_setg(errp, "Node '%s' is busy: %s",
- bdrv_get_device_or_node_name(bs),
- error_get_pretty(blocker->reason));
+ *errp = error_copy(blocker->reason);
+ error_prepend(errp, "Node '%s' is busy: ",
+ bdrv_get_device_or_node_name(bs));
}
return true;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 88f56c8..3c3f301 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1716,9 +1716,8 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
ret = qcow2_open(bs, options, flags, &local_err);
QDECREF(options);
if (local_err) {
- error_setg(errp, "Could not reopen qcow2 layer: %s",
- error_get_pretty(local_err));
- error_free(local_err);
+ error_propagate(errp, local_err);
+ error_prepend(errp, "Could not reopen qcow2 layer: ");
return;
} else if (ret < 0) {
error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
diff --git a/block/qed.c b/block/qed.c
index 9b88895..31f4cc9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1611,9 +1611,8 @@ static void bdrv_qed_invalidate_cache(BlockDriverState *bs, Error **errp)
memset(s, 0, sizeof(BDRVQEDState));
ret = bdrv_qed_open(bs, NULL, bs->open_flags, &local_err);
if (local_err) {
- error_setg(errp, "Could not reopen qed layer: %s",
- error_get_pretty(local_err));
- error_free(local_err);
+ error_propagate(errp, local_err);
+ error_prepend(errp, "Could not reopen qed layer: ");
return;
} else if (ret < 0) {
error_setg_errno(errp, -ret, "Could not reopen qed layer");
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index c57f293..df77ba8 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -142,7 +142,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
Error **errp)
{
VirtIOBlockDataPlane *s;
- Error *local_err = NULL;
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
@@ -163,11 +162,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
/* If dataplane is (re-)enabled while the guest is running there could be
* block jobs that can conflict.
*/
- if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE,
- &local_err)) {
- error_setg(errp, "cannot start dataplane thread: %s",
- error_get_pretty(local_err));
- error_free(local_err);
+ if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
+ error_prepend(errp, "cannot start dataplane thread: ");
return;
}
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 00cdac6..7bc8288 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -217,11 +217,9 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
}
if (vs->conf.vhostfd) {
- vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, &err);
+ vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, errp);
if (vhostfd == -1) {
- error_setg(errp, "vhost-scsi: unable to parse vhostfd: %s",
- error_get_pretty(err));
- error_free(err);
+ error_prepend(errp, "vhost-scsi: unable to parse vhostfd: ");
return;
}
} else {
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 26ab67f..1bbe930 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -329,9 +329,9 @@ static USBDevice *usb_try_create_simple(USBBus *bus, const char *name,
}
object_property_set_bool(OBJECT(dev), true, "realized", &err);
if (err) {
- error_setg(errp, "Failed to initialize USB device '%s': %s",
- name, error_get_pretty(err));
- error_free(err);
+ error_propagate(errp, err);
+ error_prepend(errp, "Failed to initialize USB device '%s': ",
+ name);
object_unparent(OBJECT(dev));
return NULL;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 13/23] spapr: Use error_reportf_err()
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (11 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 12/23] error: Use error_prepend() " Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:44 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 14/23] migration: Use error_reportf_err() instead of monitor_printf() Markus Armbruster
` (10 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Not caught by Coccinelle for the previous patch, because it reports
the error only conditionally.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ppc/spapr.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index addb9fc..440b56e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -122,10 +122,11 @@ static XICSState *xics_system_init(MachineState *machine,
icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs, &err);
}
if (machine_kernel_irqchip_required(machine) && !icp) {
- error_report("kernel_irqchip requested but unavailable: %s",
- error_get_pretty(err));
+ error_reportf_err(err,
+ "kernel_irqchip requested but unavailable: ");
+ } else {
+ error_free(err);
}
- error_free(err);
}
if (!icp) {
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 14/23] migration: Use error_reportf_err() instead of monitor_printf()
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (12 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 13/23] spapr: Use error_reportf_err() Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:46 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 15/23] qemu-io qemu-nbd: Use error_report() etc. instead of fprintf() Markus Armbruster
` (9 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Using error_reportf_err() instead of monitor_printf() makes no
difference when monitor_printf() is used correctly, i.e. within an HMP
monitor. Elsewhere, monitor_printf() does nothing, while
error_reportf_err() reports to stderr.
Both changed functions are HMP command handlers. These should only
run within an HMP monitor.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
migration/savevm.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index e277b72..bcaeb70 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1927,10 +1927,9 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
/* Delete old snapshots of the same name */
if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
- monitor_printf(mon,
- "Error while deleting snapshot on device '%s': %s\n",
- bdrv_get_device_name(bs1), error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err,
+ "Error while deleting snapshot on device '%s': ",
+ bdrv_get_device_name(bs1));
return;
}
@@ -2108,10 +2107,9 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
const char *name = qdict_get_str(qdict, "name");
if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
- monitor_printf(mon,
- "Error while deleting snapshot on device '%s': %s\n",
- bdrv_get_device_name(bs), error_get_pretty(err));
- error_free(err);
+ error_reportf_err(err,
+ "Error while deleting snapshot on device '%s': ",
+ bdrv_get_device_name(bs));
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 15/23] qemu-io qemu-nbd: Use error_report() etc. instead of fprintf()
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (13 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 14/23] migration: Use error_reportf_err() instead of monitor_printf() Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:51 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 16/23] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
` (8 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Just three instances left.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-io.c | 8 +++-----
qemu-nbd.c | 2 +-
tests/qemu-iotests/059.out | 8 ++++----
tests/qemu-iotests/060.out | 2 +-
tests/qemu-iotests/069.out | 2 +-
tests/qemu-iotests/070.out | 2 +-
tests/qemu-iotests/075.out | 14 +++++++-------
tests/qemu-iotests/076.out | 6 +++---
tests/qemu-iotests/078.out | 12 ++++++------
tests/qemu-iotests/080.out | 36 ++++++++++++++++++------------------
tests/qemu-iotests/083.out | 34 +++++++++++++++++-----------------
tests/qemu-iotests/088.out | 12 ++++++------
tests/qemu-iotests/092.out | 24 ++++++++++++------------
tests/qemu-iotests/103.out | 8 ++++----
tests/qemu-iotests/114.out | 2 +-
tests/qemu-iotests/116.out | 14 +++++++-------
tests/qemu-iotests/131.out | 2 +-
17 files changed, 93 insertions(+), 95 deletions(-)
diff --git a/qemu-io.c b/qemu-io.c
index 269f17c..d47228a 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -57,17 +57,15 @@ static int openfile(char *name, int flags, QDict *opts)
BlockDriverState *bs;
if (qemuio_blk) {
- fprintf(stderr, "file open already, try 'help close'\n");
+ error_report("file open already, try 'help close'");
QDECREF(opts);
return 1;
}
qemuio_blk = blk_new_open("hda", name, NULL, opts, flags, &local_err);
if (!qemuio_blk) {
- fprintf(stderr, "%s: can't open%s%s: %s\n", progname,
- name ? " device " : "", name ?: "",
- error_get_pretty(local_err));
- error_free(local_err);
+ error_reportf_err(local_err, "can't open%s%s: ",
+ name ? " device " : "", name ?: "");
return 1;
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 390852f..6521b14 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -257,7 +257,7 @@ static void *nbd_client_thread(void *arg)
fd = open(device, O_RDWR);
if (fd < 0) {
/* Linux-only, we can use %m in printf. */
- fprintf(stderr, "Failed to open %s: %m\n", device);
+ error_report("Failed to open %s: %m", device);
goto out_socket;
}
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 00057fe..d28df5b 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2,17 +2,17 @@ QA output created by 059
=== Testing invalid granularity ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.vmdk: Invalid granularity, image may be corrupt
+can't open device TEST_DIR/t.vmdk: Invalid granularity, image may be corrupt
no file open, try 'help open'
=== Testing too big L2 table size ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.vmdk: L2 table size too big
+can't open device TEST_DIR/t.vmdk: L2 table size too big
no file open, try 'help open'
=== Testing too big L1 table size ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.vmdk: L1 size too big
+can't open device TEST_DIR/t.vmdk: L1 size too big
no file open, try 'help open'
=== Testing monolithicFlat creation and opening ===
@@ -2055,7 +2055,7 @@ wrote 512/512 bytes at offset 10240
=== Testing monolithicFlat with internally generated JSON file name ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 subformat=monolithicFlat
-qemu-io: can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "read_aio"}'
+can't open: Cannot use relative extent paths with VMDK descriptor file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "read_aio"}'
=== Testing version 3 ===
image: TEST_DIR/iotest-version3.IMGFMT
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 7511189..5d40206 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -20,7 +20,7 @@ Format specific information:
lazy refcounts: false
refcount bits: 16
corrupt: true
-qemu-io: can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write
+can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write
read 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
diff --git a/tests/qemu-iotests/069.out b/tests/qemu-iotests/069.out
index c78e8c2..f975856 100644
--- a/tests/qemu-iotests/069.out
+++ b/tests/qemu-iotests/069.out
@@ -4,5 +4,5 @@ QA output created by 069
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=131072
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 backing_file=TEST_DIR/t.IMGFMT.base
-qemu-io: can't open device TEST_DIR/t.IMGFMT: Could not open backing file: Could not open 'TEST_DIR/t.IMGFMT.base': No such file or directory
+can't open device TEST_DIR/t.IMGFMT: Could not open backing file: Could not open 'TEST_DIR/t.IMGFMT.base': No such file or directory
*** done
diff --git a/tests/qemu-iotests/070.out b/tests/qemu-iotests/070.out
index ca74383..ffd4251 100644
--- a/tests/qemu-iotests/070.out
+++ b/tests/qemu-iotests/070.out
@@ -1,7 +1,7 @@
QA output created by 070
=== Verify open image read-only fails, due to dirty log ===
-qemu-io: can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log that needs to be replayed. To replay the log, execute:
+can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log that needs to be replayed. To replay the log, execute:
qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx': Operation not permitted
no file open, try 'help open'
=== Verify open image replays log ===
diff --git a/tests/qemu-iotests/075.out b/tests/qemu-iotests/075.out
index 5f1d6c1..87beae4 100644
--- a/tests/qemu-iotests/075.out
+++ b/tests/qemu-iotests/075.out
@@ -9,30 +9,30 @@ read 512/512 bytes at offset 1048064
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== block_size must be a multiple of 512 ==
-qemu-io: can't open device TEST_DIR/simple-pattern.cloop: block_size 513 must be a multiple of 512
+can't open device TEST_DIR/simple-pattern.cloop: block_size 513 must be a multiple of 512
no file open, try 'help open'
== block_size cannot be zero ==
-qemu-io: can't open device TEST_DIR/simple-pattern.cloop: block_size cannot be zero
+can't open device TEST_DIR/simple-pattern.cloop: block_size cannot be zero
no file open, try 'help open'
== huge block_size ===
-qemu-io: can't open device TEST_DIR/simple-pattern.cloop: block_size 4294966784 must be 64 MB or less
+can't open device TEST_DIR/simple-pattern.cloop: block_size 4294966784 must be 64 MB or less
no file open, try 'help open'
== offsets_size overflow ===
-qemu-io: can't open device TEST_DIR/simple-pattern.cloop: n_blocks 4294967295 must be 536870911 or less
+can't open device TEST_DIR/simple-pattern.cloop: n_blocks 4294967295 must be 536870911 or less
no file open, try 'help open'
== refuse images that require too many offsets ===
-qemu-io: can't open device TEST_DIR/simple-pattern.cloop: image requires too many offsets, try increasing block size
+can't open device TEST_DIR/simple-pattern.cloop: image requires too many offsets, try increasing block size
no file open, try 'help open'
== refuse images with non-monotonically increasing offsets ==
-qemu-io: can't open device TEST_DIR/simple-pattern.cloop: offsets not monotonically increasing at index 1, image file is corrupt
+can't open device TEST_DIR/simple-pattern.cloop: offsets not monotonically increasing at index 1, image file is corrupt
no file open, try 'help open'
== refuse images with invalid compressed block size ==
-qemu-io: can't open device TEST_DIR/simple-pattern.cloop: invalid compressed block size at index 1, image file is corrupt
+can't open device TEST_DIR/simple-pattern.cloop: invalid compressed block size at index 1, image file is corrupt
no file open, try 'help open'
*** done
diff --git a/tests/qemu-iotests/076.out b/tests/qemu-iotests/076.out
index b0000ae..72645b2 100644
--- a/tests/qemu-iotests/076.out
+++ b/tests/qemu-iotests/076.out
@@ -5,15 +5,15 @@ read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== Negative catalog size ==
-qemu-io: can't open device TEST_DIR/parallels-v1: Catalog too large
+can't open device TEST_DIR/parallels-v1: Catalog too large
no file open, try 'help open'
== Overflow in catalog allocation ==
-qemu-io: can't open device TEST_DIR/parallels-v1: Catalog too large
+can't open device TEST_DIR/parallels-v1: Catalog too large
no file open, try 'help open'
== Zero sectors per track ==
-qemu-io: can't open device TEST_DIR/parallels-v1: Invalid image: Zero sectors per track
+can't open device TEST_DIR/parallels-v1: Invalid image: Zero sectors per track
no file open, try 'help open'
== Read from a valid v2 image ==
diff --git a/tests/qemu-iotests/078.out b/tests/qemu-iotests/078.out
index ca18d2e..42b8a83 100644
--- a/tests/qemu-iotests/078.out
+++ b/tests/qemu-iotests/078.out
@@ -5,24 +5,24 @@ read 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== Negative catalog size ==
-qemu-io: can't open device TEST_DIR/empty.bochs: Catalog size is too large
+can't open device TEST_DIR/empty.bochs: Catalog size is too large
no file open, try 'help open'
== Overflow for catalog size * sizeof(uint32_t) ==
-qemu-io: can't open device TEST_DIR/empty.bochs: Catalog size is too large
+can't open device TEST_DIR/empty.bochs: Catalog size is too large
no file open, try 'help open'
== Too small catalog bitmap for image size ==
-qemu-io: can't open device TEST_DIR/empty.bochs: Catalog size is too small for this disk size
+can't open device TEST_DIR/empty.bochs: Catalog size is too small for this disk size
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/empty.bochs: Catalog size is too small for this disk size
+can't open device TEST_DIR/empty.bochs: Catalog size is too small for this disk size
no file open, try 'help open'
== Negative extent size ==
-qemu-io: can't open device TEST_DIR/empty.bochs: Extent size 2147483648 is too large
+can't open device TEST_DIR/empty.bochs: Extent size 2147483648 is too large
no file open, try 'help open'
== Zero extent size ==
-qemu-io: can't open device TEST_DIR/empty.bochs: Extent size must be at least 512
+can't open device TEST_DIR/empty.bochs: Extent size must be at least 512
no file open, try 'help open'
*** done
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 6061d84..0daac48 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -2,46 +2,46 @@ QA output created by 080
== Huge header size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow2: qcow2 header exceeds cluster size
+can't open device TEST_DIR/t.qcow2: qcow2 header exceeds cluster size
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: qcow2 header exceeds cluster size
+can't open device TEST_DIR/t.qcow2: qcow2 header exceeds cluster size
no file open, try 'help open'
== Huge unknown header extension ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow2: Invalid backing file offset
+can't open device TEST_DIR/t.qcow2: Invalid backing file offset
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Header extension too large
+can't open device TEST_DIR/t.qcow2: Header extension too large
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Header extension too large
+can't open device TEST_DIR/t.qcow2: Header extension too large
no file open, try 'help open'
== Huge refcount table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow2: Reference count table too large
+can't open device TEST_DIR/t.qcow2: Reference count table too large
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Reference count table too large
+can't open device TEST_DIR/t.qcow2: Reference count table too large
no file open, try 'help open'
== Misaligned refcount table ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow2: Invalid reference count table offset
+can't open device TEST_DIR/t.qcow2: Invalid reference count table offset
no file open, try 'help open'
== Huge refcount offset ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow2: Invalid reference count table offset
+can't open device TEST_DIR/t.qcow2: Invalid reference count table offset
no file open, try 'help open'
== Invalid snapshot table ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow2: Too many snapshots
+can't open device TEST_DIR/t.qcow2: Too many snapshots
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Too many snapshots
+can't open device TEST_DIR/t.qcow2: Too many snapshots
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Invalid snapshot table offset
+can't open device TEST_DIR/t.qcow2: Invalid snapshot table offset
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Invalid snapshot table offset
+can't open device TEST_DIR/t.qcow2: Invalid snapshot table offset
no file open, try 'help open'
== Hitting snapshot table size limit ==
@@ -52,13 +52,13 @@ read 512/512 bytes at offset 0
== Invalid L1 table ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow2: Active L1 table too large
+can't open device TEST_DIR/t.qcow2: Active L1 table too large
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Active L1 table too large
+can't open device TEST_DIR/t.qcow2: Active L1 table too large
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Invalid L1 table offset
+can't open device TEST_DIR/t.qcow2: Invalid L1 table offset
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow2: Invalid L1 table offset
+can't open device TEST_DIR/t.qcow2: Invalid L1 table offset
no file open, try 'help open'
== Invalid L1 table (with internal snapshot in the image) ==
@@ -67,7 +67,7 @@ qemu-img: Could not open 'TEST_DIR/t.IMGFMT': L1 table is too small
== Invalid backing file size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow2: Backing file name too long
+can't open device TEST_DIR/t.qcow2: Backing file name too long
no file open, try 'help open'
== Invalid L2 entry (huge physical offset) ==
diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index 8c1441b..78cc49a 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -1,52 +1,52 @@
QA output created by 083
=== Check disconnect before neg1 ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect after neg1 ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect 8 neg1 ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect 16 neg1 ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect before export ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect after export ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect 4 export ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect 12 export ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect 16 export ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect before neg2 ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect after neg2 ===
@@ -56,12 +56,12 @@ read failed: Input/output error
=== Check disconnect 8 neg2 ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect 10 neg2 ===
-qemu-io: can't open device nbd:127.0.0.1:PORT:exportname=foo
+can't open device nbd:127.0.0.1:PORT:exportname=foo
no file open, try 'help open'
=== Check disconnect before request ===
@@ -107,27 +107,27 @@ read 512/512 bytes at offset 0
=== Check disconnect before neg-classic ===
-qemu-io: can't open device nbd:127.0.0.1:PORT
+can't open device nbd:127.0.0.1:PORT
no file open, try 'help open'
=== Check disconnect 8 neg-classic ===
-qemu-io: can't open device nbd:127.0.0.1:PORT
+can't open device nbd:127.0.0.1:PORT
no file open, try 'help open'
=== Check disconnect 16 neg-classic ===
-qemu-io: can't open device nbd:127.0.0.1:PORT
+can't open device nbd:127.0.0.1:PORT
no file open, try 'help open'
=== Check disconnect 24 neg-classic ===
-qemu-io: can't open device nbd:127.0.0.1:PORT
+can't open device nbd:127.0.0.1:PORT
no file open, try 'help open'
=== Check disconnect 28 neg-classic ===
-qemu-io: can't open device nbd:127.0.0.1:PORT
+can't open device nbd:127.0.0.1:PORT
no file open, try 'help open'
=== Check disconnect after neg-classic ===
diff --git a/tests/qemu-iotests/088.out b/tests/qemu-iotests/088.out
index 6e6bfca..a2a83b8 100644
--- a/tests/qemu-iotests/088.out
+++ b/tests/qemu-iotests/088.out
@@ -2,16 +2,16 @@ QA output created by 088
== Invalid block size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.vpc: Invalid block size 0
+can't open device TEST_DIR/t.vpc: Invalid block size 0
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.vpc: Invalid block size 0
+can't open device TEST_DIR/t.vpc: Invalid block size 0
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.vpc: Invalid block size 128
+can't open device TEST_DIR/t.vpc: Invalid block size 128
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.vpc: Invalid block size 128
+can't open device TEST_DIR/t.vpc: Invalid block size 128
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.vpc: Invalid block size 305419896
+can't open device TEST_DIR/t.vpc: Invalid block size 305419896
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.vpc: Invalid block size 305419896
+can't open device TEST_DIR/t.vpc: Invalid block size 305419896
no file open, try 'help open'
*** done
diff --git a/tests/qemu-iotests/092.out b/tests/qemu-iotests/092.out
index c5c60f9..e18f54c 100644
--- a/tests/qemu-iotests/092.out
+++ b/tests/qemu-iotests/092.out
@@ -2,37 +2,37 @@ QA output created by 092
== Invalid cluster size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
+can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
+can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
+can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
+can't open device TEST_DIR/t.qcow: Cluster size must be between 512 and 64k
no file open, try 'help open'
== Invalid L2 table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
+can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
+can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
+can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
+can't open device TEST_DIR/t.qcow: L2 table size must be between 512 and 64k
no file open, try 'help open'
== Invalid size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow: Image too large
+can't open device TEST_DIR/t.qcow: Image too large
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow: Image too large
+can't open device TEST_DIR/t.qcow: Image too large
no file open, try 'help open'
== Invalid backing file length ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-io: can't open device TEST_DIR/t.qcow: Backing file name too long
+can't open device TEST_DIR/t.qcow: Backing file name too long
no file open, try 'help open'
-qemu-io: can't open device TEST_DIR/t.qcow: Backing file name too long
+can't open device TEST_DIR/t.qcow: Backing file name too long
no file open, try 'help open'
*** done
diff --git a/tests/qemu-iotests/103.out b/tests/qemu-iotests/103.out
index d05f49f..b7aaadf 100644
--- a/tests/qemu-iotests/103.out
+++ b/tests/qemu-iotests/103.out
@@ -5,10 +5,10 @@ wrote 65536/65536 bytes at offset 0
=== Testing invalid option combinations ===
-qemu-io: can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
-qemu-io: can't open device TEST_DIR/t.IMGFMT: l2-cache-size may not exceed cache-size
-qemu-io: can't open device TEST_DIR/t.IMGFMT: refcount-cache-size may not exceed cache-size
-qemu-io: can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+can't open device TEST_DIR/t.IMGFMT: l2-cache-size may not exceed cache-size
+can't open device TEST_DIR/t.IMGFMT: refcount-cache-size may not exceed cache-size
+can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and refcount-cache-size may not be set the same time
=== Testing valid option combinations ===
diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out
index 6a2c750..b6d10e4 100644
--- a/tests/qemu-iotests/114.out
+++ b/tests/qemu-iotests/114.out
@@ -7,7 +7,7 @@ virtual size: 64M (67108864 bytes)
cluster_size: 65536
backing file: TEST_DIR/t.IMGFMT.base
backing file format: foo
-qemu-io: can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknown driver 'foo'
+can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknown driver 'foo'
read 4096/4096 bytes at offset 0
4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
*** done
diff --git a/tests/qemu-iotests/116.out b/tests/qemu-iotests/116.out
index b679cee..1f11d44 100644
--- a/tests/qemu-iotests/116.out
+++ b/tests/qemu-iotests/116.out
@@ -2,36 +2,36 @@ QA output created by 116
== truncated header cluster ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
no file open, try 'help open'
== invalid header magic ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Image not in QED format
+can't open device TEST_DIR/t.qed: Image not in QED format
no file open, try 'help open'
== invalid cluster size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
no file open, try 'help open'
== invalid table size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
no file open, try 'help open'
== invalid header size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
no file open, try 'help open'
== invalid L1 table offset ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
no file open, try 'help open'
== invalid image size ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
no file open, try 'help open'
*** done
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index 021a04c..ae2412e 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -22,7 +22,7 @@ read 32768/32768 bytes at offset 163840
read 32768/32768 bytes at offset 0
32 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== Corrupt image ==
-qemu-io: can't open device TEST_DIR/t.parallels: parallels: Image was not closed correctly; cannot be opened read/write
+can't open device TEST_DIR/t.parallels: parallels: Image was not closed correctly; cannot be opened read/write
no file open, try 'help open'
ERROR image was not closed correctly
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 16/23] error: Strip trailing '\n' from error string arguments (again)
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (14 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 15/23] qemu-io qemu-nbd: Use error_report() etc. instead of fprintf() Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 20:57 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit Markus Armbruster
` (7 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, zhanghailiang, Stefan Berger, Markus Armbruster,
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@pond.sub.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Acked-by: Fam Zheng <famz@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 440b56e..3bfb957 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1833,7 +1833,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 4af1558..f2b732e 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -191,8 +191,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;
}
@@ -211,8 +211,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;
}
@@ -256,7 +256,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;
}
@@ -276,7 +276,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;
}
@@ -314,7 +314,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;
}
@@ -326,7 +326,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 0490f00..5bfcca3 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2323,7 +2323,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 bcaeb70..540eb28 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] 66+ messages in thread
* [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (15 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 16/23] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 21:00 ` Eric Blake
2015-12-18 0:48 ` Fam Zheng
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message Markus Armbruster
` (6 subsequent siblings)
23 siblings, 2 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Fam Zheng
Cc: Fam Zheng <famz@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/vmdk.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index b4a224e..08fa3f3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -760,6 +760,17 @@ static int vmdk_open_sparse(BlockDriverState *bs, BdrvChild *file, int flags,
}
}
+static const char *next_line(const char *s)
+{
+ while (*s) {
+ if (*s == '\n') {
+ return s + 1;
+ }
+ s++;
+ }
+ return s;
+}
+
static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
const char *desc_file_path, QDict *options,
Error **errp)
@@ -769,7 +780,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
char access[11];
char type[11];
char fname[512];
- const char *p = desc;
+ const char *p;
int64_t sectors = 0;
int64_t flat_offset;
char *extent_path;
@@ -779,7 +790,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
char extent_opt_prefix[32];
Error *local_err = NULL;
- while (*p) {
+ for (p = desc; *p; p = next_line(p)) {
/* parse extent line in one of below formats:
*
* RW [size in sectors] FLAT "file-name.vmdk" OFFSET
@@ -791,7 +802,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
matches = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
access, §ors, type, fname, &flat_offset);
if (matches < 4 || strcmp(access, "RW")) {
- goto next_line;
+ continue;
} else if (!strcmp(type, "FLAT")) {
if (matches != 5 || flat_offset < 0) {
error_setg(errp, "Invalid extent lines: \n%s", p);
@@ -813,7 +824,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
(strcmp(type, "FLAT") && strcmp(type, "SPARSE") &&
strcmp(type, "VMFS") && strcmp(type, "VMFSSPARSE")) ||
(strcmp(access, "RW"))) {
- goto next_line;
+ continue;
}
if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
@@ -870,15 +881,6 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
return -ENOTSUP;
}
extent->type = g_strdup(type);
-next_line:
- /* move to next line */
- while (*p) {
- if (*p == '\n') {
- p++;
- break;
- }
- p++;
- }
}
return 0;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (16 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit Markus Armbruster
@ 2015-12-17 16:49 ` Markus Armbruster
2015-12-17 21:09 ` Eric Blake
2015-12-18 0:52 ` Fam Zheng
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 19/23] pci-assign: Clean up "Failed to assign" error messages Markus Armbruster
` (5 subsequent siblings)
23 siblings, 2 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Fam Zheng
vmdk_parse_extents() reports parse errors like this:
error_setg(errp, "Invalid extent lines:\n%s", p);
where p points to the beginning of the malformed line in the image
descriptor. This results in a multi-line error message
Invalid extent lines:
<first line that doesn't parse>
<remaining text that may or may not parse, if any>
Error messages should not have newlines embedded. Since the remaining
text is not helpful, we can simply report:
Invalid extent line: <first line that doesn't parse>
Cc: Fam Zheng <famz@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/vmdk.c | 19 ++++++++++++-------
tests/qemu-iotests/059.out | 4 +---
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 08fa3f3..04b47e3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -780,7 +780,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
char access[11];
char type[11];
char fname[512];
- const char *p;
+ const char *p, *np;
int64_t sectors = 0;
int64_t flat_offset;
char *extent_path;
@@ -805,19 +805,16 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
continue;
} else if (!strcmp(type, "FLAT")) {
if (matches != 5 || flat_offset < 0) {
- error_setg(errp, "Invalid extent lines: \n%s", p);
- return -EINVAL;
+ goto invalid;
}
} else if (!strcmp(type, "VMFS")) {
if (matches == 4) {
flat_offset = 0;
} else {
- error_setg(errp, "Invalid extent lines:\n%s", p);
- return -EINVAL;
+ goto invalid;
}
} else if (matches != 4) {
- error_setg(errp, "Invalid extent lines:\n%s", p);
- return -EINVAL;
+ goto invalid;
}
if (sectors <= 0 ||
@@ -883,6 +880,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
extent->type = g_strdup(type);
}
return 0;
+
+invalid:
+ np = next_line(p);
+ assert(np != p);
+ if (np[-1] == '\n')
+ np--;
+ error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
+ return -EINVAL;
}
static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index d28df5b..9d506cb 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2038,9 +2038,7 @@ Format specific information:
format: FLAT
=== Testing malformed VMFS extent description line ===
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent lines:
-RW 12582912 VMFS "dummy.IMGFMT" 1
-
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 12582912 VMFS "dummy.IMGFMT" 1
=== Testing truncated sparse ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400 subformat=monolithicSparse
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 19/23] pci-assign: Clean up "Failed to assign" error messages
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (17 preceding siblings ...)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message Markus Armbruster
@ 2015-12-17 16:50 ` Markus Armbruster
2015-12-17 21:45 ` Eric Blake
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 20/23] vhdx: Fix "log that needs to be replayed" error message Markus Armbruster
` (4 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:50 UTC (permalink / raw)
To: qemu-devel; +Cc: Laszlo Ersek
The arguments of error_setg() & friends should yield a short error
string without newlines.
Two 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().
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/i386/kvm/pci-assign.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 0fd6923..eec1340 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -770,7 +770,7 @@ static char *assign_failed_examine(const AssignedDevice *dev)
"*** $ echo \"%04x:%02x:%02x.%x\" > /sys/bus/pci/drivers/"
"pci-stub/bind\n"
"*** $ echo \"%04x %04x\" > /sys/bus/pci/drivers/pci-stub/remove_id\n"
- "***",
+ "***\n",
ns, dev->host.domain, dev->host.bus, dev->host.slot,
dev->host.function, vendor_id, device_id,
dev->host.domain, dev->host.bus, dev->host.slot, dev->host.function,
@@ -778,7 +778,7 @@ static char *assign_failed_examine(const AssignedDevice *dev)
dev->host.function, vendor_id, device_id);
fail:
- return g_strdup("Couldn't find out why.");
+ return g_strdup("Couldn't find out why.\n");
}
static void assign_device(AssignedDevice *dev, Error **errp)
@@ -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?\n");
return r;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 20/23] vhdx: Fix "log that needs to be replayed" error message
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (18 preceding siblings ...)
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 19/23] pci-assign: Clean up "Failed to assign" error messages Markus Armbruster
@ 2015-12-17 16:50 ` Markus Armbruster
2015-12-17 21:52 ` Eric Blake
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 21/23] error: Clean up errors with embedded newlines (again) Markus Armbruster
` (3 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:50 UTC (permalink / raw)
To: qemu-devel
The arguments of error_setg_errno() should yield a short error string
without newlines.
Here, we try to append additional help to the error message by
embedding newlines in the error string. That's nice, but it's doesn't
play nicely with the errno part. tests/qemu-iotests/070.out shows the
resulting mess:
can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log that needs to be replayed. To replay the log, execute:
qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx': Operation not permitted
Switch to error_setg() and error_append_hint(). Result:
can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log that needs to be replayed
To replay the log, run:
qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx'
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/vhdx-log.c | 13 +++++++------
tests/qemu-iotests/070.out | 5 +++--
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 47ae4b1..ab86416 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -784,12 +784,13 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
if (logs.valid) {
if (bs->read_only) {
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);
+ error_setg(errp,
+ "VHDX image file '%s' opened read-only, but "
+ "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/tests/qemu-iotests/070.out b/tests/qemu-iotests/070.out
index ffd4251..131a5b1 100644
--- a/tests/qemu-iotests/070.out
+++ b/tests/qemu-iotests/070.out
@@ -1,8 +1,9 @@
QA output created by 070
=== Verify open image read-only fails, due to dirty log ===
-can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log that needs to be replayed. To replay the log, execute:
- qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx': Operation not permitted
+can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log that needs to be replayed
+To replay the log, run:
+qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx'
no file open, try 'help open'
=== Verify open image replays log ===
read 18874368/18874368 bytes at offset 0
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* [Qemu-devel] [PATCH v2 21/23] error: Clean up errors with embedded newlines (again)
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (19 preceding siblings ...)
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 20/23] vhdx: Fix "log that needs to be replayed" error message Markus Armbruster
@ 2015-12-17 16:50 ` Markus Armbruster
2015-12-17 21:53 ` Eric Blake
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 22/23] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
` (2 subsequent siblings)
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:50 UTC (permalink / raw)
To: qemu-devel; +Cc: Pavel Fedin, Markus Armbruster, 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 coming
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@pond.sub.org>
Reviewed-by: Laszlo Ersek <lersek@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 112361a..b5caa06 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] 66+ messages in thread
* [Qemu-devel] [PATCH v2 22/23] hw/s390x: Rename local variables Error *l_err to just err
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (20 preceding siblings ...)
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 21/23] error: Clean up errors with embedded newlines (again) Markus Armbruster
@ 2015-12-17 16:50 ` Markus Armbruster
2015-12-17 21:54 ` Eric Blake
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize() Markus Armbruster
2015-12-17 17:26 ` [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Cornelia Huck
23 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:50 UTC (permalink / raw)
To: qemu-devel; +Cc: David Hildenbrand, 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@pond.sub.org>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.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] 66+ messages in thread
* [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize()
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (21 preceding siblings ...)
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 22/23] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
@ 2015-12-17 16:50 ` Markus Armbruster
2015-12-17 17:25 ` Cornelia Huck
2015-12-17 21:54 ` Eric Blake
2015-12-17 17:26 ` [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Cornelia Huck
23 siblings, 2 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 16:50 UTC (permalink / raw)
To: qemu-devel
Suggested-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
hw/s390x/sclp.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
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);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint() Markus Armbruster
@ 2015-12-17 16:59 ` Eric Blake
2015-12-17 18:04 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 16:59 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3835 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> While there, tighten its assertion.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/error.h | 19 +++++++++++++++++--
> util/error.c | 2 +-
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b2362a5..048d947 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -18,6 +18,15 @@
> * Create an error:
> * error_setg(&err, "situation normal, all fouled up");
> *
> + * Create an error and add additional explanation:
> + * error_setg(&err, "invalid quark");
> + * error_append_hint(&err, "Valid quarks are up, down, strange, "
> + * "charm, top, bottom\n");
Do we want to allow/encourage a trailing dot in the hint? I'm fine if
we don't - but once we document its usage, we should probably then be
consistent with what we document; qemu-option.c used a trailing dot,
qdev-monitor.c did not.
> + *
> + * Do *not* contract this to
> + * error_setg(&err, "invalid quark\n"
> + * "Valid quarks are up, down, strange, charm, top, bottom");
> + *
> * Report an error to stderr:
> * error_report_err(err);
> * This frees the error object.
> @@ -26,6 +35,7 @@
> * const char *msg = error_get_pretty(err);
> * do with msg what needs to be done...
> * error_free(err);
> + * Note that this loses hints added with error_append_hint().
> *
> * Handle an error without reporting it (just for completeness):
> * error_free(err);
> @@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err);
> * If @errp is anything else, *@errp must be NULL.
> * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
> * human-readable error message is made from printf-style @fmt, ...
> + * The resulting message should not contain newlines.
Should we also discourage trailing punctuation?
Should we also mention no need for leading 'error: ' prefix?
> */
> #define error_setg(errp, fmt, ...) \
> error_setg_internal((errp), __FILE__, __LINE__, __func__, \
> @@ -184,7 +195,11 @@ void error_propagate(Error **dst_errp, Error *local_err);
>
> /**
> * Append a printf-style human-readable explanation to an existing error.
> - * May be called multiple times, and safe if @errp is NULL.
> + * @errp may be NULL, but neither &error_fatal nor &error_abort.
As a native speaker, 'not' sounds better than 'neither' in that
sentence. But I think both choices are grammatically correct.
> + * Trivially the case if you call it only after error_setg() or
> + * error_propagate().
> + * May be called multiple times. The resulting hint should end with a
> + * newline.
> */
> void error_append_hint(Error **errp, const char *fmt, ...)
> GCC_FMT_ATTR(2, 3);
> @@ -218,7 +233,7 @@ void error_free_or_abort(Error **errp);
> /*
> * Convenience function to error_report() and free @err.
> */
> -void error_report_err(Error *);
> +void error_report_err(Error *err);
>
> /*
> * Just like error_setg(), except you get to specify the error class.
> diff --git a/util/error.c b/util/error.c
> index 9b27c45..ebfb74b 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -132,7 +132,7 @@ void error_append_hint(Error **errp, const char *fmt, ...)
> return;
> }
> err = *errp;
> - assert(err && errp != &error_abort);
> + assert(err && errp != &error_abort && errp != &error_fatal);
Otherwise looks reasonable.
>
> if (!err->hint) {
> err->hint = g_string_new(NULL);
>
--
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report()
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report() Markus Armbruster
@ 2015-12-17 17:17 ` Eric Blake
2015-12-17 18:52 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 17:17 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Coccinelle semantic patch
>
> @@
> expression E;
> expression list ARGS;
> @@
> - errx(E, ARGS);
> + error_report(ARGS);
> + exit(E);
> @@
> expression E, FMT;
> expression list ARGS;
> @@
> - err(E, FMT, ARGS);
> + error_report(FMT /*": %s"*/, ARGS, strerror(errno));
> + exit(E);
>
> followed by a replace of '"/*": %s"*/' by ' : %s"'.
Interesting approach to working around a coccinelle shortcoming.
Hmm. Should we add error_report_errno(), to parallel error_setg_errno()?
But that can be a separate patch.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> qemu-nbd.c | 119 ++++++++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 75 insertions(+), 44 deletions(-)
>
> @@ -491,13 +498,14 @@ int main(int argc, char **argv)
> BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
> &local_err);
> if (local_err) {
> - errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s",
> - error_get_pretty(local_err));
> + error_report("Failed to parse detect_zeroes mode: %s",
> + error_get_pretty(local_err));
> + exit(EXIT_FAILURE);
> }
> if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
> !(flags & BDRV_O_UNMAP)) {
> - errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
> - "without setting discard operation to unmap");
> + error_report("setting detect-zeroes to unmap is not allowed " "without setting discard operation to unmap");
You'll want to fix the line-wrap on this.
> @@ -509,10 +517,12 @@ int main(int argc, char **argv)
> case 'o':
> dev_offset = strtoll (optarg, &end, 0);
> if (*end) {
> - errx(EXIT_FAILURE, "Invalid offset `%s'", optarg);
> + error_report("Invalid offset `%s'", optarg);
Worth cleaning this up to use '' instead of `' at some point in the
series? (Not here, though, since this patch is best when it sticks as
close to the coccinelle script as possible).
> + exit(EXIT_FAILURE);
> }
> if (dev_offset < 0) {
> - errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
> + error_report("Offset must be positive `%s'", optarg);
Obviously, any `' cleanup to '' will have quite a few callers to adjust.
> @@ -534,16 +545,19 @@ int main(int argc, char **argv)
> case 'P':
> partition = strtol(optarg, &end, 0);
> if (*end) {
> - errx(EXIT_FAILURE, "Invalid partition `%s'", optarg);
> + error_report("Invalid partition `%s'", optarg);
> + exit(EXIT_FAILURE);
> }
> if (partition < 1 || partition > 8) {
> - errx(EXIT_FAILURE, "Invalid partition %d", partition);
> + error_report("Invalid partition %d", partition);
> + exit(EXIT_FAILURE);
> }
> break;
> case 'k':
> sockpath = optarg;
> if (sockpath[0] != '/') {
> - errx(EXIT_FAILURE, "socket path must be absolute\n");
> + error_report("socket path must be absolute\n");
I'm guessing later in the series will kill the trailing newline? If so,
then this patch is fine (again, limiting to just coccinelle here).
> + exit(EXIT_FAILURE);
> }
> break;
> case 'd':
> @@ -555,10 +569,12 @@ int main(int argc, char **argv)
> case 'e':
> shared = strtol(optarg, &end, 0);
> if (*end) {
> - errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg);
> + error_report("Invalid shared device number '%s'", optarg);
> + exit(EXIT_FAILURE);
> }
> if (shared < 1) {
> - errx(EXIT_FAILURE, "Shared device number must be greater than 0\n");
> + error_report("Shared device number must be greater than 0\n");
> + exit(EXIT_FAILURE);
And another one. Maybe mention in the commit message any future
cleanups planned for style issues that are exposed by this conversion?
> }
> break;
> case 'f':
> @@ -579,21 +595,23 @@ int main(int argc, char **argv)
> exit(0);
> break;
> case '?':
> - errx(EXIT_FAILURE, "Try `%s --help' for more information.",
> - argv[0]);
> + error_report("Try `%s --help' for more information.", argv[0]);
> + exit(EXIT_FAILURE);
> }
> }
>
> if ((argc - optind) != 1) {
> - errx(EXIT_FAILURE, "Invalid number of argument.\n"
> - "Try `%s --help' for more information.",
> - argv[0]);
> + error_report("Invalid number of argument.\n" "Try `%s --help' for more information.",
> + argv[0]);
Long source line worth wrapping?
Line wraps and commit message improvements seem obvious, so I'm okay
with adding:
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize()
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize() Markus Armbruster
@ 2015-12-17 17:25 ` Cornelia Huck
2015-12-17 21:54 ` Eric Blake
1 sibling, 0 replies; 66+ messages in thread
From: Cornelia Huck @ 2015-12-17 17:25 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On Thu, 17 Dec 2015 17:50:04 +0100
Markus Armbruster <armbru@redhat.com> wrote:
> Suggested-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
> hw/s390x/sclp.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
` (22 preceding siblings ...)
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize() Markus Armbruster
@ 2015-12-17 17:26 ` Cornelia Huck
23 siblings, 0 replies; 66+ messages in thread
From: Cornelia Huck @ 2015-12-17 17:26 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On Thu, 17 Dec 2015 17:49:41 +0100
Markus Armbruster <armbru@redhat.com> wrote:
> I intend to take these patches through my tree.
OK with me for the s390x changes.
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 02/23] error: Use error_report_err() where appropriate (again)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 02/23] error: Use error_report_err() where appropriate (again) Markus Armbruster
@ 2015-12-17 17:33 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 17:33 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 331 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Same Coccinelle semantic patch as in commit 565f65d.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()
2015-12-17 16:59 ` Eric Blake
@ 2015-12-17 18:04 ` Markus Armbruster
2015-12-17 18:16 ` Eric Blake
0 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 18:04 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> While there, tighten its assertion.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/qapi/error.h | 19 +++++++++++++++++--
>> util/error.c | 2 +-
>> 2 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index b2362a5..048d947 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -18,6 +18,15 @@
>> * Create an error:
>> * error_setg(&err, "situation normal, all fouled up");
>> *
>> + * Create an error and add additional explanation:
>> + * error_setg(&err, "invalid quark");
>> + * error_append_hint(&err, "Valid quarks are up, down, strange, "
>> + * "charm, top, bottom\n");
>
> Do we want to allow/encourage a trailing dot in the hint? I'm fine if
> we don't - but once we document its usage, we should probably then be
> consistent with what we document; qemu-option.c used a trailing dot,
> qdev-monitor.c did not.
I'll fix this example.
>> + *
>> + * Do *not* contract this to
>> + * error_setg(&err, "invalid quark\n"
>> + * "Valid quarks are up, down, strange, charm, top, bottom");
>> + *
>> * Report an error to stderr:
>> * error_report_err(err);
>> * This frees the error object.
>> @@ -26,6 +35,7 @@
>> * const char *msg = error_get_pretty(err);
>> * do with msg what needs to be done...
>> * error_free(err);
>> + * Note that this loses hints added with error_append_hint().
>> *
>> * Handle an error without reporting it (just for completeness):
>> * error_free(err);
>> @@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err);
>> * If @errp is anything else, *@errp must be NULL.
>> * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
>> * human-readable error message is made from printf-style @fmt, ...
>> + * The resulting message should not contain newlines.
>
> Should we also discourage trailing punctuation?
Yes. How to best phrase it?
> Should we also mention no need for leading 'error: ' prefix?
Unlike the other anti-patterns, this one doesn't seem frequent in new
code.
>> */
>> #define error_setg(errp, fmt, ...) \
>> error_setg_internal((errp), __FILE__, __LINE__, __func__, \
>> @@ -184,7 +195,11 @@ void error_propagate(Error **dst_errp, Error *local_err);
>>
>> /**
>> * Append a printf-style human-readable explanation to an existing error.
>> - * May be called multiple times, and safe if @errp is NULL.
>> + * @errp may be NULL, but neither &error_fatal nor &error_abort.
>
> As a native speaker, 'not' sounds better than 'neither' in that
> sentence. But I think both choices are grammatically correct.
You mean "not &error_abort or &error_abort"?
>> + * Trivially the case if you call it only after error_setg() or
>> + * error_propagate().
>> + * May be called multiple times. The resulting hint should end with a
>> + * newline.
>> */
>> void error_append_hint(Error **errp, const char *fmt, ...)
>> GCC_FMT_ATTR(2, 3);
>> @@ -218,7 +233,7 @@ void error_free_or_abort(Error **errp);
>> /*
>> * Convenience function to error_report() and free @err.
>> */
>> -void error_report_err(Error *);
>> +void error_report_err(Error *err);
>>
>> /*
>> * Just like error_setg(), except you get to specify the error class.
>> diff --git a/util/error.c b/util/error.c
>> index 9b27c45..ebfb74b 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -132,7 +132,7 @@ void error_append_hint(Error **errp, const char *fmt, ...)
>> return;
>> }
>> err = *errp;
>> - assert(err && errp != &error_abort);
>> + assert(err && errp != &error_abort && errp != &error_fatal);
>
> Otherwise looks reasonable.
>
>>
>> if (!err->hint) {
>> err->hint = g_string_new(NULL);
>>
Thanks!
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 03/23] error: Use error_report_err() instead of monitor_printf()
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 03/23] error: Use error_report_err() instead of monitor_printf() Markus Armbruster
@ 2015-12-17 18:06 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 18:06 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Using error_report_err() instead of monitor_printf() makes no
> difference when monitor_printf() is used correctly, i.e. within an HMP
> monitor. Elsewhere, monitor_printf() does nothing, while
> error_report_err() reports to stderr.
>
> Most changed functions are HMP command handlers. These should only
> run within an HMP monitor. The one exception is bdrv_password_cb(),
> which should also only run within an HMP monitor.
>
> Four command handlers prefix the error message with the command name:
> balloon, migrate_set_capability, migrate_set_parameter, migrate.
> Pointless, drop.
>
> Coccinelle semantic patch:
>
> @@
> expression M, E;
> @@
> - monitor_printf(M, "%s\n", error_get_pretty(E));
> - error_free(E);
> + error_report_err(E);
> @r1@
> expression M, E;
> format F;
> position p;
> @@
> - monitor_printf(M, "...%@F@\n", error_get_pretty(E));@p
> - error_free(E);
> + error_report_err(E);
> @script:python@
> p << r1.p;
> @@
> print "%s:%s:%s: prefix dropped" % (p[0].file, p[0].line, p[0].column)
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hmp.c | 29 +++++++++--------------------
> hw/s390x/s390-skeys.c | 3 +--
> migration/savevm.c | 3 +--
> monitor.c | 6 ++----
> 4 files changed, 13 insertions(+), 28 deletions(-)
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()
2015-12-17 18:04 ` Markus Armbruster
@ 2015-12-17 18:16 ` Eric Blake
2015-12-17 19:00 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 18:16 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2176 bytes --]
On 12/17/2015 11:04 AM, Markus Armbruster wrote:
>>> + * Create an error and add additional explanation:
>>> + * error_setg(&err, "invalid quark");
>>> + * error_append_hint(&err, "Valid quarks are up, down, strange, "
>>> + * "charm, top, bottom\n");
>>
>> Do we want to allow/encourage a trailing dot in the hint? I'm fine if
>> we don't - but once we document its usage, we should probably then be
>> consistent with what we document; qemu-option.c used a trailing dot,
>> qdev-monitor.c did not.
>
> I'll fix this example.
>
>>> + *
>>> + * Do *not* contract this to
>>> + * error_setg(&err, "invalid quark\n"
>>> + * "Valid quarks are up, down, strange, charm, top, bottom");
Of course, if you put a trailing dot above, you'd also want it here in
the 'don't contract' section.
>>> @@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err);
>>> * If @errp is anything else, *@errp must be NULL.
>>> * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
>>> * human-readable error message is made from printf-style @fmt, ...
>>> + * The resulting message should not contain newlines.
>>
>> Should we also discourage trailing punctuation?
>
> Yes. How to best phrase it?
Maybe:
The resulting message should be a single phrase, with no newline or
trailing punctuation.
>
>> Should we also mention no need for leading 'error: ' prefix?
>
> Unlike the other anti-patterns, this one doesn't seem frequent in new
> code.
Fair enough.
>>> /**
>>> * Append a printf-style human-readable explanation to an existing error.
>>> - * May be called multiple times, and safe if @errp is NULL.
>>> + * @errp may be NULL, but neither &error_fatal nor &error_abort.
>>
>> As a native speaker, 'not' sounds better than 'neither' in that
>> sentence. But I think both choices are grammatically correct.
>
> You mean "not &error_abort or &error_abort"?
Yes, that works:
@errp may be NULL, but not &error_fatal or &error_abort.
--
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report()
2015-12-17 17:17 ` Eric Blake
@ 2015-12-17 18:52 ` Markus Armbruster
2015-12-17 19:56 ` Eric Blake
0 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 18:52 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> Coccinelle semantic patch
>>
>> @@
>> expression E;
>> expression list ARGS;
>> @@
>> - errx(E, ARGS);
>> + error_report(ARGS);
>> + exit(E);
>> @@
>> expression E, FMT;
>> expression list ARGS;
>> @@
>> - err(E, FMT, ARGS);
>> + error_report(FMT /*": %s"*/, ARGS, strerror(errno));
>> + exit(E);
>>
>> followed by a replace of '"/*": %s"*/' by ' : %s"'.
>
> Interesting approach to working around a coccinelle shortcoming.
>
> Hmm. Should we add error_report_errno(), to parallel error_setg_errno()?
> But that can be a separate patch.
If we have enough places that profit from it.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> qemu-nbd.c | 119 ++++++++++++++++++++++++++++++++++++++-----------------------
>> 1 file changed, 75 insertions(+), 44 deletions(-)
>>
>
>> @@ -491,13 +498,14 @@ int main(int argc, char **argv)
>> BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
>> &local_err);
>> if (local_err) {
>> - errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s",
>> - error_get_pretty(local_err));
>> + error_report("Failed to parse detect_zeroes mode: %s",
>> + error_get_pretty(local_err));
>> + exit(EXIT_FAILURE);
>> }
>> if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
>> !(flags & BDRV_O_UNMAP)) {
>> - errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
>> - "without setting discard operation to unmap");
>> + error_report("setting detect-zeroes to unmap is not allowed " "without setting discard operation to unmap");
>
> You'll want to fix the line-wrap on this.
Yes. Coccinelle has difficulties with string literal concatenation.
>> @@ -509,10 +517,12 @@ int main(int argc, char **argv)
>> case 'o':
>> dev_offset = strtoll (optarg, &end, 0);
>> if (*end) {
>> - errx(EXIT_FAILURE, "Invalid offset `%s'", optarg);
>> + error_report("Invalid offset `%s'", optarg);
>
> Worth cleaning this up to use '' instead of `' at some point in the
> series? (Not here, though, since this patch is best when it sticks as
> close to the coccinelle script as possible).
I tend to clean it up when I touch the message anyway. Perhaps
wholesale cleanup would be in order, but not in this series.
>> + exit(EXIT_FAILURE);
>> }
>> if (dev_offset < 0) {
>> - errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
>> + error_report("Offset must be positive `%s'", optarg);
>
> Obviously, any `' cleanup to '' will have quite a few callers to adjust.
Quick grep finds about a hundred.
>> @@ -534,16 +545,19 @@ int main(int argc, char **argv)
>> case 'P':
>> partition = strtol(optarg, &end, 0);
>> if (*end) {
>> - errx(EXIT_FAILURE, "Invalid partition `%s'", optarg);
>> + error_report("Invalid partition `%s'", optarg);
>> + exit(EXIT_FAILURE);
>> }
>> if (partition < 1 || partition > 8) {
>> - errx(EXIT_FAILURE, "Invalid partition %d", partition);
>> + error_report("Invalid partition %d", partition);
>> + exit(EXIT_FAILURE);
>> }
>> break;
>> case 'k':
>> sockpath = optarg;
>> if (sockpath[0] != '/') {
>> - errx(EXIT_FAILURE, "socket path must be absolute\n");
>> + error_report("socket path must be absolute\n");
>
> I'm guessing later in the series will kill the trailing newline? If so,
> then this patch is fine (again, limiting to just coccinelle here).
It's a mistake-preserving patch :)
It should be killed in PATCH 21, but isn't, because I forgot to run
Coccinelle again after rebasing v1 onto the patches new in v2. I'll fix
PATCH 21.
>> + exit(EXIT_FAILURE);
>> }
>> break;
>> case 'd':
>> @@ -555,10 +569,12 @@ int main(int argc, char **argv)
>> case 'e':
>> shared = strtol(optarg, &end, 0);
>> if (*end) {
>> - errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg);
>> + error_report("Invalid shared device number '%s'", optarg);
>> + exit(EXIT_FAILURE);
>> }
>> if (shared < 1) {
>> - errx(EXIT_FAILURE, "Shared device number must be greater than 0\n");
>> + error_report("Shared device number must be greater than 0\n");
>> + exit(EXIT_FAILURE);
>
> And another one. Maybe mention in the commit message any future
> cleanups planned for style issues that are exposed by this conversion?
What about:
A few of the error messages touched have trailing newlines. They'll
be stripped later in this series.
>> }
>> break;
>> case 'f':
>> @@ -579,21 +595,23 @@ int main(int argc, char **argv)
>> exit(0);
>> break;
>> case '?':
>> - errx(EXIT_FAILURE, "Try `%s --help' for more information.",
>> - argv[0]);
>> + error_report("Try `%s --help' for more information.", argv[0]);
>> + exit(EXIT_FAILURE);
>> }
>> }
>>
>> if ((argc - optind) != 1) {
>> - errx(EXIT_FAILURE, "Invalid number of argument.\n"
>> - "Try `%s --help' for more information.",
>> - argv[0]);
>> + error_report("Invalid number of argument.\n" "Try `%s --help' for more information.",
>> + argv[0]);
>
> Long source line worth wrapping?
>
> Line wraps and commit message improvements seem obvious, so I'm okay
> with adding:
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()
2015-12-17 18:16 ` Eric Blake
@ 2015-12-17 19:00 ` Markus Armbruster
2015-12-17 19:53 ` Eric Blake
0 siblings, 1 reply; 66+ messages in thread
From: Markus Armbruster @ 2015-12-17 19:00 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 11:04 AM, Markus Armbruster wrote:
>
>>>> + * Create an error and add additional explanation:
>>>> + * error_setg(&err, "invalid quark");
>>>> + * error_append_hint(&err, "Valid quarks are up, down, strange, "
>>>> + * "charm, top, bottom\n");
>>>
>>> Do we want to allow/encourage a trailing dot in the hint? I'm fine if
>>> we don't - but once we document its usage, we should probably then be
>>> consistent with what we document; qemu-option.c used a trailing dot,
>>> qdev-monitor.c did not.
>>
>> I'll fix this example.
>>
>>>> + *
>>>> + * Do *not* contract this to
>>>> + * error_setg(&err, "invalid quark\n"
>>>> + * "Valid quarks are up, down, strange, charm, top, bottom");
>
> Of course, if you put a trailing dot above, you'd also want it here in
> the 'don't contract' section.
Yes.
>>>> @@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err);
>>>> * If @errp is anything else, *@errp must be NULL.
>>>> * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
>>>> * human-readable error message is made from printf-style @fmt, ...
>>>> + * The resulting message should not contain newlines.
>>>
>>> Should we also discourage trailing punctuation?
>>
>> Yes. How to best phrase it?
>
> Maybe:
>
> The resulting message should be a single phrase, with no newline or
> trailing punctuation.
What about ending the message with an exclamation mark?
>>> Should we also mention no need for leading 'error: ' prefix?
>>
>> Unlike the other anti-patterns, this one doesn't seem frequent in new
>> code.
>
> Fair enough.
>
>
>>>> /**
>>>> * Append a printf-style human-readable explanation to an existing error.
>>>> - * May be called multiple times, and safe if @errp is NULL.
>>>> + * @errp may be NULL, but neither &error_fatal nor &error_abort.
>>>
>>> As a native speaker, 'not' sounds better than 'neither' in that
>>> sentence. But I think both choices are grammatically correct.
>>
>> You mean "not &error_abort or &error_abort"?
>
> Yes, that works:
>
> @errp may be NULL, but not &error_fatal or &error_abort.
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()
2015-12-17 19:00 ` Markus Armbruster
@ 2015-12-17 19:53 ` Eric Blake
2015-12-18 9:11 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 19:53 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 912 bytes --]
On 12/17/2015 12:00 PM, Markus Armbruster wrote:
>>>>> @@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err);
>>>>> * If @errp is anything else, *@errp must be NULL.
>>>>> * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
>>>>> * human-readable error message is made from printf-style @fmt, ...
>>>>> + * The resulting message should not contain newlines.
>>>>
>>>> Should we also discourage trailing punctuation?
>>>
>>> Yes. How to best phrase it?
>>
>> Maybe:
>>
>> The resulting message should be a single phrase, with no newline or
>> trailing punctuation.
>
> What about ending the message with an exclamation mark?
Very few current users do that! An exclamation mark is still trailing
punctuation! And I don't like shouting at users!
:)
--
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report()
2015-12-17 18:52 ` Markus Armbruster
@ 2015-12-17 19:56 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 19:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]
On 12/17/2015 11:52 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>>> Coccinelle semantic patch
>>>
>>> case 'k':
>>> sockpath = optarg;
>>> if (sockpath[0] != '/') {
>>> - errx(EXIT_FAILURE, "socket path must be absolute\n");
>>> + error_report("socket path must be absolute\n");
>>
>> I'm guessing later in the series will kill the trailing newline? If so,
>> then this patch is fine (again, limiting to just coccinelle here).
>
> It's a mistake-preserving patch :)
As it should be. No-op automatic semantic transformations are easiest to
review when they are really no-op.
>
> It should be killed in PATCH 21, but isn't, because I forgot to run
> Coccinelle again after rebasing v1 onto the patches new in v2. I'll fix
> PATCH 21.
>
> What about:
>
> A few of the error messages touched have trailing newlines. They'll
> be stripped later in this series.
Works for me.
>> Long source line worth wrapping?
>>
>> Line wraps and commit message improvements seem obvious, so I'm okay
>> with adding:
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Thanks!
--
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints Markus Armbruster
@ 2015-12-17 20:10 ` Eric Blake
2015-12-18 8:49 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:10 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1474 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> contrib/ivshmem-server/main.c | 4 +---
> qdev-monitor.c | 3 +--
> qemu-nbd.c | 3 +--
> 3 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
> index 54ff001..00508b5 100644
> --- a/contrib/ivshmem-server/main.c
> +++ b/contrib/ivshmem-server/main.c
> @@ -106,9 +106,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
> case 'l': /* shm_size */
> parse_option_size("shm_size", optarg, &args->shm_size, &errp);
Idea for a followup patch: The name 'errp' is most often associated with
type 'Error **'; but here it is 'Error *', which is confusing.
Other offenders in hmp.c, hw/core/nmi.c, include/qemu/sockets.h, and
tests/test-string-output-visitor.c.
> if (errp) {
> - fprintf(stderr, "cannot parse shm size: %s\n",
> - error_get_pretty(errp));
> - error_free(errp);
> + error_report_err(errp);
This loses the "cannot parse shm size: " prefix; but I don't think that
hurts. Could use a mention in the commit message, though.
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 06/23] block: Clean up "Could not create temporary overlay" error message
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 06/23] block: Clean up "Could not create temporary overlay" error message Markus Armbruster
@ 2015-12-17 20:16 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:16 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 702 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> bdrv_create() sets an error and returns -errno on failure. When the
> latter is interesting, the error is created with error_setg_errno().
>
> bdrv_append_temp_snapshot() uses the error's message to create a new
> one with error_setg_errno(). This adds a strerror() that is either
> uninteresting or duplicate. Use error_setg() instead.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/23] qemu-nbd: Clean up "Failed to load snapshot" error message
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 07/23] qemu-nbd: Clean up "Failed to load snapshot" " Markus Armbruster
@ 2015-12-17 20:17 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:17 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 631 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> bdrv_snapshot_load_tmp() sets an error and returns -errno on failure.
> We report both even though the error message is self-contained. Drop
> the redundant strerror().
>
> While there: setting errno right before exit() is pointless, so drop
> that, too.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> qemu-nbd.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 08/23] test-throttle: Simplify qemu_init_main_loop() error handling
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 08/23] test-throttle: Simplify qemu_init_main_loop() error handling Markus Armbruster
@ 2015-12-17 20:19 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:19 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> The code looks like it tries to check for both qemu_init_main_loop()
> and qemu_get_aio_context() failure in one conditional. In fact,
> qemu_get_aio_context() can fail only after qemu_init_main_loop()
> failed.
>
> Simplify accordingly: check for qemu_init_main_loop() error directly,
> without bothering to improve its error message. Call
> qemu_get_aio_context() only when qemu_get_aio_context() succeeded. It
> can't fail then, so no need to check.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> tests/test-throttle.c | 15 +--------------
> 1 file changed, 1 insertion(+), 14 deletions(-)
Much simpler :)
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 09/23] error: New error_prepend(), error_reportf_err()
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 09/23] error: New error_prepend(), error_reportf_err() Markus Armbruster
@ 2015-12-17 20:23 ` Eric Blake
2015-12-18 9:13 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:23 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Instead of simply propagating an error verbatim, we sometimes want to
> add to its message, like this:
>
> frobnicate(arg, &err);
> error_setg(errp, "Can't frobnicate %s: %s",
> arg, error_get_pretty(err));
Did you intend to have literal TABs in the commit message?
> error_free(err);
>
> This is suboptimal, because it loses err's hint (if any). Moreover,
> when errp is &error_abort or is subsequently propagated to
> &error_abort, the abort message points to the place where we last
> added to the error, not to the place where it originated.
>
> To avoid these issues, provide means to add to an error's message in
> place:
>
> frobnicate(arg, errp);
> error_prepend(errp, "Can't frobnicate %s: ", arg);
>
> Likewise, reporting an error like
>
> frobnicate(arg, &err);
> error_report("Can't frobnicate %s: %s", arg, error_get_pretty(err));
>
> can lose err's hint. To avoid:
>
> error_reportf_err(err, "Can't frobnicate %s: ", arg);
>
> The next commits will put these functions to use.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/error.h | 31 +++++++++++++++++++++++++++++--
> util/error.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 62 insertions(+), 2 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 10/23] error: Don't decorate original error message when adding to it
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 10/23] error: Don't decorate original error message when adding to it Markus Armbruster
@ 2015-12-17 20:32 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:32 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 718 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Prepend the additional information, colon, space to the original
> message without enclosing it in parenthesis or quotes, like we do
> elsewhere.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/core/qdev-properties.c | 2 +-
> qemu-img.c | 2 +-
> tests/test-aio.c | 2 +-
> tests/test-thread-pool.c | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
And I'm guessing this makes for an easier coccinelle conversion patch
down the road :)
--
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 11/23] error: Use error_reportf_err() where it makes obvious sense
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 11/23] error: Use error_reportf_err() where it makes obvious sense Markus Armbruster
@ 2015-12-17 20:39 ` Eric Blake
2015-12-18 9:18 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:39 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Done with this Coccinelle semantic patch
>
> @@
> expression FMT, E;
> expression list ARGS;
> @@
> - error_report(FMT, ARGS, error_get_pretty(E));
> + error_reportf_err(E, FMT/*@@@*/, ARGS);
> (
> - error_free(E);
> |
> exit(S);
Does S have to be declared an expression, for this branch to work
correctly? I guess not, since...
> |
> abort();
> )
>
> followed by a replace of '%s"/*@@@*/' by '"' and some line rewrapping,
> because I can't figure out how to make Coccinelle transform strings.
>
Hey - you can already make it do more than I can.
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> +++ b/arch_init.c
> @@ -258,9 +258,7 @@ void do_acpitable_option(const QemuOpts *opts)
>
> acpi_table_add(opts, &err);
> if (err) {
> - error_report("Wrong acpi table provided: %s",
> - error_get_pretty(err));
> - error_free(err);
> + error_reportf_err(err, "Wrong acpi table provided: ");
> exit(1);
...you properly found this spot.
> +++ b/blockdev.c
> @@ -1583,13 +1583,10 @@ static void internal_snapshot_abort(BlkActionState *common)
> }
>
> if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
> - error_report("Failed to delete snapshot with id '%s' and name '%s' on "
> - "device '%s' in abort: %s",
> - sn->id_str,
> - sn->name,
> - bdrv_get_device_name(bs),
> - error_get_pretty(local_error));
> - error_free(local_error);
> + error_reportf_err(local_error,
> + "Failed to delete snapshot with id '%s' and name '%s' on " "device '%s' in abort: ",
Whoops; line rewrapping touchups missed here.
With that fixed,
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 12/23] error: Use error_prepend() where it makes obvious sense
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 12/23] error: Use error_prepend() " Markus Armbruster
@ 2015-12-17 20:43 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:43 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2908 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Done with this Coccinelle semantic patch
>
> @@
> expression FMT, E1, E2;
> expression list ARGS;
> @@
> - error_setg(E1, FMT, ARGS, error_get_pretty(E2));
> + error_propagate(E1, E2);/*###*/
> + error_prepend(E1, FMT/*@@@*/, ARGS);
>
> followed by manual cleanup, first because I can't figure out how to
> make Coccinelle transform strings, and second to get rid of now
> superfluous error_propagate().
Trying to understand this...
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> +++ b/block/qed.c
> @@ -1611,9 +1611,8 @@ static void bdrv_qed_invalidate_cache(BlockDriverState *bs, Error **errp)
> memset(s, 0, sizeof(BDRVQEDState));
> ret = bdrv_qed_open(bs, NULL, bs->open_flags, &local_err);
> if (local_err) {
> - error_setg(errp, "Could not reopen qed layer: %s",
> - error_get_pretty(local_err));
> - error_free(local_err);
> + error_propagate(errp, local_err);
> + error_prepend(errp, "Could not reopen qed layer: ");
...error_propagate() was not always superfluous...
> return;
> } else if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not reopen qed layer");
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index c57f293..df77ba8 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -142,7 +142,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
> Error **errp)
> {
> VirtIOBlockDataPlane *s;
> - Error *local_err = NULL;
> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>
> @@ -163,11 +162,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
> /* If dataplane is (re-)enabled while the guest is running there could be
> * block jobs that can conflict.
> */
> - if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE,
> - &local_err)) {
> - error_setg(errp, "cannot start dataplane thread: %s",
> - error_get_pretty(local_err));
> - error_free(local_err);
> + if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
> + error_prepend(errp, "cannot start dataplane thread: ");
...but when the only use of a local error was just to rewrite its
message, then the local error variable and error_propagate() call can
just go away instead. Okay, makes sense now, and no, I don't know how
to make Coccinelle do it either.
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 13/23] spapr: Use error_reportf_err()
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 13/23] spapr: Use error_reportf_err() Markus Armbruster
@ 2015-12-17 20:44 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:44 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 466 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Not caught by Coccinelle for the previous patch, because it reports
> the error only conditionally.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/ppc/spapr.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/23] migration: Use error_reportf_err() instead of monitor_printf()
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 14/23] migration: Use error_reportf_err() instead of monitor_printf() Markus Armbruster
@ 2015-12-17 20:46 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:46 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 722 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Using error_reportf_err() instead of monitor_printf() makes no
> difference when monitor_printf() is used correctly, i.e. within an HMP
> monitor. Elsewhere, monitor_printf() does nothing, while
> error_reportf_err() reports to stderr.
>
> Both changed functions are HMP command handlers. These should only
> run within an HMP monitor.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> migration/savevm.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 15/23] qemu-io qemu-nbd: Use error_report() etc. instead of fprintf()
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 15/23] qemu-io qemu-nbd: Use error_report() etc. instead of fprintf() Markus Armbruster
@ 2015-12-17 20:51 ` Eric Blake
2015-12-17 20:57 ` Eric Blake
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:51 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 919 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Just three instances left.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> +++ b/qemu-nbd.c
> @@ -257,7 +257,7 @@ static void *nbd_client_thread(void *arg)
> fd = open(device, O_RDWR);
> if (fd < 0) {
> /* Linux-only, we can use %m in printf. */
> - fprintf(stderr, "Failed to open %s: %m\n", device);
> + error_report("Failed to open %s: %m", device);
I asked back in 1/20 if we want to add error_report_errno() - if so, it
would let us switch from the comment about non-portable use of %m over
to using that new function. Of course, doesn't affect _this_ patch.
Lots of test fallout from those three conversions, but they look fine.
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 16/23] error: Strip trailing '\n' from error string arguments (again)
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 16/23] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
@ 2015-12-17 20:57 ` Eric Blake
2015-12-18 9:47 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:57 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Fam Zheng, zhanghailiang, Stefan Berger, Markus Armbruster,
Pavel Fedin, Dr. David Alan Gilbert, Dominik Dingel,
David Hildenbrand, Peter Crosthwaite, Jason J. Herne,
Bharata B Rao, Changchun Ouyang
[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]
On 12/17/2015 09:49 AM, 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.
Don't forget to rerun this to pick up stragglers exposed by 1/23 :)
> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Acked-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Acked-by: Fam Zheng <famz@redhat.com>
If you want to add to the list:
Reviewed-by: Eric Blake <eblake@redhat.com>
> +++ b/hw/s390x/s390-skeys.c
> @@ -191,8 +191,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);
Do we want a separate patch cleaning up 'Error: ' prefixes?
--
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 15/23] qemu-io qemu-nbd: Use error_report() etc. instead of fprintf()
2015-12-17 20:51 ` Eric Blake
@ 2015-12-17 20:57 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 20:57 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]
On 12/17/2015 01:51 PM, Eric Blake wrote:
> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> Just three instances left.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/qemu-nbd.c
>> @@ -257,7 +257,7 @@ static void *nbd_client_thread(void *arg)
>> fd = open(device, O_RDWR);
>> if (fd < 0) {
>> /* Linux-only, we can use %m in printf. */
>> - fprintf(stderr, "Failed to open %s: %m\n", device);
>> + error_report("Failed to open %s: %m", device);
>
> I asked back in 1/20 if we want to add error_report_errno() - if so, it
I meant 1/23.
> would let us switch from the comment about non-portable use of %m over
> to using that new function. Of course, doesn't affect _this_ patch.
>
> Lots of test fallout from those three conversions, but they look fine.
>
> 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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit Markus Armbruster
@ 2015-12-17 21:00 ` Eric Blake
2015-12-18 9:54 ` Markus Armbruster
2015-12-18 0:48 ` Fam Zheng
1 sibling, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 21:00 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Fam Zheng
[-- Attachment #1: Type: text/plain, Size: 537 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> Cc: Fam Zheng <famz@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
Could have mentioned what the change was: factoring out a common
next_line() helper to let you drop an end-of-loop label.
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message Markus Armbruster
@ 2015-12-17 21:09 ` Eric Blake
2015-12-18 0:52 ` Fam Zheng
1 sibling, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 21:09 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Fam Zheng
[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> vmdk_parse_extents() reports parse errors like this:
>
> error_setg(errp, "Invalid extent lines:\n%s", p);
>
> where p points to the beginning of the malformed line in the image
> descriptor. This results in a multi-line error message
>
> Invalid extent lines:
> <first line that doesn't parse>
> <remaining text that may or may not parse, if any>
>
> Error messages should not have newlines embedded. Since the remaining
> text is not helpful, we can simply report:
>
> Invalid extent line: <first line that doesn't parse>
>
> Cc: Fam Zheng <famz@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 19 ++++++++++++-------
> tests/qemu-iotests/059.out | 4 +---
> 2 files changed, 13 insertions(+), 10 deletions(-)
>
> @@ -883,6 +880,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> extent->type = g_strdup(type);
> }
> return 0;
> +
> +invalid:
> + np = next_line(p);
> + assert(np != p);
> + if (np[-1] == '\n')
> + np--;
> + error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
[Side note: It would be nice if C/POSIX had a way to specify that %.*s
will take a ptrdiff_t argument, instead of forcing callers to cast to
int. Oh well.]
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 19/23] pci-assign: Clean up "Failed to assign" error messages
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 19/23] pci-assign: Clean up "Failed to assign" error messages Markus Armbruster
@ 2015-12-17 21:45 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 21:45 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Laszlo Ersek
[-- Attachment #1: Type: text/plain, Size: 709 bytes --]
On 12/17/2015 09:50 AM, Markus Armbruster wrote:
> The arguments of error_setg() & friends should yield a short error
> string without newlines.
>
> Two 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().
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/i386/kvm/pci-assign.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 20/23] vhdx: Fix "log that needs to be replayed" error message
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 20/23] vhdx: Fix "log that needs to be replayed" error message Markus Armbruster
@ 2015-12-17 21:52 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 21:52 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]
On 12/17/2015 09:50 AM, Markus Armbruster wrote:
> The arguments of error_setg_errno() should yield a short error string
> without newlines.
>
> Here, we try to append additional help to the error message by
> embedding newlines in the error string. That's nice, but it's doesn't
> play nicely with the errno part. tests/qemu-iotests/070.out shows the
> resulting mess:
>
> can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log that needs to be replayed. To replay the log, execute:
> qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx': Operation not permitted
>
> Switch to error_setg() and error_append_hint(). Result:
>
> can't open device TEST_DIR/iotest-dirtylog-10G-4M.vhdx: VHDX image file 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx' opened read-only, but contains a log that needs to be replayed
> To replay the log, run:
> qemu-img check -r all 'TEST_DIR/iotest-dirtylog-10G-4M.vhdx'
The loss of the strerror() text isn't too bad; particularly since it is
probably always going to be 'Operation not permitted' and redundant with
the text about being read-only with a replay that requires write access.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vhdx-log.c | 13 +++++++------
> tests/qemu-iotests/070.out | 5 +++--
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 21/23] error: Clean up errors with embedded newlines (again)
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 21/23] error: Clean up errors with embedded newlines (again) Markus Armbruster
@ 2015-12-17 21:53 ` Eric Blake
2015-12-18 10:04 ` Markus Armbruster
0 siblings, 1 reply; 66+ messages in thread
From: Eric Blake @ 2015-12-17 21:53 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Laszlo Ersek, Markus Armbruster, Pavel Fedin
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
On 12/17/2015 09:50 AM, 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 coming
> back. Offenders tracked down with the Coccinelle semantic patch from
> commit 312fd5f.
Not sure if you'll have any stragglers from 1/23 here, after fixing 16/23.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> hw/i386/pc.c | 4 ++--
> kvm-all.c | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 22/23] hw/s390x: Rename local variables Error *l_err to just err
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 22/23] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
@ 2015-12-17 21:54 ` Eric Blake
0 siblings, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 21:54 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: David Hildenbrand, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 633 bytes --]
On 12/17/2015 09:50 AM, Markus Armbruster wrote:
> Let's follow established naming practice here as well.
>
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
> hw/s390x/ipl.c | 12 ++++++------
> hw/s390x/sclp.c | 14 +++++++-------
> 2 files changed, 13 insertions(+), 13 deletions(-)
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize()
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize() Markus Armbruster
2015-12-17 17:25 ` Cornelia Huck
@ 2015-12-17 21:54 ` Eric Blake
1 sibling, 0 replies; 66+ messages in thread
From: Eric Blake @ 2015-12-17 21:54 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 482 bytes --]
On 12/17/2015 09:50 AM, Markus Armbruster wrote:
> Suggested-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
> hw/s390x/sclp.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
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 --]
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit Markus Armbruster
2015-12-17 21:00 ` Eric Blake
@ 2015-12-18 0:48 ` Fam Zheng
1 sibling, 0 replies; 66+ messages in thread
From: Fam Zheng @ 2015-12-18 0:48 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On Thu, 12/17 17:49, Markus Armbruster wrote:
> Cc: Fam Zheng <famz@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index b4a224e..08fa3f3 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -760,6 +760,17 @@ static int vmdk_open_sparse(BlockDriverState *bs, BdrvChild *file, int flags,
> }
> }
>
> +static const char *next_line(const char *s)
> +{
> + while (*s) {
> + if (*s == '\n') {
> + return s + 1;
> + }
> + s++;
> + }
> + return s;
> +}
> +
> static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> const char *desc_file_path, QDict *options,
> Error **errp)
> @@ -769,7 +780,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> char access[11];
> char type[11];
> char fname[512];
> - const char *p = desc;
> + const char *p;
> int64_t sectors = 0;
> int64_t flat_offset;
> char *extent_path;
> @@ -779,7 +790,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> char extent_opt_prefix[32];
> Error *local_err = NULL;
>
> - while (*p) {
> + for (p = desc; *p; p = next_line(p)) {
> /* parse extent line in one of below formats:
> *
> * RW [size in sectors] FLAT "file-name.vmdk" OFFSET
> @@ -791,7 +802,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> matches = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
> access, §ors, type, fname, &flat_offset);
> if (matches < 4 || strcmp(access, "RW")) {
> - goto next_line;
> + continue;
> } else if (!strcmp(type, "FLAT")) {
> if (matches != 5 || flat_offset < 0) {
> error_setg(errp, "Invalid extent lines: \n%s", p);
> @@ -813,7 +824,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> (strcmp(type, "FLAT") && strcmp(type, "SPARSE") &&
> strcmp(type, "VMFS") && strcmp(type, "VMFSSPARSE")) ||
> (strcmp(access, "RW"))) {
> - goto next_line;
> + continue;
> }
>
> if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
> @@ -870,15 +881,6 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> return -ENOTSUP;
> }
> extent->type = g_strdup(type);
> -next_line:
> - /* move to next line */
> - while (*p) {
> - if (*p == '\n') {
> - p++;
> - break;
> - }
> - p++;
> - }
> }
> return 0;
> }
> --
> 2.4.3
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message Markus Armbruster
2015-12-17 21:09 ` Eric Blake
@ 2015-12-18 0:52 ` Fam Zheng
2015-12-18 9:56 ` Markus Armbruster
1 sibling, 1 reply; 66+ messages in thread
From: Fam Zheng @ 2015-12-18 0:52 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On Thu, 12/17 17:49, Markus Armbruster wrote:
> vmdk_parse_extents() reports parse errors like this:
>
> error_setg(errp, "Invalid extent lines:\n%s", p);
>
> where p points to the beginning of the malformed line in the image
> descriptor. This results in a multi-line error message
>
> Invalid extent lines:
> <first line that doesn't parse>
> <remaining text that may or may not parse, if any>
>
> Error messages should not have newlines embedded. Since the remaining
> text is not helpful, we can simply report:
>
> Invalid extent line: <first line that doesn't parse>
>
> Cc: Fam Zheng <famz@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 19 ++++++++++++-------
> tests/qemu-iotests/059.out | 4 +---
> 2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 08fa3f3..04b47e3 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -780,7 +780,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> char access[11];
> char type[11];
> char fname[512];
> - const char *p;
> + const char *p, *np;
> int64_t sectors = 0;
> int64_t flat_offset;
> char *extent_path;
> @@ -805,19 +805,16 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> continue;
> } else if (!strcmp(type, "FLAT")) {
> if (matches != 5 || flat_offset < 0) {
> - error_setg(errp, "Invalid extent lines: \n%s", p);
> - return -EINVAL;
> + goto invalid;
> }
> } else if (!strcmp(type, "VMFS")) {
> if (matches == 4) {
> flat_offset = 0;
> } else {
> - error_setg(errp, "Invalid extent lines:\n%s", p);
> - return -EINVAL;
> + goto invalid;
> }
> } else if (matches != 4) {
> - error_setg(errp, "Invalid extent lines:\n%s", p);
> - return -EINVAL;
> + goto invalid;
> }
>
> if (sectors <= 0 ||
> @@ -883,6 +880,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> extent->type = g_strdup(type);
> }
> return 0;
> +
> +invalid:
> + np = next_line(p);
> + assert(np != p);
> + if (np[-1] == '\n')
> + np--;
Braces are required.
> + error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
> + return -EINVAL;
> }
>
> static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index d28df5b..9d506cb 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -2038,9 +2038,7 @@ Format specific information:
> format: FLAT
>
> === Testing malformed VMFS extent description line ===
> -qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent lines:
> -RW 12582912 VMFS "dummy.IMGFMT" 1
> -
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 12582912 VMFS "dummy.IMGFMT" 1
>
> === Testing truncated sparse ===
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400 subformat=monolithicSparse
> --
> 2.4.3
>
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints
2015-12-17 20:10 ` Eric Blake
@ 2015-12-18 8:49 ` Markus Armbruster
0 siblings, 0 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-18 8:49 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> contrib/ivshmem-server/main.c | 4 +---
>> qdev-monitor.c | 3 +--
>> qemu-nbd.c | 3 +--
>> 3 files changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
>> index 54ff001..00508b5 100644
>> --- a/contrib/ivshmem-server/main.c
>> +++ b/contrib/ivshmem-server/main.c
>> @@ -106,9 +106,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
>> case 'l': /* shm_size */
>> parse_option_size("shm_size", optarg, &args->shm_size, &errp);
>
> Idea for a followup patch: The name 'errp' is most often associated with
> type 'Error **'; but here it is 'Error *', which is confusing.
Yes. Another round of these:
e940f54 qmp hmp: Consistently name Error * objects err, and not errp
2f719f1 hw: Consistently name Error * objects err, and not errp
4399c43 qemu-img: Consistently name Error * objects err, and not errp
> Other offenders in hmp.c, hw/core/nmi.c, include/qemu/sockets.h, and
> tests/test-string-output-visitor.c.
>
>> if (errp) {
>> - fprintf(stderr, "cannot parse shm size: %s\n",
>> - error_get_pretty(errp));
>> - error_free(errp);
>> + error_report_err(errp);
>
> This loses the "cannot parse shm size: " prefix; but I don't think that
> hurts. Could use a mention in the commit message, though.
Losing the prefix is fine, because the error messages always mention the
parameter name. For instance,
cannot parse shm size: Parameter 'shm_size' expects a size
becomes
Parameter 'shm_size' expects a size
You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and terabytes.
Note we now get the hint.
Including the error location would be even nicer, but is out of scope
for this series.
The program shows its complete usage help afterwards, which is annoying,
but out of scope for this series.
I'll amend the commit message.
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()
2015-12-17 19:53 ` Eric Blake
@ 2015-12-18 9:11 ` Markus Armbruster
0 siblings, 0 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-18 9:11 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 12:00 PM, Markus Armbruster wrote:
>
>>>>>> @@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err);
>>>>>> * If @errp is anything else, *@errp must be NULL.
>>>>>> * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
>>>>>> * human-readable error message is made from printf-style @fmt, ...
>>>>>> + * The resulting message should not contain newlines.
>>>>>
>>>>> Should we also discourage trailing punctuation?
>>>>
>>>> Yes. How to best phrase it?
>>>
>>> Maybe:
>>>
>>> The resulting message should be a single phrase, with no newline or
>>> trailing punctuation.
>>
>> What about ending the message with an exclamation mark?
>
> Very few current users do that! An exclamation mark is still trailing
> punctuation! And I don't like shouting at users!
>
> :)
Okay, okay, I applied it %-)
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 09/23] error: New error_prepend(), error_reportf_err()
2015-12-17 20:23 ` Eric Blake
@ 2015-12-18 9:13 ` Markus Armbruster
0 siblings, 0 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-18 9:13 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> Instead of simply propagating an error verbatim, we sometimes want to
>> add to its message, like this:
>>
>> frobnicate(arg, &err);
>> error_setg(errp, "Can't frobnicate %s: %s",
>> arg, error_get_pretty(err));
>
> Did you intend to have literal TABs in the commit message?
Editing accident, fixed.
>> error_free(err);
>>
>> This is suboptimal, because it loses err's hint (if any). Moreover,
>> when errp is &error_abort or is subsequently propagated to
>> &error_abort, the abort message points to the place where we last
>> added to the error, not to the place where it originated.
>>
>> To avoid these issues, provide means to add to an error's message in
>> place:
>>
>> frobnicate(arg, errp);
>> error_prepend(errp, "Can't frobnicate %s: ", arg);
>>
>> Likewise, reporting an error like
>>
>> frobnicate(arg, &err);
>> error_report("Can't frobnicate %s: %s", arg, error_get_pretty(err));
>>
>> can lose err's hint. To avoid:
>>
>> error_reportf_err(err, "Can't frobnicate %s: ", arg);
>>
>> The next commits will put these functions to use.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/qapi/error.h | 31 +++++++++++++++++++++++++++++--
>> util/error.c | 33 +++++++++++++++++++++++++++++++++
>> 2 files changed, 62 insertions(+), 2 deletions(-)
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 11/23] error: Use error_reportf_err() where it makes obvious sense
2015-12-17 20:39 ` Eric Blake
@ 2015-12-18 9:18 ` Markus Armbruster
0 siblings, 0 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-18 9:18 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> Done with this Coccinelle semantic patch
>>
>> @@
>> expression FMT, E;
>> expression list ARGS;
>> @@
>> - error_report(FMT, ARGS, error_get_pretty(E));
>> + error_reportf_err(E, FMT/*@@@*/, ARGS);
>> (
>> - error_free(E);
>> |
>> exit(S);
>
> Does S have to be declared an expression, for this branch to work
> correctly? I guess not, since...
Pasto, fixed.
>> |
>> abort();
>> )
>>
>> followed by a replace of '%s"/*@@@*/' by '"' and some line rewrapping,
>> because I can't figure out how to make Coccinelle transform strings.
>>
>
> Hey - you can already make it do more than I can.
>
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> +++ b/arch_init.c
>> @@ -258,9 +258,7 @@ void do_acpitable_option(const QemuOpts *opts)
>>
>> acpi_table_add(opts, &err);
>> if (err) {
>> - error_report("Wrong acpi table provided: %s",
>> - error_get_pretty(err));
>> - error_free(err);
>> + error_reportf_err(err, "Wrong acpi table provided: ");
>> exit(1);
>
> ...you properly found this spot.
>
>> +++ b/blockdev.c
>> @@ -1583,13 +1583,10 @@ static void
>> internal_snapshot_abort(BlkActionState *common)
>> }
>>
>> if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
>> - error_report("Failed to delete snapshot with id '%s' and name '%s'
>> on "
>> - "device '%s' in abort: %s",
>> - sn->id_str,
>> - sn->name,
>> - bdrv_get_device_name(bs),
>> - error_get_pretty(local_error));
>> - error_free(local_error);
>> + error_reportf_err(local_error,
>> + "Failed to delete snapshot with id '%s' and name '%s' on " "device
>> '%s' in abort: ",
>
> Whoops; line rewrapping touchups missed here.
>
> With that fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 16/23] error: Strip trailing '\n' from error string arguments (again)
2015-12-17 20:57 ` Eric Blake
@ 2015-12-18 9:47 ` Markus Armbruster
0 siblings, 0 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-18 9:47 UTC (permalink / raw)
To: Eric Blake
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
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 09:49 AM, 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.
>
> Don't forget to rerun this to pick up stragglers exposed by 1/23 :)
Just two:
qemu-nbd.c:574:76:"Shared device number must be greater than 0\n"
qemu-nbd.c:557:61:"socket path must be absolute\n"
>> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Acked-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>> Acked-by: Fam Zheng <famz@redhat.com>
>
> If you want to add to the list:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +++ b/hw/s390x/s390-skeys.c
>> @@ -191,8 +191,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);
>
> Do we want a separate patch cleaning up 'Error: ' prefixes?
After my Christmas break, in a separate series probably.
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit
2015-12-17 21:00 ` Eric Blake
@ 2015-12-18 9:54 ` Markus Armbruster
0 siblings, 0 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-18 9:54 UTC (permalink / raw)
To: Eric Blake; +Cc: Fam Zheng, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> Cc: Fam Zheng <famz@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> block/vmdk.c | 28 +++++++++++++++-------------
>> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> Could have mentioned what the change was: factoring out a common
> next_line() helper to let you drop an end-of-loop label.
Adding:
Factor out loop stepping to turn a while-loop with goto into a
for-loop with continue.
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message
2015-12-18 0:52 ` Fam Zheng
@ 2015-12-18 9:56 ` Markus Armbruster
0 siblings, 0 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-18 9:56 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel
Fam Zheng <famz@redhat.com> writes:
> On Thu, 12/17 17:49, Markus Armbruster wrote:
>> vmdk_parse_extents() reports parse errors like this:
>>
>> error_setg(errp, "Invalid extent lines:\n%s", p);
>>
>> where p points to the beginning of the malformed line in the image
>> descriptor. This results in a multi-line error message
>>
>> Invalid extent lines:
>> <first line that doesn't parse>
>> <remaining text that may or may not parse, if any>
>>
>> Error messages should not have newlines embedded. Since the remaining
>> text is not helpful, we can simply report:
>>
>> Invalid extent line: <first line that doesn't parse>
>>
>> Cc: Fam Zheng <famz@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> block/vmdk.c | 19 ++++++++++++-------
>> tests/qemu-iotests/059.out | 4 +---
>> 2 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 08fa3f3..04b47e3 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -780,7 +780,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>> char access[11];
>> char type[11];
>> char fname[512];
>> - const char *p;
>> + const char *p, *np;
>> int64_t sectors = 0;
>> int64_t flat_offset;
>> char *extent_path;
>> @@ -805,19 +805,16 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>> continue;
>> } else if (!strcmp(type, "FLAT")) {
>> if (matches != 5 || flat_offset < 0) {
>> - error_setg(errp, "Invalid extent lines: \n%s", p);
>> - return -EINVAL;
>> + goto invalid;
>> }
>> } else if (!strcmp(type, "VMFS")) {
>> if (matches == 4) {
>> flat_offset = 0;
>> } else {
>> - error_setg(errp, "Invalid extent lines:\n%s", p);
>> - return -EINVAL;
>> + goto invalid;
>> }
>> } else if (matches != 4) {
>> - error_setg(errp, "Invalid extent lines:\n%s", p);
>> - return -EINVAL;
>> + goto invalid;
>> }
>>
>> if (sectors <= 0 ||
>> @@ -883,6 +880,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>> extent->type = g_strdup(type);
>> }
>> return 0;
>> +
>> +invalid:
>> + np = next_line(p);
>> + assert(np != p);
>> + if (np[-1] == '\n')
>> + np--;
>
> Braces are required.
Fixing...
>> + error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
>> + return -EINVAL;
>> }
>>
>> static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
>> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
>> index d28df5b..9d506cb 100644
>> --- a/tests/qemu-iotests/059.out
>> +++ b/tests/qemu-iotests/059.out
>> @@ -2038,9 +2038,7 @@ Format specific information:
>> format: FLAT
>>
>> === Testing malformed VMFS extent description line ===
>> -qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent lines:
>> -RW 12582912 VMFS "dummy.IMGFMT" 1
>> -
>> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 12582912 VMFS "dummy.IMGFMT" 1
>>
>> === Testing truncated sparse ===
>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400 subformat=monolithicSparse
>> --
>> 2.4.3
>>
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [Qemu-devel] [PATCH v2 21/23] error: Clean up errors with embedded newlines (again)
2015-12-17 21:53 ` Eric Blake
@ 2015-12-18 10:04 ` Markus Armbruster
0 siblings, 0 replies; 66+ messages in thread
From: Markus Armbruster @ 2015-12-18 10:04 UTC (permalink / raw)
To: Eric Blake; +Cc: Pavel Fedin, Laszlo Ersek, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/17/2015 09:50 AM, 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 coming
>> back. Offenders tracked down with the Coccinelle semantic patch from
>> commit 312fd5f.
>
> Not sure if you'll have any stragglers from 1/23 here, after fixing 16/23.
Just one:
qemu-nbd.c:604:28:"Invalid number of argument.\n" "Try `%s --help' for more information."
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Pavel Fedin <p.fedin@samsung.com>
>> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> hw/i386/pc.c | 4 ++--
>> kvm-all.c | 6 +++---
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 66+ messages in thread
end of thread, other threads:[~2015-12-18 10:05 UTC | newest]
Thread overview: 66+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-17 16:49 [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 01/23] qemu-nbd: Replace BSDism <err.h> by error_report() Markus Armbruster
2015-12-17 17:17 ` Eric Blake
2015-12-17 18:52 ` Markus Armbruster
2015-12-17 19:56 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 02/23] error: Use error_report_err() where appropriate (again) Markus Armbruster
2015-12-17 17:33 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 03/23] error: Use error_report_err() instead of monitor_printf() Markus Armbruster
2015-12-17 18:06 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 04/23] error: Use error_report_err() instead of ad hoc prints Markus Armbruster
2015-12-17 20:10 ` Eric Blake
2015-12-18 8:49 ` Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint() Markus Armbruster
2015-12-17 16:59 ` Eric Blake
2015-12-17 18:04 ` Markus Armbruster
2015-12-17 18:16 ` Eric Blake
2015-12-17 19:00 ` Markus Armbruster
2015-12-17 19:53 ` Eric Blake
2015-12-18 9:11 ` Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 06/23] block: Clean up "Could not create temporary overlay" error message Markus Armbruster
2015-12-17 20:16 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 07/23] qemu-nbd: Clean up "Failed to load snapshot" " Markus Armbruster
2015-12-17 20:17 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 08/23] test-throttle: Simplify qemu_init_main_loop() error handling Markus Armbruster
2015-12-17 20:19 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 09/23] error: New error_prepend(), error_reportf_err() Markus Armbruster
2015-12-17 20:23 ` Eric Blake
2015-12-18 9:13 ` Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 10/23] error: Don't decorate original error message when adding to it Markus Armbruster
2015-12-17 20:32 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 11/23] error: Use error_reportf_err() where it makes obvious sense Markus Armbruster
2015-12-17 20:39 ` Eric Blake
2015-12-18 9:18 ` Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 12/23] error: Use error_prepend() " Markus Armbruster
2015-12-17 20:43 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 13/23] spapr: Use error_reportf_err() Markus Armbruster
2015-12-17 20:44 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 14/23] migration: Use error_reportf_err() instead of monitor_printf() Markus Armbruster
2015-12-17 20:46 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 15/23] qemu-io qemu-nbd: Use error_report() etc. instead of fprintf() Markus Armbruster
2015-12-17 20:51 ` Eric Blake
2015-12-17 20:57 ` Eric Blake
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 16/23] error: Strip trailing '\n' from error string arguments (again) Markus Armbruster
2015-12-17 20:57 ` Eric Blake
2015-12-18 9:47 ` Markus Armbruster
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 17/23] vmdk: Clean up control flow in vmdk_parse_extents() a bit Markus Armbruster
2015-12-17 21:00 ` Eric Blake
2015-12-18 9:54 ` Markus Armbruster
2015-12-18 0:48 ` Fam Zheng
2015-12-17 16:49 ` [Qemu-devel] [PATCH v2 18/23] vmdk: Clean up "Invalid extent lines" error message Markus Armbruster
2015-12-17 21:09 ` Eric Blake
2015-12-18 0:52 ` Fam Zheng
2015-12-18 9:56 ` Markus Armbruster
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 19/23] pci-assign: Clean up "Failed to assign" error messages Markus Armbruster
2015-12-17 21:45 ` Eric Blake
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 20/23] vhdx: Fix "log that needs to be replayed" error message Markus Armbruster
2015-12-17 21:52 ` Eric Blake
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 21/23] error: Clean up errors with embedded newlines (again) Markus Armbruster
2015-12-17 21:53 ` Eric Blake
2015-12-18 10:04 ` Markus Armbruster
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 22/23] hw/s390x: Rename local variables Error *l_err to just err Markus Armbruster
2015-12-17 21:54 ` Eric Blake
2015-12-17 16:50 ` [Qemu-devel] [PATCH v2 23/23] s390/sclp: Simplify control flow in sclp_realize() Markus Armbruster
2015-12-17 17:25 ` Cornelia Huck
2015-12-17 21:54 ` Eric Blake
2015-12-17 17:26 ` [Qemu-devel] [PATCH v2 00/23] Error reporting cleanups and fixes Cornelia Huck
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.