Re: [Qemu-devel] [PATCH 5/5] monitor: Catch printing to non-existent monitor

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> The monitor_vprintf() function now touches the 'mon' pointer
> before calling monitor_puts(), this causes block migration
> to segfault as its functions call monitor_printf() with a
> NULL 'mon'.

I figure this worked fine until commit 4a29a85d made monitor_vprintf()
dereference mon.

> To fix the problem this commit moves the 'mon' NULL check
> from monitor_puts() to monitor_vprintf().
>
> This can potentially hide bugs, but for some reason this has
> been the behavior for a long time.
>
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
>  monitor.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index b518cc4..ebd0282 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -177,9 +177,6 @@ static void monitor_puts(Monitor *mon, const char *str)
>  {
>      char c;
>  
> -    if (!mon)
> -        return;
> -
>      for(;;) {
>          c = *str++;
>          if (c == '\0')
> @@ -195,6 +192,9 @@ static void monitor_puts(Monitor *mon, const char *str)
>  
>  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  {
> +    if (!mon)
> +        return;
> +
>      if (mon->mc && !mon->mc->print_enabled) {
>          qemu_error_new(QERR_UNDEFINED_ERROR);
>      } else {

There are no other callers of monitor_puts(), so removing the check
there is okay.

Before the code motion, we throw QERR_UNDEFINED_ERROR on
monitor_vprintf(NULL, ...).  Afterwards, we don't.  Could you explain
why that's okay?