* [Qemu-devel] [PATCH v3 01/15] qemu-img: Use error_vreport() in error_exit()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz, qemu-block
error_exit() uses low-level error_printf() to report errors.
Modernize it to use error_vreport().
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qemu-img.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 8c7b2437f0..c376e91ca0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -85,13 +85,11 @@ static void QEMU_NORETURN GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...)
{
va_list ap;
- error_printf("qemu-img: ");
-
va_start(ap, fmt);
- error_vprintf(fmt, ap);
+ error_vreport(fmt, ap);
va_end(ap);
- error_printf("\nTry 'qemu-img --help' for more information\n");
+ error_printf("Try 'qemu-img --help' for more information\n");
exit(EXIT_FAILURE);
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 01/15] qemu-img: Use error_vreport() in error_exit()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
error_exit() uses low-level error_printf() to report errors.
Modernize it to use error_vreport().
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qemu-img.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 8c7b2437f0..c376e91ca0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -85,13 +85,11 @@ static void QEMU_NORETURN GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...)
{
va_list ap;
- error_printf("qemu-img: ");
-
va_start(ap, fmt);
- error_vprintf(fmt, ap);
+ error_vreport(fmt, ap);
va_end(ap);
- error_printf("\nTry 'qemu-img --help' for more information\n");
+ error_printf("Try 'qemu-img --help' for more information\n");
exit(EXIT_FAILURE);
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 02/15] block/ssh: Do not report read/write/flush errors to the user
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Richard W.M. Jones, Kevin Wolf, Max Reitz, qemu-block
Callbacks ssh_co_readv(), ssh_co_writev(), ssh_co_flush() report
errors to the user with error_printf(). They shouldn't, it's their
caller's job. Replace by a suitable trace point. While there, drop
the unreachable !s->sftp case.
Perhaps we should convert this part of the block driver interface to
Error, so block drivers can pass more detail to their callers. Not
today.
Cc: "Richard W.M. Jones" <rjones@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/ssh.c | 38 +++++++++++++-------------------------
block/trace-events | 3 +++
2 files changed, 16 insertions(+), 25 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index 190ef95300..859249113d 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -159,31 +159,19 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
g_free(msg);
}
-static void GCC_FMT_ATTR(2, 3)
-sftp_error_report(BDRVSSHState *s, const char *fs, ...)
+static void sftp_error_trace(BDRVSSHState *s, const char *op)
{
- va_list args;
+ char *ssh_err;
+ int ssh_err_code;
+ unsigned long sftp_err_code;
- va_start(args, fs);
- error_vprintf(fs, args);
+ /* This is not an errno. See <libssh2.h>. */
+ ssh_err_code = libssh2_session_last_error(s->session,
+ &ssh_err, NULL, 0);
+ /* See <libssh2_sftp.h>. */
+ sftp_err_code = libssh2_sftp_last_error((s)->sftp);
- if ((s)->sftp) {
- char *ssh_err;
- int ssh_err_code;
- unsigned long sftp_err_code;
-
- /* This is not an errno. See <libssh2.h>. */
- ssh_err_code = libssh2_session_last_error(s->session,
- &ssh_err, NULL, 0);
- /* See <libssh2_sftp.h>. */
- sftp_err_code = libssh2_sftp_last_error((s)->sftp);
-
- error_printf(": %s (libssh2 error code: %d, sftp error code: %lu)",
- ssh_err, ssh_err_code, sftp_err_code);
- }
-
- va_end(args);
- error_printf("\n");
+ trace_sftp_error(op, ssh_err, ssh_err_code, sftp_err_code);
}
static int parse_uri(const char *filename, QDict *options, Error **errp)
@@ -1035,7 +1023,7 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
goto again;
}
if (r < 0) {
- sftp_error_report(s, "read failed");
+ sftp_error_trace(s, "read");
s->offset = -1;
return -EIO;
}
@@ -1105,7 +1093,7 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
goto again;
}
if (r < 0) {
- sftp_error_report(s, "write failed");
+ sftp_error_trace(s, "write");
s->offset = -1;
return -EIO;
}
@@ -1188,7 +1176,7 @@ static coroutine_fn int ssh_flush(BDRVSSHState *s, BlockDriverState *bs)
return 0;
}
if (r < 0) {
- sftp_error_report(s, "fsync failed");
+ sftp_error_trace(s, "fsync");
return -EIO;
}
diff --git a/block/trace-events b/block/trace-events
index 7335a42540..79ccd8d824 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -208,3 +208,6 @@ sheepdog_co_rw_vector_new(uint64_t oid) "new oid 0x%" PRIx64
sheepdog_snapshot_create_info(const char *sn_name, const char *id, const char *name, int64_t size, int is_snapshot) "sn_info: name %s id_str %s s: name %s vm_state_size %" PRId64 " " "is_snapshot %d"
sheepdog_snapshot_create(const char *sn_name, const char *id) "%s %s"
sheepdog_snapshot_create_inode(const char *name, uint32_t snap, uint32_t vdi) "s->inode: name %s snap_id 0x%" PRIx32 " vdi 0x%" PRIx32
+
+# ssh.c
+sftp_error(const char *op, const char *ssh_err, int ssh_err_code, unsigned long sftp_err_code) "%s failed: %s (libssh2 error code: %d, sftp error code: %lu)"
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 02/15] block/ssh: Do not report read/write/flush errors to the user
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Richard W.M. Jones, qemu-block, Max Reitz
Callbacks ssh_co_readv(), ssh_co_writev(), ssh_co_flush() report
errors to the user with error_printf(). They shouldn't, it's their
caller's job. Replace by a suitable trace point. While there, drop
the unreachable !s->sftp case.
Perhaps we should convert this part of the block driver interface to
Error, so block drivers can pass more detail to their callers. Not
today.
Cc: "Richard W.M. Jones" <rjones@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/ssh.c | 38 +++++++++++++-------------------------
block/trace-events | 3 +++
2 files changed, 16 insertions(+), 25 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index 190ef95300..859249113d 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -159,31 +159,19 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
g_free(msg);
}
-static void GCC_FMT_ATTR(2, 3)
-sftp_error_report(BDRVSSHState *s, const char *fs, ...)
+static void sftp_error_trace(BDRVSSHState *s, const char *op)
{
- va_list args;
+ char *ssh_err;
+ int ssh_err_code;
+ unsigned long sftp_err_code;
- va_start(args, fs);
- error_vprintf(fs, args);
+ /* This is not an errno. See <libssh2.h>. */
+ ssh_err_code = libssh2_session_last_error(s->session,
+ &ssh_err, NULL, 0);
+ /* See <libssh2_sftp.h>. */
+ sftp_err_code = libssh2_sftp_last_error((s)->sftp);
- if ((s)->sftp) {
- char *ssh_err;
- int ssh_err_code;
- unsigned long sftp_err_code;
-
- /* This is not an errno. See <libssh2.h>. */
- ssh_err_code = libssh2_session_last_error(s->session,
- &ssh_err, NULL, 0);
- /* See <libssh2_sftp.h>. */
- sftp_err_code = libssh2_sftp_last_error((s)->sftp);
-
- error_printf(": %s (libssh2 error code: %d, sftp error code: %lu)",
- ssh_err, ssh_err_code, sftp_err_code);
- }
-
- va_end(args);
- error_printf("\n");
+ trace_sftp_error(op, ssh_err, ssh_err_code, sftp_err_code);
}
static int parse_uri(const char *filename, QDict *options, Error **errp)
@@ -1035,7 +1023,7 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
goto again;
}
if (r < 0) {
- sftp_error_report(s, "read failed");
+ sftp_error_trace(s, "read");
s->offset = -1;
return -EIO;
}
@@ -1105,7 +1093,7 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
goto again;
}
if (r < 0) {
- sftp_error_report(s, "write failed");
+ sftp_error_trace(s, "write");
s->offset = -1;
return -EIO;
}
@@ -1188,7 +1176,7 @@ static coroutine_fn int ssh_flush(BDRVSSHState *s, BlockDriverState *bs)
return 0;
}
if (r < 0) {
- sftp_error_report(s, "fsync failed");
+ sftp_error_trace(s, "fsync");
return -EIO;
}
diff --git a/block/trace-events b/block/trace-events
index 7335a42540..79ccd8d824 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -208,3 +208,6 @@ sheepdog_co_rw_vector_new(uint64_t oid) "new oid 0x%" PRIx64
sheepdog_snapshot_create_info(const char *sn_name, const char *id, const char *name, int64_t size, int is_snapshot) "sn_info: name %s id_str %s s: name %s vm_state_size %" PRId64 " " "is_snapshot %d"
sheepdog_snapshot_create(const char *sn_name, const char *id) "%s %s"
sheepdog_snapshot_create_inode(const char *name, uint32_t snap, uint32_t vdi) "s->inode: name %s snap_id 0x%" PRIx32 " vdi 0x%" PRIx32
+
+# ssh.c
+sftp_error(const char *op, const char *ssh_err, int ssh_err_code, unsigned long sftp_err_code) "%s failed: %s (libssh2 error code: %d, sftp error code: %lu)"
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 03/15] loader-fit: Wean off error_printf()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Paul Burton, Aleksandar Rikalo
load_fit() reports errors with error_printf() instead of
error_report(). Worse, it even reports errors it actually recovers
from, in fit_cfg_compatible() and fit_load_fdt(). Messed up in
initial commit 51b58561c1d.
Convert the helper functions for load_fit() to Error. Make sure each
failure path sets an error.
Fix fit_cfg_compatible() and fit_load_fdt() not to report errors they
actually recover from.
Convert load_fit() to error_report().
Cc: Paul Burton <pburton@wavecomp.com>
Cc: Aleksandar Rikalo <arikalo@wavecomp.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/core/loader-fit.c | 62 +++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 26 deletions(-)
diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 447f60857d..f27b6af942 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -18,6 +18,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/units.h"
#include "exec/memory.h"
#include "hw/loader.h"
@@ -33,7 +34,7 @@
#define FIT_LOADER_MAX_PATH (128)
static const void *fit_load_image_alloc(const void *itb, const char *name,
- int *poff, size_t *psz)
+ int *poff, size_t *psz, Error **errp)
{
const void *data;
const char *comp;
@@ -46,6 +47,7 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
off = fdt_path_offset(itb, path);
if (off < 0) {
+ error_setg(errp, "can't find node %s", path);
return NULL;
}
if (poff) {
@@ -54,6 +56,7 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
data = fdt_getprop(itb, off, "data", &sz);
if (!data) {
+ error_setg(errp, "can't get %s/data", path);
return NULL;
}
@@ -73,7 +76,7 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
uncomp_len = gunzip(uncomp_data, uncomp_len, (void *) data, sz);
if (uncomp_len < 0) {
- error_printf("unable to decompress %s image\n", name);
+ error_setg(errp, "unable to decompress %s image", name);
g_free(uncomp_data);
return NULL;
}
@@ -85,18 +88,19 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
return data;
}
- error_printf("unknown compression '%s'\n", comp);
+ error_setg(errp, "unknown compression '%s'", comp);
return NULL;
}
static int fit_image_addr(const void *itb, int img, const char *name,
- hwaddr *addr)
+ hwaddr *addr, Error **errp)
{
const void *prop;
int len;
prop = fdt_getprop(itb, img, name, &len);
if (!prop) {
+ error_setg(errp, "can't find %s address", name);
return -ENOENT;
}
@@ -108,13 +112,14 @@ static int fit_image_addr(const void *itb, int img, const char *name,
*addr = fdt64_to_cpu(*(fdt64_t *)prop);
return 0;
default:
- error_printf("invalid %s address length %d\n", name, len);
+ error_setg(errp, "invalid %s address length %d", name, len);
return -EINVAL;
}
}
static int fit_load_kernel(const struct fit_loader *ldr, const void *itb,
- int cfg, void *opaque, hwaddr *pend)
+ int cfg, void *opaque, hwaddr *pend,
+ Error **errp)
{
const char *name;
const void *data;
@@ -126,26 +131,26 @@ static int fit_load_kernel(const struct fit_loader *ldr, const void *itb,
name = fdt_getprop(itb, cfg, "kernel", NULL);
if (!name) {
- error_printf("no kernel specified by FIT configuration\n");
+ error_setg(errp, "no kernel specified by FIT configuration");
return -EINVAL;
}
- load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz);
+ load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
if (!data) {
- error_printf("unable to load kernel image from FIT\n");
+ error_prepend(errp, "unable to load kernel image from FIT: ");
return -EINVAL;
}
- err = fit_image_addr(itb, img_off, "load", &load_addr);
+ err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
if (err) {
- error_printf("unable to read kernel load address from FIT\n");
+ error_prepend(errp, "unable to read kernel load address from FIT: ");
ret = err;
goto out;
}
- err = fit_image_addr(itb, img_off, "entry", &entry_addr);
+ err = fit_image_addr(itb, img_off, "entry", &entry_addr, errp);
if (err) {
- error_printf("unable to read kernel entry address from FIT\n");
+ error_prepend(errp, "unable to read kernel entry address from FIT: ");
ret = err;
goto out;
}
@@ -172,7 +177,7 @@ out:
static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
int cfg, void *opaque, const void *match_data,
- hwaddr kernel_end)
+ hwaddr kernel_end, Error **errp)
{
const char *name;
const void *data;
@@ -187,16 +192,18 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
return 0;
}
- load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz);
+ load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
if (!data) {
- error_printf("unable to load FDT image from FIT\n");
+ error_prepend(errp, "unable to load FDT image from FIT: ");
return -EINVAL;
}
- err = fit_image_addr(itb, img_off, "load", &load_addr);
+ err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
if (err == -ENOENT) {
load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
+ error_free(*errp);
} else if (err) {
+ error_prepend(errp, "unable to read FDT load address from FIT: ");
ret = err;
goto out;
}
@@ -229,7 +236,7 @@ static bool fit_cfg_compatible(const void *itb, int cfg, const char *compat)
return false;
}
- fdt = fit_load_image_alloc(itb, fdt_name, NULL, NULL);
+ fdt = fit_load_image_alloc(itb, fdt_name, NULL, NULL, NULL);
if (!fdt) {
return false;
}
@@ -252,11 +259,12 @@ out:
int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque)
{
+ Error *err = NULL;
const struct fit_loader_match *match;
const void *itb, *match_data = NULL;
const char *def_cfg_name;
char path[FIT_LOADER_MAX_PATH];
- int itb_size, configs, cfg_off, off, err;
+ int itb_size, configs, cfg_off, off;
hwaddr kernel_end;
int ret;
@@ -267,6 +275,7 @@ int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque)
configs = fdt_path_offset(itb, "/configurations");
if (configs < 0) {
+ error_report("can't find node /configurations");
ret = configs;
goto out;
}
@@ -301,20 +310,21 @@ int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque)
}
if (cfg_off < 0) {
- /* couldn't find a configuration to use */
+ error_report("can't find configuration");
ret = cfg_off;
goto out;
}
- err = fit_load_kernel(ldr, itb, cfg_off, opaque, &kernel_end);
- if (err) {
- ret = err;
+ ret = fit_load_kernel(ldr, itb, cfg_off, opaque, &kernel_end, &err);
+ if (ret) {
+ error_report_err(err);
goto out;
}
- err = fit_load_fdt(ldr, itb, cfg_off, opaque, match_data, kernel_end);
- if (err) {
- ret = err;
+ ret = fit_load_fdt(ldr, itb, cfg_off, opaque, match_data, kernel_end,
+ &err);
+ if (ret) {
+ error_report_err(err);
goto out;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 03/15] loader-fit: Wean off error_printf()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Aleksandar Rikalo, Paul Burton
load_fit() reports errors with error_printf() instead of
error_report(). Worse, it even reports errors it actually recovers
from, in fit_cfg_compatible() and fit_load_fdt(). Messed up in
initial commit 51b58561c1d.
Convert the helper functions for load_fit() to Error. Make sure each
failure path sets an error.
Fix fit_cfg_compatible() and fit_load_fdt() not to report errors they
actually recover from.
Convert load_fit() to error_report().
Cc: Paul Burton <pburton@wavecomp.com>
Cc: Aleksandar Rikalo <arikalo@wavecomp.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/core/loader-fit.c | 62 +++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 26 deletions(-)
diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 447f60857d..f27b6af942 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -18,6 +18,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/units.h"
#include "exec/memory.h"
#include "hw/loader.h"
@@ -33,7 +34,7 @@
#define FIT_LOADER_MAX_PATH (128)
static const void *fit_load_image_alloc(const void *itb, const char *name,
- int *poff, size_t *psz)
+ int *poff, size_t *psz, Error **errp)
{
const void *data;
const char *comp;
@@ -46,6 +47,7 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
off = fdt_path_offset(itb, path);
if (off < 0) {
+ error_setg(errp, "can't find node %s", path);
return NULL;
}
if (poff) {
@@ -54,6 +56,7 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
data = fdt_getprop(itb, off, "data", &sz);
if (!data) {
+ error_setg(errp, "can't get %s/data", path);
return NULL;
}
@@ -73,7 +76,7 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
uncomp_len = gunzip(uncomp_data, uncomp_len, (void *) data, sz);
if (uncomp_len < 0) {
- error_printf("unable to decompress %s image\n", name);
+ error_setg(errp, "unable to decompress %s image", name);
g_free(uncomp_data);
return NULL;
}
@@ -85,18 +88,19 @@ static const void *fit_load_image_alloc(const void *itb, const char *name,
return data;
}
- error_printf("unknown compression '%s'\n", comp);
+ error_setg(errp, "unknown compression '%s'", comp);
return NULL;
}
static int fit_image_addr(const void *itb, int img, const char *name,
- hwaddr *addr)
+ hwaddr *addr, Error **errp)
{
const void *prop;
int len;
prop = fdt_getprop(itb, img, name, &len);
if (!prop) {
+ error_setg(errp, "can't find %s address", name);
return -ENOENT;
}
@@ -108,13 +112,14 @@ static int fit_image_addr(const void *itb, int img, const char *name,
*addr = fdt64_to_cpu(*(fdt64_t *)prop);
return 0;
default:
- error_printf("invalid %s address length %d\n", name, len);
+ error_setg(errp, "invalid %s address length %d", name, len);
return -EINVAL;
}
}
static int fit_load_kernel(const struct fit_loader *ldr, const void *itb,
- int cfg, void *opaque, hwaddr *pend)
+ int cfg, void *opaque, hwaddr *pend,
+ Error **errp)
{
const char *name;
const void *data;
@@ -126,26 +131,26 @@ static int fit_load_kernel(const struct fit_loader *ldr, const void *itb,
name = fdt_getprop(itb, cfg, "kernel", NULL);
if (!name) {
- error_printf("no kernel specified by FIT configuration\n");
+ error_setg(errp, "no kernel specified by FIT configuration");
return -EINVAL;
}
- load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz);
+ load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
if (!data) {
- error_printf("unable to load kernel image from FIT\n");
+ error_prepend(errp, "unable to load kernel image from FIT: ");
return -EINVAL;
}
- err = fit_image_addr(itb, img_off, "load", &load_addr);
+ err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
if (err) {
- error_printf("unable to read kernel load address from FIT\n");
+ error_prepend(errp, "unable to read kernel load address from FIT: ");
ret = err;
goto out;
}
- err = fit_image_addr(itb, img_off, "entry", &entry_addr);
+ err = fit_image_addr(itb, img_off, "entry", &entry_addr, errp);
if (err) {
- error_printf("unable to read kernel entry address from FIT\n");
+ error_prepend(errp, "unable to read kernel entry address from FIT: ");
ret = err;
goto out;
}
@@ -172,7 +177,7 @@ out:
static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
int cfg, void *opaque, const void *match_data,
- hwaddr kernel_end)
+ hwaddr kernel_end, Error **errp)
{
const char *name;
const void *data;
@@ -187,16 +192,18 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
return 0;
}
- load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz);
+ load_data = data = fit_load_image_alloc(itb, name, &img_off, &sz, errp);
if (!data) {
- error_printf("unable to load FDT image from FIT\n");
+ error_prepend(errp, "unable to load FDT image from FIT: ");
return -EINVAL;
}
- err = fit_image_addr(itb, img_off, "load", &load_addr);
+ err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
if (err == -ENOENT) {
load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
+ error_free(*errp);
} else if (err) {
+ error_prepend(errp, "unable to read FDT load address from FIT: ");
ret = err;
goto out;
}
@@ -229,7 +236,7 @@ static bool fit_cfg_compatible(const void *itb, int cfg, const char *compat)
return false;
}
- fdt = fit_load_image_alloc(itb, fdt_name, NULL, NULL);
+ fdt = fit_load_image_alloc(itb, fdt_name, NULL, NULL, NULL);
if (!fdt) {
return false;
}
@@ -252,11 +259,12 @@ out:
int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque)
{
+ Error *err = NULL;
const struct fit_loader_match *match;
const void *itb, *match_data = NULL;
const char *def_cfg_name;
char path[FIT_LOADER_MAX_PATH];
- int itb_size, configs, cfg_off, off, err;
+ int itb_size, configs, cfg_off, off;
hwaddr kernel_end;
int ret;
@@ -267,6 +275,7 @@ int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque)
configs = fdt_path_offset(itb, "/configurations");
if (configs < 0) {
+ error_report("can't find node /configurations");
ret = configs;
goto out;
}
@@ -301,20 +310,21 @@ int load_fit(const struct fit_loader *ldr, const char *filename, void *opaque)
}
if (cfg_off < 0) {
- /* couldn't find a configuration to use */
+ error_report("can't find configuration");
ret = cfg_off;
goto out;
}
- err = fit_load_kernel(ldr, itb, cfg_off, opaque, &kernel_end);
- if (err) {
- ret = err;
+ ret = fit_load_kernel(ldr, itb, cfg_off, opaque, &kernel_end, &err);
+ if (ret) {
+ error_report_err(err);
goto out;
}
- err = fit_load_fdt(ldr, itb, cfg_off, opaque, match_data, kernel_end);
- if (err) {
- ret = err;
+ ret = fit_load_fdt(ldr, itb, cfg_off, opaque, match_data, kernel_end,
+ &err);
+ if (ret) {
+ error_report_err(err);
goto out;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 04/15] mips/boston: Report errors with error_report(), not error_printf()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Paul Burton, Aleksandar Rikalo
Cc: Paul Burton <pburton@wavecomp.com>
Cc: Aleksandar Rikalo <arikalo@wavecomp.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/mips/boston.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index e5bab3cadc..a8b29f62f5 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -528,21 +528,21 @@ static void boston_mach_init(MachineState *machine)
fw_size = load_image_targphys(machine->firmware,
0x1fc00000, 4 * MiB);
if (fw_size == -1) {
- error_printf("unable to load firmware image '%s'\n",
+ error_report("unable to load firmware image '%s'",
machine->firmware);
exit(1);
}
} else if (machine->kernel_filename) {
fit_err = load_fit(&boston_fit_loader, machine->kernel_filename, s);
if (fit_err) {
- error_printf("unable to load FIT image\n");
+ error_report("unable to load FIT image");
exit(1);
}
gen_firmware(memory_region_get_ram_ptr(flash) + 0x7c00000,
s->kernel_entry, s->fdt_base, is_64b);
} else if (!qtest_enabled()) {
- error_printf("Please provide either a -kernel or -bios argument\n");
+ error_report("Please provide either a -kernel or -bios argument");
exit(1);
}
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 04/15] mips/boston: Report errors with error_report(), not error_printf()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Aleksandar Rikalo, Paul Burton
Cc: Paul Burton <pburton@wavecomp.com>
Cc: Aleksandar Rikalo <arikalo@wavecomp.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/mips/boston.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index e5bab3cadc..a8b29f62f5 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -528,21 +528,21 @@ static void boston_mach_init(MachineState *machine)
fw_size = load_image_targphys(machine->firmware,
0x1fc00000, 4 * MiB);
if (fw_size == -1) {
- error_printf("unable to load firmware image '%s'\n",
+ error_report("unable to load firmware image '%s'",
machine->firmware);
exit(1);
}
} else if (machine->kernel_filename) {
fit_err = load_fit(&boston_fit_loader, machine->kernel_filename, s);
if (fit_err) {
- error_printf("unable to load FIT image\n");
+ error_report("unable to load FIT image");
exit(1);
}
gen_firmware(memory_region_get_ram_ptr(flash) + 0x7c00000,
s->kernel_entry, s->fdt_base, is_64b);
} else if (!qtest_enabled()) {
- error_printf("Please provide either a -kernel or -bios argument\n");
+ error_report("Please provide either a -kernel or -bios argument");
exit(1);
}
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 05/15] pci: Report fatal errors with error_report(), not error_printf()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Marcel Apfelbaum
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
hw/pci/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6d13ef877b..1808b242dd 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -951,7 +951,7 @@ static uint16_t pci_req_id_cache_extract(PCIReqIDCache *cache)
result = PCI_BUILD_BDF(bus_n, 0);
break;
default:
- error_printf("Invalid PCI requester ID cache type: %d\n",
+ error_report("Invalid PCI requester ID cache type: %d",
cache->type);
exit(1);
break;
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 05/15] pci: Report fatal errors with error_report(), not error_printf()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
hw/pci/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6d13ef877b..1808b242dd 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -951,7 +951,7 @@ static uint16_t pci_req_id_cache_extract(PCIReqIDCache *cache)
result = PCI_BUILD_BDF(bus_n, 0);
break;
default:
- error_printf("Invalid PCI requester ID cache type: %d\n",
+ error_report("Invalid PCI requester ID cache type: %d",
cache->type);
exit(1);
break;
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 06/15] hpet: Report warnings with warn_report(), not error_printf()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Paolo Bonzini
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/timer/hpet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index d97436bc7b..41024f39fb 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -744,7 +744,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
HPETTimer *timer;
if (!s->intcap) {
- error_printf("Hpet's intcap not initialized.\n");
+ warn_report("Hpet's intcap not initialized");
}
if (hpet_cfg.count == UINT8_MAX) {
/* first instance */
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 06/15] hpet: Report warnings with warn_report(), not error_printf()
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Michael S. Tsirkin
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/timer/hpet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index d97436bc7b..41024f39fb 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -744,7 +744,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
HPETTimer *timer;
if (!s->intcap) {
- error_printf("Hpet's intcap not initialized.\n");
+ warn_report("Hpet's intcap not initialized");
}
if (hpet_cfg.count == UINT8_MAX) {
/* first instance */
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 07/15] vfio: Report warnings with warn_report(), not error_printf()
2019-04-17 19:06 [Qemu-devel] [PATCH v3 00/15] Clean up use of error_printf() Markus Armbruster
` (5 preceding siblings ...)
2019-04-17 19:06 ` Markus Armbruster
@ 2019-04-17 19:06 ` Markus Armbruster
2019-04-17 22:05 ` Alex Williamson
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 08/15] s390x/kvm: " Markus Armbruster
` (7 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/vfio/pci.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 504019c458..0142819ea6 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -947,8 +947,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
/* Since pci handles romfile, just print a message and return */
if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
- error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified romfile\n",
- vdev->vbasedev.name);
+ warn_report("Device at %s is known to cause system instability"
+ " issues during option rom execution",
+ vdev->vbasedev.name);
+ error_printf("Proceeding anyway since user specified romfile\n");
}
return;
}
@@ -973,11 +975,16 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
if (vfio_blacklist_opt_rom(vdev)) {
if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
- error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified non zero value for rombar\n",
- vdev->vbasedev.name);
+ warn_report("Device at %s is known to cause system instability"
+ " issues during option rom execution",
+ vdev->vbasedev.name);
+ error_printf("Proceeding anyway since user specified"
+ " non zero value for rombar\n");
} else {
- error_printf("Warning : Rom loading for device at %s has been disabled due to system instability issues. Specify rombar=1 or romfile to force\n",
- vdev->vbasedev.name);
+ warn_report("Rom loading for device at %s has been disabled"
+ " due to system instability issues",
+ vdev->vbasedev.name);
+ error_printf("Specify rombar=1 or romfile to force\n");
return;
}
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v3 07/15] vfio: Report warnings with warn_report(), not error_printf()
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 07/15] vfio: " Markus Armbruster
@ 2019-04-17 22:05 ` Alex Williamson
2019-04-18 6:18 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Alex Williamson @ 2019-04-17 22:05 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On Wed, 17 Apr 2019 21:06:33 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/vfio/pci.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 504019c458..0142819ea6 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -947,8 +947,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> /* Since pci handles romfile, just print a message and return */
> if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
> - error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified romfile\n",
> - vdev->vbasedev.name);
> + warn_report("Device at %s is known to cause system instability"
> + " issues during option rom execution",
> + vdev->vbasedev.name);
> + error_printf("Proceeding anyway since user specified romfile\n");
I'm confused, the original warning is "this device is know to have
issues, proceeding because you asked me to". Are we categorizing the
first half as a warning and the latter as random uncategorized error
spew? Did an automated script chunk it this way because of the period
and strict application of the "single phrase" specification of
warn_report()? If this is the recommended semantics, I'm not sure how
I'd know to generate this myself for similar situations. Should we
instead try to express this in something acceptable as a single
phrase? Thanks,
Alex
> }
> return;
> }
> @@ -973,11 +975,16 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>
> if (vfio_blacklist_opt_rom(vdev)) {
> if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
> - error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified non zero value for rombar\n",
> - vdev->vbasedev.name);
> + warn_report("Device at %s is known to cause system instability"
> + " issues during option rom execution",
> + vdev->vbasedev.name);
> + error_printf("Proceeding anyway since user specified"
> + " non zero value for rombar\n");
> } else {
> - error_printf("Warning : Rom loading for device at %s has been disabled due to system instability issues. Specify rombar=1 or romfile to force\n",
> - vdev->vbasedev.name);
> + warn_report("Rom loading for device at %s has been disabled"
> + " due to system instability issues",
> + vdev->vbasedev.name);
> + error_printf("Specify rombar=1 or romfile to force\n");
> return;
> }
> }
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v3 07/15] vfio: Report warnings with warn_report(), not error_printf()
2019-04-17 22:05 ` Alex Williamson
@ 2019-04-18 6:18 ` Markus Armbruster
2019-04-18 16:36 ` Alex Williamson
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2019-04-18 6:18 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel
Alex Williamson <alex.williamson@redhat.com> writes:
> On Wed, 17 Apr 2019 21:06:33 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> hw/vfio/pci.c | 19 +++++++++++++------
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 504019c458..0142819ea6 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -947,8 +947,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>> if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
>> /* Since pci handles romfile, just print a message and return */
>> if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
>> - error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified romfile\n",
>> - vdev->vbasedev.name);
>> + warn_report("Device at %s is known to cause system instability"
>> + " issues during option rom execution",
>> + vdev->vbasedev.name);
>> + error_printf("Proceeding anyway since user specified romfile\n");
>
> I'm confused, the original warning is "this device is know to have
> issues, proceeding because you asked me to". Are we categorizing the
> first half as a warning and the latter as random uncategorized error
> spew? Did an automated script chunk it this way because of the period
> and strict application of the "single phrase" specification of
> warn_report()? If this is the recommended semantics, I'm not sure how
> I'd know to generate this myself for similar situations. Should we
> instead try to express this in something acceptable as a single
> phrase? Thanks,
This is an instance of the following error reporting pattern:
concise error / warning message (one line, single phrase)
additional information (free format)
We use error_report() / warn_report() for the former (this adds
decorations to the message), and error_printf() for the latter.
"git-grep -w error_printf" will lead you to other instances. Recommend
to look with this series applied, because it removes misuses of
error_printf().
Particularly relevant instances are error_report_err() and
warn_report_err(): these print the error proper with error_report() /
warn_report_err(), and the hint, if any, with error_printf().
Clearer now?
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v3 07/15] vfio: Report warnings with warn_report(), not error_printf()
2019-04-18 6:18 ` Markus Armbruster
@ 2019-04-18 16:36 ` Alex Williamson
2019-04-18 20:25 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Alex Williamson @ 2019-04-18 16:36 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On Thu, 18 Apr 2019 08:18:56 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
>
> > On Wed, 17 Apr 2019 21:06:33 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Cc: Alex Williamson <alex.williamson@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >> hw/vfio/pci.c | 19 +++++++++++++------
> >> 1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index 504019c458..0142819ea6 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -947,8 +947,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> >> if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> >> /* Since pci handles romfile, just print a message and return */
> >> if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
> >> - error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified romfile\n",
> >> - vdev->vbasedev.name);
> >> + warn_report("Device at %s is known to cause system instability"
> >> + " issues during option rom execution",
> >> + vdev->vbasedev.name);
> >> + error_printf("Proceeding anyway since user specified romfile\n");
> >
> > I'm confused, the original warning is "this device is know to have
> > issues, proceeding because you asked me to". Are we categorizing the
> > first half as a warning and the latter as random uncategorized error
> > spew? Did an automated script chunk it this way because of the period
> > and strict application of the "single phrase" specification of
> > warn_report()? If this is the recommended semantics, I'm not sure how
> > I'd know to generate this myself for similar situations. Should we
> > instead try to express this in something acceptable as a single
> > phrase? Thanks,
>
> This is an instance of the following error reporting pattern:
>
> concise error / warning message (one line, single phrase)
> additional information (free format)
>
> We use error_report() / warn_report() for the former (this adds
> decorations to the message), and error_printf() for the latter.
>
> "git-grep -w error_printf" will lead you to other instances. Recommend
> to look with this series applied, because it removes misuses of
> error_printf().
>
> Particularly relevant instances are error_report_err() and
> warn_report_err(): these print the error proper with error_report() /
> warn_report_err(), and the hint, if any, with error_printf().
>
> Clearer now?
I can't guarantee that I'd be able to reproduce these sorts of
semantics without prompting, but yes, there does seem to be some method
to the madness ;) Thanks,
Acked-by: Alex Williamson <alex.williamson@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v3 07/15] vfio: Report warnings with warn_report(), not error_printf()
2019-04-18 16:36 ` Alex Williamson
@ 2019-04-18 20:25 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-18 20:25 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel
Alex Williamson <alex.williamson@redhat.com> writes:
> On Thu, 18 Apr 2019 08:18:56 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Alex Williamson <alex.williamson@redhat.com> writes:
>>
>> > On Wed, 17 Apr 2019 21:06:33 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Cc: Alex Williamson <alex.williamson@redhat.com>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >> hw/vfio/pci.c | 19 +++++++++++++------
>> >> 1 file changed, 13 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> >> index 504019c458..0142819ea6 100644
>> >> --- a/hw/vfio/pci.c
>> >> +++ b/hw/vfio/pci.c
>> >> @@ -947,8 +947,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>> >> if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
>> >> /* Since pci handles romfile, just print a message and return */
>> >> if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
>> >> - error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified romfile\n",
>> >> - vdev->vbasedev.name);
>> >> + warn_report("Device at %s is known to cause system instability"
>> >> + " issues during option rom execution",
>> >> + vdev->vbasedev.name);
>> >> + error_printf("Proceeding anyway since user specified romfile\n");
>> >
>> > I'm confused, the original warning is "this device is know to have
>> > issues, proceeding because you asked me to". Are we categorizing the
>> > first half as a warning and the latter as random uncategorized error
>> > spew? Did an automated script chunk it this way because of the period
>> > and strict application of the "single phrase" specification of
>> > warn_report()? If this is the recommended semantics, I'm not sure how
>> > I'd know to generate this myself for similar situations. Should we
>> > instead try to express this in something acceptable as a single
>> > phrase? Thanks,
>>
>> This is an instance of the following error reporting pattern:
>>
>> concise error / warning message (one line, single phrase)
>> additional information (free format)
>>
>> We use error_report() / warn_report() for the former (this adds
>> decorations to the message), and error_printf() for the latter.
>>
>> "git-grep -w error_printf" will lead you to other instances. Recommend
>> to look with this series applied, because it removes misuses of
>> error_printf().
>>
>> Particularly relevant instances are error_report_err() and
>> warn_report_err(): these print the error proper with error_report() /
>> warn_report_err(), and the hint, if any, with error_printf().
>>
>> Clearer now?
>
> I can't guarantee that I'd be able to reproduce these sorts of
> semantics without prompting, but yes, there does seem to be some method
> to the madness ;) Thanks,
Hopefully, presence of good examples, absence of bad examples, and
review will do the trick often enough.
> Acked-by: Alex Williamson <alex.williamson@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 08/15] s390x/kvm: Report warnings with warn_report(), not error_printf()
2019-04-17 19:06 [Qemu-devel] [PATCH v3 00/15] Clean up use of error_printf() Markus Armbruster
` (6 preceding siblings ...)
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 07/15] vfio: " Markus Armbruster
@ 2019-04-17 19:06 ` Markus Armbruster
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 09/15] vl: Make -machine $TYPE, help and -accel help print to stdout Markus Armbruster
` (6 subsequent siblings)
14 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Thomas Huth, qemu-s390x
kvm_s390_mem_op() can fail in two ways: when !cap_mem_op, it returns
-ENOSYS, and when kvm_vcpu_ioctl() fails, it returns -errno set by
ioctl(). Its caller s390_cpu_virt_mem_rw() recovers from both
failures.
kvm_s390_mem_op() prints "KVM_S390_MEM_OP failed" with error_printf()
in the latter failure mode. Since this is obviously a warning, use
warn_report().
Perhaps the reporting should be left to the caller. It could warn on
failure other than -ENOSYS.
Cc: Thomas Huth <thuth@redhat.com>
Cc: qemu-s390x@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
target/s390x/kvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 19530fb94e..2c6e35b5aa 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -782,7 +782,7 @@ int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf,
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_S390_MEM_OP, &mem_op);
if (ret < 0) {
- error_printf("KVM_S390_MEM_OP failed: %s\n", strerror(-ret));
+ warn_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
}
return ret;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 09/15] vl: Make -machine $TYPE, help and -accel help print to stdout
2019-04-17 19:06 [Qemu-devel] [PATCH v3 00/15] Clean up use of error_printf() Markus Armbruster
` (7 preceding siblings ...)
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 08/15] s390x/kvm: " Markus Armbruster
@ 2019-04-17 19:06 ` Markus Armbruster
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 10/15] monitor error: Make printf()-like functions return a value Markus Armbruster
` (5 subsequent siblings)
14 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel
Command line help help explicitly requested by the user should be
printed to stdout, not stderr. We do elsewhere. Adjust -machine
$TYPE,help and -accel help to match: use printf() instead of
error_printf().
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
vl.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/vl.c b/vl.c
index 9877972d97..190c773176 100644
--- a/vl.c
+++ b/vl.c
@@ -1556,12 +1556,12 @@ static int machine_help_func(QemuOpts *opts, MachineState *machine)
continue;
}
- error_printf("%s.%s=%s", MACHINE_GET_CLASS(machine)->name,
- prop->name, prop->type);
+ printf("%s.%s=%s", MACHINE_GET_CLASS(machine)->name,
+ prop->name, prop->type);
if (prop->description) {
- error_printf(" (%s)\n", prop->description);
+ printf(" (%s)\n", prop->description);
} else {
- error_printf("\n");
+ printf("\n");
}
}
@@ -3643,7 +3643,7 @@ int main(int argc, char **argv, char **envp)
optarg, true);
optarg = qemu_opt_get(accel_opts, "accel");
if (!optarg || is_help_option(optarg)) {
- error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
+ printf("Possible accelerators: kvm, xen, hax, tcg\n");
exit(0);
}
opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 10/15] monitor error: Make printf()-like functions return a value
2019-04-17 19:06 [Qemu-devel] [PATCH v3 00/15] Clean up use of error_printf() Markus Armbruster
` (8 preceding siblings ...)
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 09/15] vl: Make -machine $TYPE, help and -accel help print to stdout Markus Armbruster
@ 2019-04-17 19:06 ` Markus Armbruster
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 11/15] qemu-print: New qemu_printf(), qemu_vprintf() etc Markus Armbruster
` (4 subsequent siblings)
14 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Dr . David Alan Gilbert
printf() & friends return the number of characters written on success,
negative value on error.
monitor_printf(), monitor_vfprintf(), monitor_vprintf(),
error_printf(), error_printf_unless_qmp(), error_vprintf(), and
error_vprintf_unless_qmp() return void. Some of them carry a TODO
comment asking for int instead.
Improve them to return int like printf() does.
This makes our use of monitor_printf() as fprintf_function slightly
less dirty: the function cast no longer adds a return value that isn't
there. It still changes a parameter's pointer type. That will be
addressed in a future commit.
monitor_vfprintf() always returns zero. Improve it to return the
proper value.
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
include/monitor/monitor.h | 8 ++---
include/qemu/error-report.h | 8 ++---
monitor.c | 61 ++++++++++++++++++++-----------------
stubs/error-printf.c | 13 +++++---
util/qemu-error.c | 12 +++++---
5 files changed, 57 insertions(+), 45 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index c1b40a9cac..e4c3717454 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -28,9 +28,9 @@ void monitor_resume(Monitor *mon);
int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp);
-void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
+int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
GCC_FMT_ATTR(2, 0);
-void monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+int monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
int monitor_fprintf(FILE *stream, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
void monitor_flush(Monitor *mon);
int monitor_set_cpu(int cpu_index);
@@ -48,7 +48,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
void monitor_fdset_dup_fd_remove(int dup_fd);
int monitor_fdset_dup_fd_find(int dup_fd);
-void monitor_vfprintf(FILE *stream,
- const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
+int monitor_vfprintf(FILE *stream,
+ const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
#endif /* MONITOR_H */
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index ce43c02314..00d069b20f 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -30,10 +30,10 @@ void loc_set_none(void);
void loc_set_cmdline(char **argv, int idx, int cnt);
void loc_set_file(const char *fname, int lno);
-void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
-void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
-void error_vprintf_unless_qmp(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
-void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
+int error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
+int error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
+int error_vprintf_unless_qmp(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
+int error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
void warn_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
diff --git a/monitor.c b/monitor.c
index 4807bbe811..7b4a78d798 100644
--- a/monitor.c
+++ b/monitor.c
@@ -430,15 +430,14 @@ void monitor_flush(Monitor *mon)
}
/* flush at every end of line */
-static void monitor_puts(Monitor *mon, const char *str)
+static int monitor_puts(Monitor *mon, const char *str)
{
+ int i;
char c;
qemu_mutex_lock(&mon->mon_lock);
- for(;;) {
- c = *str++;
- if (c == '\0')
- break;
+ for (i = 0; str[i]; i++) {
+ c = str[i];
if (c == '\n') {
qstring_append_chr(mon->outbuf, '\r');
}
@@ -448,39 +447,48 @@ static void monitor_puts(Monitor *mon, const char *str)
}
}
qemu_mutex_unlock(&mon->mon_lock);
+
+ return i;
}
-void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
+int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
{
char *buf;
+ int n;
if (!mon)
- return;
+ return -1;
if (monitor_is_qmp(mon)) {
- return;
+ return -1;
}
buf = g_strdup_vprintf(fmt, ap);
- monitor_puts(mon, buf);
+ n = monitor_puts(mon, buf);
g_free(buf);
+ return n;
}
-void monitor_printf(Monitor *mon, const char *fmt, ...)
+int monitor_printf(Monitor *mon, const char *fmt, ...)
{
+ int ret;
+
va_list ap;
va_start(ap, fmt);
- monitor_vprintf(mon, fmt, ap);
+ ret = monitor_vprintf(mon, fmt, ap);
va_end(ap);
+ return ret;
}
int monitor_fprintf(FILE *stream, const char *fmt, ...)
{
+ int ret;
+
va_list ap;
va_start(ap, fmt);
- monitor_vprintf((Monitor *)stream, fmt, ap);
+ ret = monitor_vprintf((Monitor *)stream, fmt, ap);
va_end(ap);
- return 0;
+ return ret;
}
static void qmp_send_response(Monitor *mon, const QDict *rsp)
@@ -4535,35 +4543,32 @@ static void monitor_readline_flush(void *opaque)
/*
* Print to current monitor if we have one, else to stream.
- * TODO should return int, so callers can calculate width, but that
- * requires surgery to monitor_vprintf(). Left for another day.
*/
-void monitor_vfprintf(FILE *stream, const char *fmt, va_list ap)
+int monitor_vfprintf(FILE *stream, const char *fmt, va_list ap)
{
if (cur_mon && !monitor_cur_is_qmp()) {
- monitor_vprintf(cur_mon, fmt, ap);
- } else {
- vfprintf(stream, fmt, ap);
+ return monitor_vprintf(cur_mon, fmt, ap);
}
+ return vfprintf(stream, fmt, ap);
}
/*
* Print to current monitor if we have one, else to stderr.
- * TODO should return int, so callers can calculate width, but that
- * requires surgery to monitor_vprintf(). Left for another day.
*/
-void error_vprintf(const char *fmt, va_list ap)
+int error_vprintf(const char *fmt, va_list ap)
{
- monitor_vfprintf(stderr, fmt, ap);
+ return monitor_vfprintf(stderr, fmt, ap);
}
-void error_vprintf_unless_qmp(const char *fmt, va_list ap)
+int error_vprintf_unless_qmp(const char *fmt, va_list ap)
{
- if (cur_mon && !monitor_cur_is_qmp()) {
- monitor_vprintf(cur_mon, fmt, ap);
- } else if (!cur_mon) {
- vfprintf(stderr, fmt, ap);
+ if (!cur_mon) {
+ return vfprintf(stderr, fmt, ap);
}
+ if (!monitor_cur_is_qmp()) {
+ return monitor_vprintf(cur_mon, fmt, ap);
+ }
+ return -1;
}
static void monitor_list_append(Monitor *mon)
diff --git a/stubs/error-printf.c b/stubs/error-printf.c
index 99c6406668..1f9d3b3714 100644
--- a/stubs/error-printf.c
+++ b/stubs/error-printf.c
@@ -2,19 +2,22 @@
#include "qemu-common.h"
#include "qemu/error-report.h"
-void error_vprintf(const char *fmt, va_list ap)
+int error_vprintf(const char *fmt, va_list ap)
{
+ int ret;
+
if (g_test_initialized() && !g_test_subprocess() &&
getenv("QTEST_SILENT_ERRORS")) {
char *msg = g_strdup_vprintf(fmt, ap);
g_test_message("%s", msg);
+ ret = strlen(msg);
g_free(msg);
- } else {
- vfprintf(stderr, fmt, ap);
+ return ret;
}
+ return vfprintf(stderr, fmt, ap);
}
-void error_vprintf_unless_qmp(const char *fmt, va_list ap)
+int error_vprintf_unless_qmp(const char *fmt, va_list ap)
{
- error_vprintf(fmt, ap);
+ return error_vprintf(fmt, ap);
}
diff --git a/util/qemu-error.c b/util/qemu-error.c
index d08139d9ac..f373f3b3b0 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -24,22 +24,26 @@ typedef enum {
REPORT_TYPE_INFO,
} report_type;
-void error_printf(const char *fmt, ...)
+int error_printf(const char *fmt, ...)
{
va_list ap;
+ int ret;
va_start(ap, fmt);
- error_vprintf(fmt, ap);
+ ret = error_vprintf(fmt, ap);
va_end(ap);
+ return ret;
}
-void error_printf_unless_qmp(const char *fmt, ...)
+int error_printf_unless_qmp(const char *fmt, ...)
{
va_list ap;
+ int ret;
va_start(ap, fmt);
- error_vprintf_unless_qmp(fmt, ap);
+ ret = error_vprintf_unless_qmp(fmt, ap);
va_end(ap);
+ return ret;
}
static Location std_loc = {
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 11/15] qemu-print: New qemu_printf(), qemu_vprintf() etc.
2019-04-17 19:06 [Qemu-devel] [PATCH v3 00/15] Clean up use of error_printf() Markus Armbruster
` (9 preceding siblings ...)
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 10/15] monitor error: Make printf()-like functions return a value Markus Armbruster
@ 2019-04-17 19:06 ` Markus Armbruster
2019-04-17 19:06 ` Markus Armbruster
` (3 subsequent siblings)
14 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Dr . David Alan Gilbert
We commonly want to print to the current monitor if we have one, else
to stdout/stderr. For stderr, have error_printf(). For stdout, all
we have is monitor_vfprintf(), which is rather unwieldy. We often
print to stderr just because error_printf() is easier.
New qemu_printf() and qemu_vprintf() do exactly what's needed. The
next commits will put them to use.
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
MAINTAINERS | 2 ++
include/qemu/qemu-print.h | 19 ++++++++++++++++++
stubs/monitor.c | 5 +++++
tests/test-util-sockets.c | 1 +
util/Makefile.objs | 1 +
util/qemu-print.c | 42 +++++++++++++++++++++++++++++++++++++++
6 files changed, 70 insertions(+)
create mode 100644 include/qemu/qemu-print.h
create mode 100644 util/qemu-print.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 56139ac8ab..1aa19dc4ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1895,6 +1895,8 @@ F: hmp.[ch]
F: hmp-commands*.hx
F: include/monitor/hmp-target.h
F: tests/test-hmp.c
+F: include/qemu/qemu-print.h
+F: util/qemu-print.c
Network device backends
M: Jason Wang <jasowang@redhat.com>
diff --git a/include/qemu/qemu-print.h b/include/qemu/qemu-print.h
new file mode 100644
index 0000000000..8fed32bf42
--- /dev/null
+++ b/include/qemu/qemu-print.h
@@ -0,0 +1,19 @@
+/*
+ * Print to stream or current monitor
+ *
+ * Copyright (C) 2019 Red Hat Inc.
+ *
+ * Authors:
+ * Markus Armbruster <armbru@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_PRINT_H
+#define QEMU_PRINT_H
+
+int qemu_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
+int qemu_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
+
+#endif
diff --git a/stubs/monitor.c b/stubs/monitor.c
index b57fe6c32f..b2ea975e40 100644
--- a/stubs/monitor.c
+++ b/stubs/monitor.c
@@ -6,6 +6,11 @@
__thread Monitor *cur_mon;
+int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
+{
+ abort();
+}
+
int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
{
error_setg(errp, "only QEMU supports file descriptor passing");
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 6195a3ac36..fd1ced058c 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -70,6 +70,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
* otherwise we get duplicate syms at link time.
*/
__thread Monitor *cur_mon;
+int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
void monitor_init(Chardev *chr, int flags) {}
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 835fcd69e2..9206878dec 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -23,6 +23,7 @@ util-obj-y += bitmap.o bitops.o hbitmap.o
util-obj-y += fifo8.o
util-obj-y += cacheinfo.o
util-obj-y += error.o qemu-error.o
+util-obj-y += qemu-print.o
util-obj-y += id.o
util-obj-y += iov.o qemu-config.o qemu-sockets.o uri.o notify.o
util-obj-y += qemu-option.o qemu-progress.o
diff --git a/util/qemu-print.c b/util/qemu-print.c
new file mode 100644
index 0000000000..86f9417af8
--- /dev/null
+++ b/util/qemu-print.c
@@ -0,0 +1,42 @@
+/*
+ * Print to stream or current monitor
+ *
+ * Copyright (C) 2019 Red Hat Inc.
+ *
+ * Authors:
+ * Markus Armbruster <armbru@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "monitor/monitor.h"
+#include "qemu/qemu-print.h"
+
+/*
+ * Print like vprintf().
+ * Print to current monitor if we have one, else to stdout.
+ */
+int qemu_vprintf(const char *fmt, va_list ap)
+{
+ if (cur_mon) {
+ return monitor_vprintf(cur_mon, fmt, ap);
+ }
+ return vprintf(fmt, ap);
+}
+
+/*
+ * Print like printf().
+ * Print to current monitor if we have one, else to stdout.
+ */
+int qemu_printf(const char *fmt, ...)
+{
+ va_list ap;
+ int ret;
+
+ va_start(ap, fmt);
+ ret = qemu_vprintf(fmt, ap);
+ va_end(ap);
+ return ret;
+}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 12/15] blockdev: Make -drive format=help print to stdout
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz, qemu-block
Command line help explicitly requested by the user should be printed
to stdout, not stderr. We do elsewhere. Adjust -drive to match: use
qemu_printf() instead of error_printf(). Plain printf() would be
wrong because we need to print to the current monitor for "drive_add
... format=help".
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
blockdev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 4775a07d93..79fbac8450 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -40,6 +40,7 @@
#include "monitor/monitor.h"
#include "qemu/error-report.h"
#include "qemu/option.h"
+#include "qemu/qemu-print.h"
#include "qemu/config-file.h"
#include "qapi/qapi-commands-block.h"
#include "qapi/qapi-commands-transaction.h"
@@ -301,7 +302,7 @@ DriveInfo *drive_get_next(BlockInterfaceType type)
static void bdrv_format_print(void *opaque, const char *name)
{
- error_printf(" %s", name);
+ qemu_printf(" %s", name);
}
typedef struct {
@@ -530,11 +531,11 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
if ((buf = qemu_opt_get(opts, "format")) != NULL) {
if (is_help_option(buf)) {
- error_printf("Supported formats:");
+ qemu_printf("Supported formats:");
bdrv_iterate_format(bdrv_format_print, NULL, false);
- error_printf("\nSupported formats (read-only):");
+ qemu_printf("\nSupported formats (read-only):");
bdrv_iterate_format(bdrv_format_print, NULL, true);
- error_printf("\n");
+ qemu_printf("\n");
goto early_err;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 12/15] blockdev: Make -drive format=help print to stdout
@ 2019-04-17 19:06 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
Command line help explicitly requested by the user should be printed
to stdout, not stderr. We do elsewhere. Adjust -drive to match: use
qemu_printf() instead of error_printf(). Plain printf() would be
wrong because we need to print to the current monitor for "drive_add
... format=help".
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
blockdev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 4775a07d93..79fbac8450 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -40,6 +40,7 @@
#include "monitor/monitor.h"
#include "qemu/error-report.h"
#include "qemu/option.h"
+#include "qemu/qemu-print.h"
#include "qemu/config-file.h"
#include "qapi/qapi-commands-block.h"
#include "qapi/qapi-commands-transaction.h"
@@ -301,7 +302,7 @@ DriveInfo *drive_get_next(BlockInterfaceType type)
static void bdrv_format_print(void *opaque, const char *name)
{
- error_printf(" %s", name);
+ qemu_printf(" %s", name);
}
typedef struct {
@@ -530,11 +531,11 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
if ((buf = qemu_opt_get(opts, "format")) != NULL) {
if (is_help_option(buf)) {
- error_printf("Supported formats:");
+ qemu_printf("Supported formats:");
bdrv_iterate_format(bdrv_format_print, NULL, false);
- error_printf("\nSupported formats (read-only):");
+ qemu_printf("\nSupported formats (read-only):");
bdrv_iterate_format(bdrv_format_print, NULL, true);
- error_printf("\n");
+ qemu_printf("\n");
goto early_err;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 13/15] char: Make -chardev help print to stdout
2019-04-17 19:06 [Qemu-devel] [PATCH v3 00/15] Clean up use of error_printf() Markus Armbruster
` (11 preceding siblings ...)
2019-04-17 19:06 ` Markus Armbruster
@ 2019-04-17 19:06 ` Markus Armbruster
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 14/15] char-pty: Print "char device redirected" message " Markus Armbruster
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 15/15] monitor: Simplify how -device/device_add print help Markus Armbruster
14 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Paolo Bonzini
Command line help explicitly requested by the user should be printed
to stdout, not stderr. We do elsewhere. Adjust -chardev to match:
use qemu_printf() instead of error_printf(). Plain printf() would be
wrong because we need to print to the current monitor for "chardev-add
help".
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
chardev/char.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/chardev/char.c b/chardev/char.c
index 514cd6b0c3..54724a56b1 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -28,6 +28,7 @@
#include "sysemu/sysemu.h"
#include "qemu/config-file.h"
#include "qemu/error-report.h"
+#include "qemu/qemu-print.h"
#include "chardev/char.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-char.h"
@@ -651,7 +652,7 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
chardev_name_foreach(help_string_append, str);
- error_printf("Available chardev backend types: %s\n", str->str);
+ qemu_printf("Available chardev backend types: %s\n", str->str);
g_string_free(str, true);
return NULL;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v3 14/15] char-pty: Print "char device redirected" message to stdout
2019-04-17 19:06 [Qemu-devel] [PATCH v3 00/15] Clean up use of error_printf() Markus Armbruster
` (12 preceding siblings ...)
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 13/15] char: Make -chardev help " Markus Armbruster
@ 2019-04-17 19:06 ` Markus Armbruster
2019-04-17 19:31 ` Eric Blake
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 15/15] monitor: Simplify how -device/device_add print help Markus Armbruster
14 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel
char_pty_open() prints a "char device redirected to PTY_NAME (label
LABEL)" message to the current monitor or else to stderr. This is not
an error, so it shouldn't go to stderr. Print it to stdout instead.
Why is it even printed? No other ChardevClass::open() prints anything
on success. It's because you need to know PTY_NAME to actually use
this char device, e.g. like e.g. "socat STDIO,cfmakeraw FILE:PTY_NAME"
to use the monitor's readline interface. You can get PTY_NAME with
"info chardev" (a.k.a. query-chardev for QMP), but only if you already
have a monitor.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
chardev/char-pty.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index b034332edd..04759b0ef9 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -28,6 +28,7 @@
#include "io/channel-file.h"
#include "qemu/sockets.h"
#include "qemu/error-report.h"
+#include "qemu/qemu-print.h"
#include "chardev/char-io.h"
@@ -211,8 +212,8 @@ static void char_pty_open(Chardev *chr,
qemu_set_nonblock(master_fd);
chr->filename = g_strdup_printf("pty:%s", pty_name);
- error_printf("char device redirected to %s (label %s)\n",
- pty_name, chr->label);
+ qemu_printf("char device redirected to %s (label %s)\n",
+ pty_name, chr->label);
s = PTY_CHARDEV(chr);
s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/15] char-pty: Print "char device redirected" message to stdout
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 14/15] char-pty: Print "char device redirected" message " Markus Armbruster
@ 2019-04-17 19:31 ` Eric Blake
0 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2019-04-17 19:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 981 bytes --]
On 4/17/19 2:06 PM, Markus Armbruster wrote:
> char_pty_open() prints a "char device redirected to PTY_NAME (label
> LABEL)" message to the current monitor or else to stderr. This is not
> an error, so it shouldn't go to stderr. Print it to stdout instead.
>
> Why is it even printed? No other ChardevClass::open() prints anything
> on success. It's because you need to know PTY_NAME to actually use
> this char device, e.g. like e.g. "socat STDIO,cfmakeraw FILE:PTY_NAME"
> to use the monitor's readline interface. You can get PTY_NAME with
> "info chardev" (a.k.a. query-chardev for QMP), but only if you already
> have a monitor.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> chardev/char-pty.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v3 15/15] monitor: Simplify how -device/device_add print help
2019-04-17 19:06 [Qemu-devel] [PATCH v3 00/15] Clean up use of error_printf() Markus Armbruster
` (13 preceding siblings ...)
2019-04-17 19:06 ` [Qemu-devel] [PATCH v3 14/15] char-pty: Print "char device redirected" message " Markus Armbruster
@ 2019-04-17 19:06 ` Markus Armbruster
14 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2019-04-17 19:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Dr . David Alan Gilbert
Commit a95db58f210 added monitor_vfprintf() as an error_printf()
generalized from stderr to arbitrary streams, then used it wrapped in
helper out_printf() to print -device/device_add help to stdout. Use
qemu_printf() instead, and delete monitor_vfprintf() and out_printf().
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
include/monitor/monitor.h | 3 ---
monitor.c | 16 ++++------------
qdev-monitor.c | 36 ++++++++++++++----------------------
3 files changed, 18 insertions(+), 37 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index e4c3717454..316a168c41 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -48,7 +48,4 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
void monitor_fdset_dup_fd_remove(int dup_fd);
int monitor_fdset_dup_fd_find(int dup_fd);
-int monitor_vfprintf(FILE *stream,
- const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
-
#endif /* MONITOR_H */
diff --git a/monitor.c b/monitor.c
index 7b4a78d798..10be8bdb86 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4541,23 +4541,15 @@ static void monitor_readline_flush(void *opaque)
monitor_flush(opaque);
}
-/*
- * Print to current monitor if we have one, else to stream.
- */
-int monitor_vfprintf(FILE *stream, const char *fmt, va_list ap)
-{
- if (cur_mon && !monitor_cur_is_qmp()) {
- return monitor_vprintf(cur_mon, fmt, ap);
- }
- return vfprintf(stream, fmt, ap);
-}
-
/*
* Print to current monitor if we have one, else to stderr.
*/
int error_vprintf(const char *fmt, va_list ap)
{
- return monitor_vfprintf(stderr, fmt, ap);
+ if (cur_mon && !monitor_cur_is_qmp()) {
+ return monitor_vprintf(cur_mon, fmt, ap);
+ }
+ return vfprintf(stderr, fmt, ap);
}
int error_vprintf_unless_qmp(const char *fmt, va_list ap)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index d4320986a2..373b9ad445 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -31,6 +31,7 @@
#include "qemu/error-report.h"
#include "qemu/help_option.h"
#include "qemu/option.h"
+#include "qemu/qemu-print.h"
#include "sysemu/block-backend.h"
#include "migration/misc.h"
@@ -104,31 +105,22 @@ static bool qdev_class_has_alias(DeviceClass *dc)
return (qdev_class_get_alias(dc) != NULL);
}
-static void out_printf(const char *fmt, ...)
-{
- va_list ap;
-
- va_start(ap, fmt);
- monitor_vfprintf(stdout, fmt, ap);
- va_end(ap);
-}
-
static void qdev_print_devinfo(DeviceClass *dc)
{
- out_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
+ qemu_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
if (dc->bus_type) {
- out_printf(", bus %s", dc->bus_type);
+ qemu_printf(", bus %s", dc->bus_type);
}
if (qdev_class_has_alias(dc)) {
- out_printf(", alias \"%s\"", qdev_class_get_alias(dc));
+ qemu_printf(", alias \"%s\"", qdev_class_get_alias(dc));
}
if (dc->desc) {
- out_printf(", desc \"%s\"", dc->desc);
+ qemu_printf(", desc \"%s\"", dc->desc);
}
if (!dc->user_creatable) {
- out_printf(", no-user");
+ qemu_printf(", no-user");
}
- out_printf("\n");
+ qemu_printf("\n");
}
static void qdev_print_devinfos(bool show_no_user)
@@ -164,7 +156,7 @@ static void qdev_print_devinfos(bool show_no_user)
continue;
}
if (!cat_printed) {
- out_printf("%s%s devices:\n", i ? "\n" : "", cat_name[i]);
+ qemu_printf("%s%s devices:\n", i ? "\n" : "", cat_name[i]);
cat_printed = true;
}
qdev_print_devinfo(dc);
@@ -286,20 +278,20 @@ int qdev_device_help(QemuOpts *opts)
}
if (prop_list) {
- out_printf("%s options:\n", driver);
+ qemu_printf("%s options:\n", driver);
} else {
- out_printf("There are no options for %s.\n", driver);
+ qemu_printf("There are no options for %s.\n", driver);
}
for (prop = prop_list; prop; prop = prop->next) {
int len;
- out_printf(" %s=<%s>%n", prop->value->name, prop->value->type, &len);
+ qemu_printf(" %s=<%s>%n", prop->value->name, prop->value->type, &len);
if (prop->value->has_description) {
if (len < 24) {
- out_printf("%*s", 24 - len, "");
+ qemu_printf("%*s", 24 - len, "");
}
- out_printf(" - %s\n", prop->value->description);
+ qemu_printf(" - %s\n", prop->value->description);
} else {
- out_printf("\n");
+ qemu_printf("\n");
}
}
--
2.17.2
^ permalink raw reply related [flat|nested] 28+ messages in thread