Re: [Qemu-devel] [PATCH 5/6] error: Remove redundant error_printf_unless_qmp

[Markus Armbruster <armbru@redhat.com>]

Cole Robinson <crobinso@redhat.com> writes:

> error_printf is just a wrapper around monitor_printf, which already
> drops the requested output if cur_mon is qmp.

Since commit 74ee59a:

    monitor: drop unused monitor debug code
    
    In the old QMP days, this code was used to find out QMP commands that
    might be calling monitor_printf() down its call chain.
    
    This is almost impossible to happen today, because the qapi converted
    commands don't even have a monitor object. Besides, it's been more than
    a year since I used this last time.
    
    Let's just drop it.
    
    Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
    Reviewed-by: Markus Armbruster <armbru@redhat.com>

I gave my R-by then, but I'm no longer sure it was a sensible move.
Attempting to print in QMP context is as much a sign of trouble as it
ever was.  I hate sweeping signs of trouble under the carpet.  If misuse
is really "almost impossible", then we should assert() it is, and fix
the bugs caught by it.

I can see error_printf() / error_printf_unless_qmp() used in a couple of
ways:

1. Print hints after qerror_report(), with error_printf_unless_qmp()

   qerror_report() is a transitional interface to help with converting
   existing HMP commands to QMP.  It should not be used elsewhere.

   We suppress the hints in QMP, because the QMP wire format doesn't
   provide for hints.

   We can't add hints to an error when using error_set(), because that
   API lacks support for hints.  Pity.

   Examples: see your patch below.

2. Print hints after error_report(), with error_printf()

   Use of error_report() in QMP context is a sign of trouble just like
   any other non-JSON output to the monitor.

   error_printf() rather than error_printf_unless_qmp(), because the
   latter explicitly signals intent "skip this in QMP", while the former
   signals "QMP should not happen".

   The difference in intent is what makes me wary of this patch.

   Example: vfio_pci_load_rom().

3. Ordinary output in code shared between command line and HMP, with
   error_printf()

   error_printf() was pressed into use as convenient "print to monitor
   in HMP context, else to tty" function.  Inappropriate, because it
   prints to stderr rather than stdout.

   Examples: many help texts under is_help_option().

4. Warnings, with error_printf()

   I figure these should use error_report() instead.

   Examples: block/ssh.c, hw/misc/vfio.c, ...

> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  hw/usb/bus.c                |  2 +-
>  hw/usb/hcd-ehci.c           |  4 ++--
>  include/qemu/error-report.h |  1 -
>  util/qemu-error.c           | 11 -----------
>  util/qemu-option.c          |  7 -------
>  5 files changed, 3 insertions(+), 22 deletions(-)
>
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index fe70429..f860631 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -348,7 +348,7 @@ int usb_register_companion(const char *masterbus, USBPort *ports[],
>          qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
>                        "an USB masterbus");
>          if (bus) {
> -            error_printf_unless_qmp(
> +            error_printf(
>                  "USB bus '%s' does not allow companion controllers\n",
>                  masterbus);
>          }
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 355bbd6..81ef01d 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -855,7 +855,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
>      if (firstport + portcount > NB_PORTS) {
>          qerror_report(QERR_INVALID_PARAMETER_VALUE, "firstport",
>                        "firstport on masterbus");
> -        error_printf_unless_qmp(
> +        error_printf(
>              "firstport value of %u makes companion take ports %u - %u, which "
>              "is outside of the valid range of 0 - %u\n", firstport, firstport,
>              firstport + portcount - 1, NB_PORTS - 1);
> @@ -866,7 +866,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
>          if (s->companion_ports[firstport + i]) {
>              qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
>                            "an USB masterbus");
> -            error_printf_unless_qmp(
> +            error_printf(
>                  "port %u on masterbus %s already has a companion assigned\n",
>                  firstport + i, bus->qbus.name);
>              return -1;
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 000eae3..a08ee95 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -36,7 +36,6 @@ 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_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  void error_set_progname(const char *argv0);
>  void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  const char *error_get_progname(void);
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index 80df49a..0ccd3e9 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -40,17 +40,6 @@ void error_printf(const char *fmt, ...)
>      va_end(ap);
>  }
>  
> -void error_printf_unless_qmp(const char *fmt, ...)
> -{
> -    va_list ap;
> -
> -    if (!monitor_cur_is_qmp()) {
> -        va_start(ap, fmt);
> -        error_vprintf(fmt, ap);
> -        va_end(ap);
> -    }
> -}
> -
>  static Location std_loc = {
>      .kind = LOC_NONE
>  };
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 9d898af..552fd06 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -201,10 +201,6 @@ void parse_option_size(const char *name, const char *value,
>              break;
>          default:
>              error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -            error_printf_unless_qmp("You may use k, M, G or T suffixes for "
> -                    "kilobytes, megabytes, gigabytes and terabytes.\n");
> -#endif
>              return;
>          }
>      } else {

We haven't been able to repair this breakage for a while, so giving up
and removing its debris isn't unreasonable.  However, I'd rather keep it
for now as a reminder of the deficiency in the error_set() API.

> @@ -811,9 +807,6 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>      if (id) {
>          if (!id_wellformed(id)) {
>              error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -            error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
> -#endif
>              return NULL;
>          }
>          opts = qemu_opts_find(list, id);

Likewise.