Re: [PATCH 05/17] monitor: remove 'skip_flush' field

["Dr. David Alan Gilbert" <dave@treblig.org>]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> The 'skip_flush' field is set on the dummy throwaway HMP monitor
> object created by QMP's  'human-monitor-command', as an indication
> not to try to write data to the chardev. Instead the QMP command
> impl will grab the data straight out of the in-memory buffer.
> 
> The flag is redundant, however, as the monitor code could instead
> simply check the 'fe_is_open' field on the CharFrontend, which
> will be false in the same scenarios that 'skip_flush' is true.

For monitors on sockets where the other end isn't connected, does that
change the behaviour?   Does that also have !fe_is_open and can that hit this
case?

Dave

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  monitor/hmp.c              |  2 +-
>  monitor/monitor-internal.h |  4 +---
>  monitor/monitor.c          | 11 +++++++----
>  monitor/qmp-cmds.c         |  2 +-
>  monitor/qmp.c              |  2 +-
>  5 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 614d9a3707..72f8303662 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1539,7 +1539,7 @@ void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp)
>          return;
>      }
>  
> -    monitor_data_init(&mon->parent, false, false, false);
> +    monitor_data_init(&mon->parent, false, false);
>  
>      mon->use_readline = use_readline;
>      if (mon->use_readline) {
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 25320928a7..84117805b7 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -102,7 +102,6 @@ struct Monitor {
>      CharFrontend chr;
>      int suspend_cnt;            /* Needs to be accessed atomically */
>      bool is_qmp;
> -    bool skip_flush;
>      bool use_io_thread;
>  
>      char *mon_cpu_path;
> @@ -183,8 +182,7 @@ extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>  extern QemuMutex monitor_lock;
>  extern MonitorList mon_list;
>  
> -void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
> -                       bool use_io_thread);
> +void monitor_data_init(Monitor *mon, bool is_qmp, bool use_io_thread);
>  void monitor_data_destroy(Monitor *mon);
>  int monitor_can_read(void *opaque);
>  void monitor_list_append(Monitor *mon);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 7936e2ab22..f7e3708d2f 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -167,7 +167,12 @@ void monitor_flush_locked(Monitor *mon)
>      size_t len;
>      const char *buf;
>  
> -    if (mon->skip_flush) {
> +    /*
> +     * When used by QMP human-monitor-command, no chardev
> +     * will be connected, as we want to just collect the
> +     * output in the buffer
> +     */
> +    if (!mon->chr.fe_is_open) {
>          return;
>      }
>  
> @@ -621,8 +626,7 @@ static void monitor_iothread_init(void)
>      mon_iothread = iothread_create("mon_iothread", &error_abort);
>  }
>  
> -void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
> -                       bool use_io_thread)
> +void monitor_data_init(Monitor *mon, bool is_qmp, bool use_io_thread)
>  {
>      if (use_io_thread && !mon_iothread) {
>          monitor_iothread_init();
> @@ -630,7 +634,6 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
>      qemu_mutex_init(&mon->mon_lock);
>      mon->is_qmp = is_qmp;
>      mon->outbuf = g_string_new(NULL);
> -    mon->skip_flush = skip_flush;
>      mon->use_io_thread = use_io_thread;
>  }
>  
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index d0ed241d6a..be2bd985b7 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -168,7 +168,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      char *output = NULL;
>      MonitorHMP *hmp = MONITOR_HMP(object_new(TYPE_MONITOR_HMP));
>  
> -    monitor_data_init(&hmp->parent, false, true, false);
> +    monitor_data_init(&hmp->parent, false, false);
>  
>      if (has_cpu_index) {
>          int ret = monitor_set_cpu(&hmp->parent, cpu_index);
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 36cb078f30..fe2aec9ce9 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -538,7 +538,7 @@ void monitor_new_qmp(Chardev *chr, bool pretty, Error **errp)
>      qemu_chr_fe_set_echo(&mon->parent.chr, true);
>  
>      /* Note: we run QMP monitor in I/O thread when @chr supports that */
> -    monitor_data_init(&mon->parent, true, false,
> +    monitor_data_init(&mon->parent, true,
>                        qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
>  
>      mon->pretty = pretty;
> -- 
> 2.53.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/