[Qemu-devel] Re: [STAGING]: Block migration segfaults

[Jan Kiszka]

Luiz Capitulino wrote:
>  Hi there,
> 
>  Got this while testing block migration in staging:
> 
> """
> Program terminated with signal 11, Segmentation fault.
> #0  0x0000000000410cf9 in monitor_vprintf (mon=0x0, fmt=0x5ae5e7 "Start full 
> migration for %s\n",
> ap=0x7fff1f830a40) at /home/lcapitulino/src/aliguori-queue/monitor.c:192
> 192         if (mon->mc && !mon->mc->print_enabled) {
> """
> 
>  The problem here is that init_blk_migration() calls monitor_printf() with
> a NULL 'mon' and the backtrace shows that this is true for the entire call
> chain.

What is the backtrace? And how did you start the migration?

> 
>  You probably didn't note it before because the lowest-level monitor
> print function would just return if the 'mon' parameter was NULL.

I was aware that mon might be NULL, but the existing code handled this
gracefully.

> 
>  A patch from me (4a29a in staging) changes a higher level monitor
> function to touch 'mon' before passing it down and here's the segfault.
> 
>  Now, the point is: I could give the old behavior back but I think we're
> hiding a bug there. Why would you call monitor_printf() with a NULL 'mon'?

If there is no monitor associated with the current context, it can be
NULL. This is mostly the case during early startup. One may also use
this to suppress output (though I don't recall any real case ATM).

> 
>  Anyways, the following patch adds the old behavior back just in case
> you want to see it working...

Yes, better restore the check. Still, your call stack would be
interesting. Maybe there is actual another bug behind it.

> 
> commit 3575196202d4e54c1fc63a631ca5bd1a7778e30d
> Author: Luiz Capitulino <address@hidden>
> Date:   Thu Dec 3 15:49:16 2009 -0200
> 
>     monitor: Fix block migration segfault

Let's call it by it true name: Catch printing to non-existent monitor.

>     
>     The monitor_vprintf() function now touches the 'mon' variable
>     before calling monitor_puts(), this causes block migration
>     to segfault as its functions call monitor_printf() with a
>     NULL 'mon'.
>     
>     This is probably hiding the real bug, but for some reason this
>     has been the behavior for a long time.
>     
>     We also change monitor_print_object() to use monitor_print(),
>     so that monitor_puts() is only called by monitor_vprintf().
>     
>     Signed-off-by: Luiz Capitulino <address@hidden>
> 
> diff --git a/monitor.c b/monitor.c
> index b035e0b..6d0b1dd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -171,9 +171,6 @@ static void monitor_puts(Monitor *mon, const char *str)
>  {
>      char c;
>  
> -    if (!mon)
> -        return;
> -
>      for(;;) {
>          c = *str++;
>          if (c == '\0')
> @@ -189,6 +186,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 {
> @@ -269,7 +269,7 @@ static void monitor_print_qobject(Monitor *mon, const 
> QObject *data)
>              break;
>      }
>  
> -    monitor_puts(mon, "\n");
> +    monitor_printf(mon, "\n");
>  }
>  
>  static void monitor_json_emitter(Monitor *mon, const QObject *data)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux