[Qemu-devel] [PATCH v2 00/15] Clean up use of error_printf()

[Qemu-devel] [PATCH v2 00/15] Clean up use of error_printf()

[Markus Armbruster]

This series cleans up two kinds of error_printf() misuse:

* Errors and warnings should be reported with error_report() and
  warn_report().

* Help output should be go to stdout, not stderr.

This is obviously for 4.1.  If nobody objects, I'll take the whole
series through my tree.

v2:
* PATCH 02: Commit message tweaked, indentation fixed [Eric]
* PATCH 10: Indentation fixed [Marcel]
* PATCH 11: Use-after-free fixed [Patchew]
* PATCH 12: test-util-sockets.c updated along with stubs/monitor.c
* PATCH 14: Commit message typo [Eric]

Markus Armbruster (15):
  qemu-img: Use error_vreport() in error_exit()
  block/ssh: Do not report read/write/flush errors to the user
  char-pty: Drop "char device redirected to" message
  loader-fit: Wean off error_printf()
  mips/boston: Report errors with error_report(), not error_printf()
  pci: Report fatal errors with error_report(), not error_printf()
  hpet: Report warnings with warn_report(), not error_printf()
  vfio: Report warnings with warn_report(), not error_printf()
  s390x/kvm: Report warnings with warn_report(), not error_printf()
  vl: Make -machine $TYPE,help and -accel help print to stdout
  monitor error: Make printf()-like functions return a value
  qemu-print: New qemu_printf(), qemu_vprintf() etc.
  blockdev: Make -drive format=help print to stdout
  char: Make -chardev help print to stdout
  monitor: Simplify how -device/device_add print help

 MAINTAINERS                 |  2 ++
 block/ssh.c                 | 38 +++++++-------------
 block/trace-events          |  3 ++
 blockdev.c                  |  9 ++---
 chardev/char-pty.c          |  2 --
 chardev/char.c              |  3 +-
 hw/core/loader-fit.c        | 62 +++++++++++++++++++--------------
 hw/mips/boston.c            |  6 ++--
 hw/pci/pci.c                |  2 +-
 hw/timer/hpet.c             |  2 +-
 hw/vfio/pci.c               | 19 ++++++----
 include/monitor/monitor.h   |  7 ++--
 include/qemu/error-report.h |  8 ++---
 include/qemu/qemu-print.h   | 19 ++++++++++
 monitor.c                   | 69 ++++++++++++++++++-------------------
 qdev-monitor.c              | 36 ++++++++-----------
 qemu-img.c                  |  6 ++--
 stubs/error-printf.c        | 13 ++++---
 stubs/monitor.c             |  5 +++
 target/s390x/kvm.c          |  2 +-
 tests/test-util-sockets.c   |  1 +
 util/Makefile.objs          |  1 +
 util/qemu-error.c           | 12 ++++---
 util/qemu-print.c           | 42 ++++++++++++++++++++++
 vl.c                        | 10 +++---
 25 files changed, 224 insertions(+), 155 deletions(-)
 create mode 100644 include/qemu/qemu-print.h
 create mode 100644 util/qemu-print.c

-- 
2.17.2

[Qemu-devel] [PATCH v2 01/15] qemu-img: Use error_vreport() in error_exit()

[Markus Armbruster]

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 aa6f81f1ea..55201fb913 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

[Qemu-devel] [PATCH v2 01/15] qemu-img: Use error_vreport() in error_exit()

[Markus Armbruster]

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 aa6f81f1ea..55201fb913 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

[Qemu-devel] [PATCH v2 02/15] block/ssh: Do not report read/write/flush errors to the user

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 02/15] block/ssh: Do not report read/write/flush errors to the user

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 03/15] char-pty: Drop "char device redirected to" message

[Markus Armbruster]

char_pty_open() prints a "char device redirected to PTY_NAME (label
LABEL)" message to the current monitor or else to stderr.  No other
ChardevClass::open() prints anything on success.  Drop the message.

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>
---
 chardev/char-pty.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index b034332edd..a48d3e5d20 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -211,8 +211,6 @@ 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);
 
     s = PTY_CHARDEV(chr);
     s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 03/15] char-pty: Drop "char device redirected to" message

[Paolo Bonzini]

On 11/04/19 16:52, 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.  No other
> ChardevClass::open() prints anything on success.  Drop the message.
> 
> 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>
> ---
>  chardev/char-pty.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index b034332edd..a48d3e5d20 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -211,8 +211,6 @@ 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);
>  
>      s = PTY_CHARDEV(chr);
>      s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));

The reason for the message is that the char device is completely useless
until the user knows the /dev/pts/N path[1].  You can get it with "info
chardev" (aka query-chardev for QMP) but there's an interesting chicken
and egg problem if the pty is for your monitor...

Paolo

