[1/2] qmp: use readline mode for stdio

Instead of using an echo for '-qmp stdio' we use a readline mode. The readline mode
adds a history for users which is useful.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 monitor.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 vl.c      |    3 ++
 2 files changed, 81 insertions(+), 5 deletions(-)

Il 30/05/2012 12:01, Pavel Hrdina ha scritto:
> Instead of using an echo for '-qmp stdio' we use a readline mode. The readline mode
> adds a history for users which is useful.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  monitor.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  vl.c      |    3 ++
>  2 files changed, 81 insertions(+), 5 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 12a6fe2..9863fdd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -206,6 +206,8 @@ static const mon_cmd_t qmp_cmds[];
>  Monitor *cur_mon;
>  Monitor *default_mon;
>  
> +static void monitor_stdio_control_command_cb(Monitor *mon, const char *cmdline,
> +                               void *opaque);
>  static void monitor_command_cb(Monitor *mon, const char *cmdline,
>                                 void *opaque);
>  
> @@ -214,6 +216,11 @@ static inline int qmp_cmd_mode(const Monitor *mon)
>      return (mon->mc ? mon->mc->command_mode : 0);
>  }
>  
> +static inline int monitor_readline_mode(const Monitor *mon)
> +{
> +    return (mon->flags & MONITOR_USE_READLINE);
> +}
> +
>  /* Return true if in control mode, false otherwise */
>  static inline int monitor_ctrl_mode(const Monitor *mon)
>  {
> @@ -226,6 +233,16 @@ int monitor_cur_is_qmp(void)
>      return cur_mon && monitor_ctrl_mode(cur_mon);
>  }
>  
> +static void monitor_control_read_command(Monitor *mon, int show_prompt)
> +{
> +    if (!mon->rs)
> +        return;
> +
> +    readline_start(mon->rs, "", 0, monitor_stdio_control_command_cb, NULL);
> +    if (show_prompt)
> +        readline_show_prompt(mon->rs);
> +}
> +
>  void monitor_read_command(Monitor *mon, int show_prompt)
>  {
>      if (!mon->rs)
> @@ -287,7 +304,7 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  
>      mon_print_count_inc(mon);
>  
> -    if (monitor_ctrl_mode(mon)) {
> +    if (monitor_ctrl_mode(mon) && !monitor_readline_mode(mon)) {
>          return;
>      }
>  
> @@ -4431,10 +4448,23 @@ out:
>  static void monitor_control_read(void *opaque, const uint8_t *buf, int size)
>  {
>      Monitor *old_mon = cur_mon;
> +    int i;
>  
>      cur_mon = opaque;
>  
> -    json_message_parser_feed(&cur_mon->mc->parser, (const char *) buf, size);
> +    if (monitor_readline_mode(cur_mon)) {
> +        if (cur_mon->rs) {
> +            for (i = 0; i < size; i++)
> +                readline_handle_byte(cur_mon->rs, buf[i]);
> +        } else {
> +            if (size == 0 || buf[size - 1] != 0)
> +                monitor_printf(cur_mon, "corrupted command\n");
> +            else
> +                json_message_parser_feed(&cur_mon->mc->parser, (const char *) buf, size);
> +        }
> +    } else {
> +        json_message_parser_feed(&cur_mon->mc->parser, (const char *) buf, size);
> +    }
>  
>      cur_mon = old_mon;
>  }
> @@ -4459,6 +4489,13 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
>      cur_mon = old_mon;
>  }
>  
> +static void monitor_stdio_control_command_cb(Monitor *mon, const char *cmdline, void *opaque)
> +{
> +    monitor_suspend(mon);
> +    json_message_parser_feed(&mon->mc->parser, cmdline, strlen(cmdline));
> +    monitor_resume(mon);
> +}
> +
>  static void monitor_command_cb(Monitor *mon, const char *cmdline, void *opaque)
>  {
>      monitor_suspend(mon);
> @@ -4499,7 +4536,41 @@ static void monitor_control_event(void *opaque, int event)
>      Monitor *mon = opaque;
>  
>      switch (event) {
> +    case CHR_EVENT_MUX_IN:
> +        if (!monitor_readline_mode(mon))
> +            break;
> +        mon->mux_out = 0;
> +        if (mon->reset_seen) {
> +            readline_restart(mon->rs);
> +            monitor_resume(mon);
> +            monitor_flush(mon);
> +        } else {
> +            mon->suspend_cnt = 0;
> +        }
> +        break;
> +
> +    case CHR_EVENT_MUX_OUT:
> +        if (!monitor_readline_mode(mon))
> +            break;
> +        if (mon->reset_seen) {
> +            if (mon->suspend_cnt == 0) {
> +                monitor_printf(mon, "\n");
> +            }
> +            monitor_flush(mon);
> +            monitor_suspend(mon);
> +        } else {
> +            mon->suspend_cnt++;
> +        }
> +        mon->mux_out = 1;
> +        break;
> +
>      case CHR_EVENT_OPENED:
> +        if (monitor_readline_mode(mon)) {
> +            mon->reset_seen = 1;
> +            if (!mon->mux_out) {
> +                readline_show_prompt(mon->rs);
> +            }
> +        }
>          mon->mc->command_mode = 0;
>          json_message_parser_init(&mon->mc->parser, handle_qmp_command);
>          data = get_qmp_greeting();
> @@ -4594,9 +4665,12 @@ void monitor_init(CharDriverState *chr, int flags)
>  
>      mon->chr = chr;
>      mon->flags = flags;
> -    if (flags & MONITOR_USE_READLINE) {
> +    if (monitor_readline_mode(mon)) {
>          mon->rs = readline_init(mon, monitor_find_completion);
> -        monitor_read_command(mon, 0);
> +        if (monitor_ctrl_mode(mon))
> +            monitor_control_read_command(mon, 0);
> +        else
> +            monitor_read_command(mon, 0);
>      }
>  
>      if (monitor_ctrl_mode(mon)) {

I think the patch up to here is fine.

> @@ -4604,7 +4678,6 @@ void monitor_init(CharDriverState *chr, int flags)
>          /* Control mode requires special handlers */
>          qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
>                                monitor_control_event, mon);
> -        qemu_chr_fe_set_echo(chr, true);
>      } else {
>          qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
>                                monitor_event, mon);

Here, please make the call conditional on (flags & MONITOR_USE_READLINE)
== 0 instead.

Everything up to here should be a separate patch, the first in a series.

> diff --git a/vl.c b/vl.c
> index 23ab3a3..5e9c130 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1894,6 +1894,9 @@ static int mon_init_func(QemuOpts *opts, void *opaque)
>          exit(1);
>      }
>  
> +    if ((flags & MONITOR_USE_CONTROL) && strcmp(chr->filename, "stdio") == 0)
> +        flags |= MONITOR_USE_READLINE;
> +
>      monitor_init(chr, flags);
>      return 0;
>  }

But this is a hack.  Instead, split this in three patches:

1) add new boolean QemuOpts "readline" and "control" that replace
"mode"; default readline to the opposite of control.

