Re: [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue,  3 Jun 2014 18:39:10 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This lets the block layer emit QMP events from outside the I/O thread.

To be honest I'm starting to forget monitor code, but this looks correct
to me. I have only one comment below that doesn't prevent me from adding:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  monitor.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 342e83b..ebc66fb 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -191,8 +191,11 @@ struct Monitor {
>      int flags;
>      int suspend_cnt;
>      bool skip_flush;
> +
> +    QemuMutex out_lock;
>      QString *outbuf;
> -    guint watch;
> +    guint out_watch;
> +
>      ReadLineState *rs;
>      MonitorControl *mc;
>      CPUState *mon_cpu;
> @@ -265,17 +268,22 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>      }
>  }
>  
> +static void do_monitor_flush(Monitor *mon);
> +
>  static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>                                    void *opaque)
>  {
>      Monitor *mon = opaque;
>  
> -    mon->watch = 0;
> +    qemu_mutex_lock(&mon->out_lock);
> +    mon->out_watch = 0;
>      monitor_flush(mon);
> +    qemu_mutex_unlock(&mon->out_lock);
>      return FALSE;
>  }
>  
> -void monitor_flush(Monitor *mon)
> +/* Called with mon->out_lock held.  */
> +static void do_monitor_flush(Monitor *mon)
>  {
>      int rc;
>      size_t len;
> @@ -302,18 +310,26 @@ void monitor_flush(Monitor *mon)
>              QDECREF(mon->outbuf);
>              mon->outbuf = tmp;
>          }
> -        if (mon->watch == 0) {
> -            mon->watch = qemu_chr_fe_add_watch(mon->chr, G_IO_OUT,
> -                                               monitor_unblocked, mon);
> +        if (mon->out_watch == 0) {
> +            mon->out_watch = qemu_chr_fe_add_watch(mon->chr, G_IO_OUT,
> +                                                   monitor_unblocked, mon);
>          }
>      }
>  }
>  
> +void monitor_flush(Monitor *mon)
> +{
> +    qemu_mutex_lock(&mon->out_lock);
> +    do_monitor_flush(mon);
> +    qemu_mutex_unlock(&mon->out_lock);
> +}
> +
>  /* flush at every end of line */
>  static void monitor_puts(Monitor *mon, const char *str)
>  {
>      char c;
>  
> +    qemu_mutex_lock(&mon->out_lock);
>      for(;;) {
>          c = *str++;
>          if (c == '\0')
> @@ -323,9 +339,10 @@ static void monitor_puts(Monitor *mon, const char *str)
>          }
>          qstring_append_chr(mon->outbuf, c);
>          if (c == '\n') {
> -            monitor_flush(mon);
> +            do_monitor_flush(mon);
>          }
>      }
> +    qemu_mutex_unlock(&mon->out_lock);
>  }
>  
>  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
> @@ -690,6 +707,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline);
>  static void monitor_data_init(Monitor *mon)
>  {
>      memset(mon, 0, sizeof(Monitor));
> +    qemu_mutex_init(&mon->out_lock);
>      mon->outbuf = qstring_new();
>      /* Use *mon_cmds by default. */
>      mon->cmd_table = mon_cmds;
> @@ -698,6 +716,7 @@ static void monitor_data_init(Monitor *mon)
>  static void monitor_data_destroy(Monitor *mon)
>  {
>      QDECREF(mon->outbuf);
> +    qemu_mutex_destroy(&mon->out_lock);
>  }
>  
>  char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> @@ -725,11 +744,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      handle_user_command(&hmp, command_line);
>      cur_mon = old_mon;
>  
> +    qemu_mutex_lock(&hmp.out_lock);
>      if (qstring_get_length(hmp.outbuf) > 0) {
>          output = g_strdup(qstring_get_str(hmp.outbuf));
>      } else {
>          output = g_strdup("");
>      }
> +    qemu_mutex_unlock(&hmp.out_lock);

Are you sure we need to lock/unlock in this function? hmp is allocated
in the stack.

>  
>  out:
>      monitor_data_destroy(&hmp);