[1] once you know it, you can use the monitor's readline interface with
e.g. "socat STDIO,cfmakeraw FILE:/dev/pts/1"

Re: [Qemu-devel] [PATCH v2 03/15] char-pty: Drop "char device redirected to" message

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/04/19 16:52, 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.  No other
>> ChardevClass::open() prints anything on success.  Drop the message.
>> 
>> 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>
>> ---
>>  chardev/char-pty.c | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>> index b034332edd..a48d3e5d20 100644
>> --- a/chardev/char-pty.c
>> +++ b/chardev/char-pty.c
>> @@ -211,8 +211,6 @@ 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);
>>  
>>      s = PTY_CHARDEV(chr);
>>      s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
>
> The reason for the message is that the char device is completely useless
> until the user knows the /dev/pts/N path[1].  You can get it with "info
> chardev" (aka query-chardev for QMP) but there's an interesting chicken
> and egg problem if the pty is for your monitor...
>
> Paolo

During review of v1, I wrote:

    If we should decide the message is still useful enough to be worth
    keeping, I could direct it to stdout instead of dropping it.

No clear conclusion emerged, so I did nothing for v2.  If we conclude to
keep the message now, I'll gladly do that.

> [1] once you know it, you can use the monitor's readline interface with
> e.g. "socat STDIO,cfmakeraw FILE:/dev/pts/1"

There's also

    $ socat UNIX:/path/to/socket READLINE,history=$HOME/.hmp_history,prompt='(qemu) '

Lacks completion.  But then our very own reimplementation of readline
lacks any number of other features.

Re: [Qemu-devel] [PATCH v2 03/15] char-pty: Drop "char device redirected to" message

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/04/19 16:52, 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.  No other
>> ChardevClass::open() prints anything on success.  Drop the message.
>> 
>> 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>
>> ---
>>  chardev/char-pty.c | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>> index b034332edd..a48d3e5d20 100644
>> --- a/chardev/char-pty.c
>> +++ b/chardev/char-pty.c
>> @@ -211,8 +211,6 @@ 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);
>>  
>>      s = PTY_CHARDEV(chr);
>>      s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
>
> The reason for the message is that the char device is completely useless
> until the user knows the /dev/pts/N path[1].  You can get it with "info
> chardev" (aka query-chardev for QMP) but there's an interesting chicken
> and egg problem if the pty is for your monitor...
>
> Paolo

During review of v1, I wrote:

    If we should decide the message is still useful enough to be worth
    keeping, I could direct it to stdout instead of dropping it.

No clear conclusion emerged, so I did nothing for v2.  If we conclude to
keep the message now, I'll gladly do that.

> [1] once you know it, you can use the monitor's readline interface with
> e.g. "socat STDIO,cfmakeraw FILE:/dev/pts/1"

There's also

    $ socat UNIX:/path/to/socket READLINE,history=$HOME/.hmp_history,prompt='(qemu) '

Lacks completion.  But then our very own reimplementation of readline
lacks any number of other features.

Re: [Qemu-devel] [PATCH v2 03/15] char-pty: Drop "char device redirected to" message

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 11/04/19 16:52, 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.  No other
>>> ChardevClass::open() prints anything on success.  Drop the message.
>>> 
>>> 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>
>>> ---
>>>  chardev/char-pty.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>> 
>>> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>>> index b034332edd..a48d3e5d20 100644
>>> --- a/chardev/char-pty.c
>>> +++ b/chardev/char-pty.c
>>> @@ -211,8 +211,6 @@ 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);
>>>  
>>>      s = PTY_CHARDEV(chr);
>>>      s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
>>
>> The reason for the message is that the char device is completely useless
>> until the user knows the /dev/pts/N path[1].  You can get it with "info
>> chardev" (aka query-chardev for QMP) but there's an interesting chicken
>> and egg problem if the pty is for your monitor...
>>
>> Paolo
>
> During review of v1, I wrote:
>
>     If we should decide the message is still useful enough to be worth
>     keeping, I could direct it to stdout instead of dropping it.
>
> No clear conclusion emerged, so I did nothing for v2.  If we conclude to
> keep the message now, I'll gladly do that.

No further comments, no clear conclusion, so let's stick to the status
quo.  I'll drop this patch and add one to print the message to stdout.

[...]