2) add a new method to CharDriverState "isatty".  The function should
call the isatty() function for stdio, and should always return true for
mux and vc. For everything else do not implement it (the default
implementation should be to always return false).

3) change the readline default to be true also if control=on and isatty
returns true.

Paolo

On 05/31/2012 12:34 PM, Paolo Bonzini wrote:
> Il 30/05/2012 12:01, Pavel Hrdina ha scritto:
>> Instead of using an echo for '-qmp stdio' we use a readline mode. The readline mode
>> adds a history for users which is useful.
>>
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> ---
>>   monitor.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   vl.c      |    3 ++
>>   2 files changed, 81 insertions(+), 5 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 12a6fe2..9863fdd 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -206,6 +206,8 @@ static const mon_cmd_t qmp_cmds[];
>>   Monitor *cur_mon;
>>   Monitor *default_mon;
>>
>> +static void monitor_stdio_control_command_cb(Monitor *mon, const char *cmdline,
>> +                               void *opaque);
>>   static void monitor_command_cb(Monitor *mon, const char *cmdline,
>>                                  void *opaque);
>>
>> @@ -214,6 +216,11 @@ static inline int qmp_cmd_mode(const Monitor *mon)
>>       return (mon->mc ? mon->mc->command_mode : 0);
>>   }
>>
>> +static inline int monitor_readline_mode(const Monitor *mon)
>> +{
>> +    return (mon->flags&  MONITOR_USE_READLINE);
>> +}
>> +
>>   /* Return true if in control mode, false otherwise */
>>   static inline int monitor_ctrl_mode(const Monitor *mon)
>>   {
>> @@ -226,6 +233,16 @@ int monitor_cur_is_qmp(void)
>>       return cur_mon&&  monitor_ctrl_mode(cur_mon);
>>   }
>>
>> +static void monitor_control_read_command(Monitor *mon, int show_prompt)
>> +{
>> +    if (!mon->rs)
>> +        return;
>> +
>> +    readline_start(mon->rs, "", 0, monitor_stdio_control_command_cb, NULL);
>> +    if (show_prompt)
>> +        readline_show_prompt(mon->rs);
>> +}
>> +
>>   void monitor_read_command(Monitor *mon, int show_prompt)
>>   {
>>       if (!mon->rs)
>> @@ -287,7 +304,7 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>>
>>       mon_print_count_inc(mon);
>>
>> -    if (monitor_ctrl_mode(mon)) {
>> +    if (monitor_ctrl_mode(mon)&&  !monitor_readline_mode(mon)) {
>>           return;
>>       }
>>
>> @@ -4431,10 +4448,23 @@ out:
>>   static void monitor_control_read(void *opaque, const uint8_t *buf, int size)
>>   {
>>       Monitor *old_mon = cur_mon;
>> +    int i;
>>
>>       cur_mon = opaque;
>>
>> -    json_message_parser_feed(&cur_mon->mc->parser, (const char *) buf, size);
>> +    if (monitor_readline_mode(cur_mon)) {
>> +        if (cur_mon->rs) {
>> +            for (i = 0; i<  size; i++)
>> +                readline_handle_byte(cur_mon->rs, buf[i]);
>> +        } else {
>> +            if (size == 0 || buf[size - 1] != 0)
>> +                monitor_printf(cur_mon, "corrupted command\n");
>> +            else
>> +                json_message_parser_feed(&cur_mon->mc->parser, (const char *) buf, size);
>> +        }
>> +    } else {
>> +        json_message_parser_feed(&cur_mon->mc->parser, (const char *) buf, size);
>> +    }
>>
>>       cur_mon = old_mon;
>>   }
>> @@ -4459,6 +4489,13 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
>>       cur_mon = old_mon;
>>   }
>>
>> +static void monitor_stdio_control_command_cb(Monitor *mon, const char *cmdline, void *opaque)
>> +{
>> +    monitor_suspend(mon);
>> +    json_message_parser_feed(&mon->mc->parser, cmdline, strlen(cmdline));
>> +    monitor_resume(mon);
>> +}
>> +
>>   static void monitor_command_cb(Monitor *mon, const char *cmdline, void *opaque)
>>   {
>>       monitor_suspend(mon);
>> @@ -4499,7 +4536,41 @@ static void monitor_control_event(void *opaque, int event)
>>       Monitor *mon = opaque;
>>
>>       switch (event) {
>> +    case CHR_EVENT_MUX_IN:
>> +        if (!monitor_readline_mode(mon))
>> +            break;
>> +        mon->mux_out = 0;
>> +        if (mon->reset_seen) {
>> +            readline_restart(mon->rs);
>> +            monitor_resume(mon);
>> +            monitor_flush(mon);
>> +        } else {
>> +            mon->suspend_cnt = 0;
>> +        }
>> +        break;
>> +
>> +    case CHR_EVENT_MUX_OUT:
>> +        if (!monitor_readline_mode(mon))
>> +            break;
>> +        if (mon->reset_seen) {
>> +            if (mon->suspend_cnt == 0) {
>> +                monitor_printf(mon, "\n");
>> +            }
>> +            monitor_flush(mon);
>> +            monitor_suspend(mon);
>> +        } else {
>> +            mon->suspend_cnt++;
>> +        }
>> +        mon->mux_out = 1;
>> +        break;
>> +
>>       case CHR_EVENT_OPENED:
>> +        if (monitor_readline_mode(mon)) {
>> +            mon->reset_seen = 1;
>> +            if (!mon->mux_out) {
>> +                readline_show_prompt(mon->rs);
>> +            }
>> +        }
>>           mon->mc->command_mode = 0;
>>           json_message_parser_init(&mon->mc->parser, handle_qmp_command);
>>           data = get_qmp_greeting();
>> @@ -4594,9 +4665,12 @@ void monitor_init(CharDriverState *chr, int flags)
>>
>>       mon->chr = chr;
>>       mon->flags = flags;
>> -    if (flags&  MONITOR_USE_READLINE) {
>> +    if (monitor_readline_mode(mon)) {
>>           mon->rs = readline_init(mon, monitor_find_completion);
>> -        monitor_read_command(mon, 0);
>> +        if (monitor_ctrl_mode(mon))
>> +            monitor_control_read_command(mon, 0);
>> +        else
>> +            monitor_read_command(mon, 0);
>>       }
>>
>>       if (monitor_ctrl_mode(mon)) {
> I think the patch up to here is fine.
>
>> @@ -4604,7 +4678,6 @@ void monitor_init(CharDriverState *chr, int flags)
>>           /* Control mode requires special handlers */
>>           qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
>>                                 monitor_control_event, mon);
>> -        qemu_chr_fe_set_echo(chr, true);
>>       } else {
>>           qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
>>                                 monitor_event, mon);
> Here, please make the call conditional on (flags&  MONITOR_USE_READLINE)
> == 0 instead.
>
> Everything up to here should be a separate patch, the first in a series.
>
>> diff --git a/vl.c b/vl.c
>> index 23ab3a3..5e9c130 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1894,6 +1894,9 @@ static int mon_init_func(QemuOpts *opts, void *opaque)
>>           exit(1);
>>       }
>>
>> +    if ((flags&  MONITOR_USE_CONTROL)&&  strcmp(chr->filename, "stdio") == 0)
>> +        flags |= MONITOR_USE_READLINE;
>> +
>>       monitor_init(chr, flags);
>>       return 0;
>>   }
> But this is a hack.  Instead, split this in three patches:
>
> 1) add new boolean QemuOpts "readline" and "control" that replace
> "mode"; default readline to the opposite of control.
>
> 2) add a new method to CharDriverState "isatty".  The function should
> call the isatty() function for stdio, and should always return true for
> mux and vc. For everything else do not implement it (the default
> implementation should be to always return false).
>
> 3) change the readline default to be true also if control=on and isatty
> returns true.
>
> Paolo
Thanks Paolo, I'll rewrite it, but first I'll wait for result of 
discussion between Luiz and Kevin and maybe others, if someone else 
join, whether it should be implemented for qmp or not.