Re: [PATCH v4 36/49] monitor: tighten monitor_printf*()

On Wed, Aug 26, 2026 at 01:41:55PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Aug 26, 2026 at 12:54 PM Daniel P. Berrangé <[email protected]> wrote:
> >
> > On Tue, Aug 25, 2026 at 11:09:43PM +0400, Marc-André Lureau wrote:
> > > Rename monitor_printf->monitor_hmp_printf, monitor_vprintf->
> > > monitor_hmp_vprintf, and monitor_printc->monitor_hmp_printc, changing
> > > the first parameter from Monitor * to MonitorHMP * to enforce type
> > > safety. The implementation is also simplified: monitor_hmp_vprintf now
> > > directly calls g_strdup_vprintf + monitor_puts, removing the virtual
> > > dispatch via moncls->vprintf.
> > >
> > > The dev_print() callbacks are temporarily using the MONITOR_HMP(mon)
> > > cast, they are fixed in the following commits.
> > >
> > > Signed-off-by: Marc-André Lureau <[email protected]>
> > > ---
> > >  audio/audio-hmp-cmds.c                  |   6 +-
> > >  backends/cryptodev-hmp-cmds.c           |   9 +-
> > >  block/monitor/block-hmp-cmds.c          | 170 +++++++++---------
> > >  chardev/char-hmp-cmds.c                 |  12 +-
> > >  disas/disas-mon.c                       |  10 +-
> > >  docs/devel/style.rst                    |   2 +-
> > >  docs/devel/writing-monitor-commands.rst |   8 +-
> > >  dump/dump-hmp-cmds.c                    |   5 +-
> > >  hw/char/virtio-serial-bus.c             |  10 +-
> > >  hw/core/machine-hmp-cmds.c              | 213 +++++++++++-----------
> > >  hw/core/sysbus.c                        |   5 +-
> > >  hw/hexagon/hexagon_tlb.c                |  44 ++---
> > >  hw/i386/kvm/xen-stubs.c                 |   6 +-
> > >  hw/i386/kvm/xen_evtchn.c                |  21 +--
> > >  hw/i386/sgx-hmp-stub.c                  |   3 +-
> > >  hw/i386/sgx.c                           |  29 ++-
> > >  hw/misc/auxbus.c                        |   9 +-
> > >  hw/misc/mos6522-stub.c                  |   3 +-
> > >  hw/net/rocker/rocker-hmp-cmds.c         | 146 ++++++++--------
> > >  hw/pci/pci-hmp-cmds.c                   | 114 ++++++------
> > >  hw/pci/pci-stub.c                       |   3 +-
> > >  hw/s390x/s390-skeys.c                   |   9 +-
> > >  hw/s390x/s390-stattrib.c                |  20 +--
> > >  hw/uefi/ovmf-log.c                      |   5 +-
> > >  hw/usb/bus.c                            |  11 +-
> > >  hw/usb/host-libusb.c                    |  21 ++-
> > >  hw/virtio/virtio-hmp-cmds.c             | 297 ++++++++++++++++---------------
> > >  hw/xen/xen-bus.c                        |   5 +-
> > >  include/disas/disas.h                   |   4 +-
> > >  include/monitor/hmp.h                   |  12 +-
> > >  migration/dirtyrate.c                   |  46 +++--
> > >  migration/migration-hmp-cmds.c          | 301 ++++++++++++++++----------------
> > >  monitor/hmp-cmds.c                      | 141 +++++++--------
> > >  monitor/hmp.c                           | 143 +++++++--------
> > >  monitor/monitor-internal.h              |   6 -
> > >  monitor/monitor.c                       |  33 ++--
> > >  net/net-hmp-cmds.c                      |  31 ++--
> > >  net/slirp.c                             |  31 ++--
> > >  qom/qom-hmp-cmds.c                      |  27 ++-
> > >  replay/replay-debugging.c               |   5 +-
> > >  stats/stats-hmp-cmds.c                  |  57 +++---
> > >  stubs/hmp-cmd-info_sev.c                |   3 +-
> > >  stubs/monitor-core.c                    |   2 +-
> > >  system/dirtylimit-hmp-cmds.c            |  10 +-
> > >  system/qdev-monitor.c                   |  19 +-
> > >  system/runstate-hmp-cmds.c              |  16 +-
> > >  system/tpm-hmp-cmds.c                   |  29 ++-
> > >  target/i386/cpu-apic.c                  |   3 +-
> > >  target/i386/monitor.c                   | 152 ++++++++--------
> > >  target/i386/sev.c                       |  35 ++--
> > >  target/m68k/monitor.c                   |   3 +-
> > >  target/ppc/monitor.c                    |   3 +-
> > >  target/riscv/monitor.c                  |  55 +++---
> > >  target/sh4/monitor.c                    |  29 ++-
> > >  target/sparc/monitor.c                  |   3 +-
> > >  target/xtensa/monitor.c                 |   3 +-
> > >  tests/unit/test-util-sockets.c          |   2 +-
> > >  tools/qemu-vnc/clipboard.c              |   4 +-
> > >  tools/qemu-vnc/stubs.c                  |   2 +-
> > >  trace/trace-hmp-cmds.c                  |  12 +-
> > >  ui/ui-hmp-cmds.c                        | 115 ++++++------
> > >  util/error-report.c                     |   2 +-
> > >  util/qemu-print.c                       |  11 +-
> > >  63 files changed, 1219 insertions(+), 1327 deletions(-)
> >
> >
> >
> >
> > > diff --git a/util/qemu-print.c b/util/qemu-print.c
> > > index 5d4143d425a1..5938f2b6c338 100644
> > > --- a/util/qemu-print.c
> > > +++ b/util/qemu-print.c
> > > @@ -13,6 +13,7 @@
> > >  #include "qemu/osdep.h"
> > >  #include "monitor/monitor.h"
> > >  #include "monitor/hmp.h"
> > > +#include "qom/object.h"
> > >  #include "qemu/qemu-print.h"
> > >
> > >  /*
> > > @@ -23,8 +24,13 @@
> > >  int qemu_vprintf(const char *fmt, va_list ap)
> > >  {
> > >      Monitor *cur_mon = monitor_cur();
> > > +
> > > +    /* for all monitors: QMP & HMP */
> > >      if (cur_mon) {
> > > -        return monitor_vprintf(cur_mon, fmt, ap);
> > > +        /* don't use monitor_cur_hmp(), to avoid a second lookup */
> > > +        MonitorHMP *hmp = (MonitorHMP *)
> > > +            object_dynamic_cast(OBJECT(cur_mon), TYPE_MONITOR_HMP);
> > > +        return monitor_hmp_vprintf(hmp, fmt, ap);
> >
> > This isn't the same semantics AFAICT.
> >
> > Original code, if monitor_cur() == QMP, we call monitor_vprintf()
> > which will return -1.
> >
> > New code, if monitor_cur() == QMP, we will get a NULL back from
> > object_dynamic_cast which we then pass into monitor_hmp_vprintf
> > which will then crash on monitor_puts() IIUC.
> 
> monitor_hmp_vprintf() has an early return, if given NULL monitor, it returns -1.

Hmm, I feel like we should be dealing with NULL in this method, as it
is surprising to be calling a monitor_hmp_XXX method in scenario where
QMP is a (theoretical) possibility.


With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|