Re: [Qemu-devel] [PATCH v2 03/15] char-pty: Drop "char device redirected to" message

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 11/04/19 16:52, 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.  No other
>>> ChardevClass::open() prints anything on success.  Drop the message.
>>> 
>>> 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>
>>> ---
>>>  chardev/char-pty.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>> 
>>> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>>> index b034332edd..a48d3e5d20 100644
>>> --- a/chardev/char-pty.c
>>> +++ b/chardev/char-pty.c
>>> @@ -211,8 +211,6 @@ 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);
>>>  
>>>      s = PTY_CHARDEV(chr);
>>>      s->ioc = QIO_CHANNEL(qio_channel_file_new_fd(master_fd));
>>
>> The reason for the message is that the char device is completely useless
>> until the user knows the /dev/pts/N path[1].  You can get it with "info
>> chardev" (aka query-chardev for QMP) but there's an interesting chicken
>> and egg problem if the pty is for your monitor...
>>
>> Paolo
>
> During review of v1, I wrote:
>
>     If we should decide the message is still useful enough to be worth
>     keeping, I could direct it to stdout instead of dropping it.
>
> No clear conclusion emerged, so I did nothing for v2.  If we conclude to
> keep the message now, I'll gladly do that.

No further comments, no clear conclusion, so let's stick to the status
quo.  I'll drop this patch and add one to print the message to stdout.

[...]

Re: [Qemu-devel] [PATCH v2 03/15] char-pty: Drop "char device redirected to" message

[Peter Krempa]

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

On Thu, Apr 11, 2019 at 16:52:44 +0200, 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.  No other
> ChardevClass::open() prints anything on success.  Drop the message.
> 
> 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>
> ---
>  chardev/char-pty.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index b034332edd..a48d3e5d20 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -211,8 +211,6 @@ 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);

Don't be scared by the one occurence of this string in a 'strstr' call
in libvirt. We thankfully deleted the code that was using it to extract
the PTY path a long time ago. The only leftover part is to skip it when
we are attempting to extract an useful error from qemu which would be
reported to stderr prior to initiating QMP so getting rid of the message
actually has no effect.

https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=932534e85f34a479c7eac174e997bfd9c85bd22d

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 03/15] char-pty: Drop "char device redirected to" message

[Peter Krempa]

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

On Thu, Apr 11, 2019 at 16:52:44 +0200, 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.  No other
> ChardevClass::open() prints anything on success.  Drop the message.
> 
> 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>
> ---
>  chardev/char-pty.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index b034332edd..a48d3e5d20 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -211,8 +211,6 @@ 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);

Don't be scared by the one occurence of this string in a 'strstr' call
in libvirt. We thankfully deleted the code that was using it to extract
the PTY path a long time ago. The only leftover part is to skip it when
we are attempting to extract an useful error from qemu which would be
reported to stderr prior to initiating QMP so getting rid of the message
actually has no effect.

https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=932534e85f34a479c7eac174e997bfd9c85bd22d

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH v2 04/15] loader-fit: Wean off error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 04/15] loader-fit: Wean off error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 05/15] mips/boston: Report errors with error_report(), not error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 05/15] mips/boston: Report errors with error_report(), not error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 06/15] pci: Report fatal errors with error_report(), not error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 06/15] pci: Report fatal errors with error_report(), not error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 07/15] hpet: Report warnings with warn_report(), not error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 07/15] hpet: Report warnings with warn_report(), not error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 08/15] vfio: Report warnings with warn_report(), not error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 09/15] s390x/kvm: Report warnings with warn_report(), not error_printf()

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 10/15] vl: Make -machine $TYPE, help and -accel help print to stdout

[Markus Armbruster]

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 c696ad2a13..fdfa1c7453 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

[Qemu-devel] [PATCH v2 11/15] monitor error: Make printf()-like functions return a value

[Markus Armbruster]

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 0a8d9cc9ea..5db4e5db0d 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_set_progname(const char *argv0);
 
 void error_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 fcbe8a1f74..36eb3ea239 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

[Qemu-devel] [PATCH v2 12/15] qemu-print: New qemu_printf(), qemu_vprintf() etc.

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 13/15] blockdev: Make -drive format=help print to stdout

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 13/15] blockdev: Make -drive format=help print to stdout

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 14/15] char: Make -chardev help print to stdout

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 15/15] monitor: Simplify how -device/device_add print help

[Markus Armbruster]

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

[Qemu-devel] [PATCH v2 14.5/15] char-pty: Print "char device redirected" message to stdout

[Markus Armbruster]

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index b034332edd..4ff2f328fa 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -211,8 +211,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

Re: [Qemu-devel] [PATCH v2 14.5/15] char-pty: Print "char device redirected" message to stdout

[no-reply]

Patchew URL: https://patchew.org/QEMU/87mukov6vk.fsf@dusky.pond.sub.org/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  AR      libqemuutil.a
  LINK    elf2dmp
  AS      optionrom/multiboot.o
/tmp/qemu-test/src/chardev/char-pty.c:214:5: error: implicit declaration of function 'qemu_printf' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    qemu_printf("char device redirected to %s (label %s)\n",
    ^
/tmp/qemu-test/src/chardev/char-pty.c:214:5: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
2 errors generated.
  LINK    ivshmem-client
  LINK    ivshmem-server


The full log is available at
http://patchew.org/logs/87mukov6vk.fsf@dusky.pond.sub.org/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com