[Qemu-devel] [PATCH 0/9] monitor: enable OOB by default

[Qemu-devel] [PATCH 0/9] monitor: enable OOB by default

[Peter Xu]

Based-on: <20180703085358.13941-1-armbru@redhat.com>

This work is based on Markus's latest out-of-band fixes:
  "[PATCH v2 00/32] ] qmp: Fixes and cleanups around OOB commands"

Major stuff: some cleanups that were previously suggested by Markus or
Eric.  Meanwhile fix up the flow control issue.  Since my proposal
here is to drop COMMAND_DROPPED event, then we don't need to introduce
per-monitor event emission API any more.  Though Markus told me that
he might use that code somewhere else, so I'll post that per-monitor
event code out separately as RFC later.

Patch 1-3: cleanups.

Patch 4-7: solve the flow control issue reported by Markus that
response queue might overflow even if we have limitation on the
request queue.  Firstly we drop the COMMAND_DROP event since that
won't work for response queue (since queuing an event will need to
append to the response queue itself), then we use monitor suspend and
resume to control the flow.  Last, we apply that to response queue
too.

Patch 8-9: the original patches to enable out-of-band by default.

Tests: I didn't write flow control tests for qtest, however I tested
the program with this tiny tool "slowread":

        #include <unistd.h>
        #include <assert.h>

        int main(void)
        {
                char c;
                ssize_t ret;

                while (1) {
                        ret = read(STDIN_FILENO, &c, 1);
                        if (ret != 1)
                                break;
                        write(STDOUT_FILENO, &c, 1);
                        usleep(1000);
                }

                return 0;
        }

Basically it limits the read speed to 1000Bps.  Then I prepare a
command list "cmd_list":

        {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
        {"execute": "query-status"}
        {"execute": "query-status"}
        .....
        {"execute": "query-status"}
        {"execute": "query-status"}

Then I run this to make sure it works well:

  $ cat cmd_list | qemu-system-x86_64 -qmp stdio -nographic -nodefaults | slowread

Please review.  Thanks,

Peter Xu (9):
  monitor: simplify monitor_qmp_setup_handlers_bh
  qapi: allow build_params to return "void"
  qapi: remove error checks for event emission
  monitor: move need_resume flag into monitor struct
  monitor: suspend monitor instead of send CMD_DROP
  qapi: remove COMMAND_DROPPED event
  monitor: restrict response queue length too
  monitor: remove "x-oob", turn oob on by default
  Revert "tests: Add parameter to qtest_init_without_qmp_handshake"

 docs/devel/qapi-code-gen.txt |   6 +-
 qapi/misc.json               |  40 ---------
 include/monitor/monitor.h    |   1 -
 include/qapi/qmp-event.h     |   3 +-
 tests/libqtest.h             |   4 +-
 block/block-backend.c        |   8 +-
 block/qcow2.c                |   2 +-
 block/quorum.c               |   4 +-
 block/write-threshold.c      |   3 +-
 blockjob.c                   |  13 ++-
 cpus.c                       |   8 +-
 dump.c                       |   3 +-
 hw/acpi/core.c               |   2 +-
 hw/acpi/cpu.c                |   2 +-
 hw/acpi/memory_hotplug.c     |   5 +-
 hw/char/virtio-console.c     |   3 +-
 hw/core/qdev.c               |   3 +-
 hw/net/virtio-net.c          |   2 +-
 hw/ppc/spapr_rtc.c           |   2 +-
 hw/timer/mc146818rtc.c       |   2 +-
 hw/virtio/virtio-balloon.c   |   3 +-
 hw/watchdog/watchdog.c       |  15 ++--
 job.c                        |   2 +-
 migration/migration.c        |   4 +-
 migration/ram.c              |   2 +-
 monitor.c                    | 159 +++++++++++++++++++----------------
 scsi/pr-manager-helper.c     |   3 +-
 tests/libqtest.c             |  10 +--
 tests/qmp-test.c             |   6 +-
 tests/test-qmp-event.c       |  11 ++-
 ui/spice-core.c              |  10 +--
 ui/vnc.c                     |   7 +-
 vl.c                         |  21 ++---
 scripts/qapi/common.py       |   4 +-
 scripts/qapi/events.py       |  23 ++---
 35 files changed, 164 insertions(+), 232 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/9] monitor: simplify monitor_qmp_setup_handlers_bh

[Peter Xu]

When we reach monitor_qmp_setup_handlers_bh() we must be using the
IOThread then, so no need to check against it any more.  Instead, we
assert.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index 14af7b7ea6..0e3ac52d87 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4583,15 +4583,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     Monitor *mon = opaque;
     GMainContext *context;
 
-    if (mon->use_io_thread) {
-        /* Use @mon_iothread context */
-        context = monitor_get_io_context();
-        assert(context);
-    } else {
-        /* Use default main loop context */
-        context = NULL;
-    }
-
+    assert(mon->use_io_thread);
+    /* Use @mon_iothread context */
+    context = monitor_get_io_context();
+    assert(context);
     qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
                              monitor_qmp_event, NULL, mon, context, true);
     monitor_list_append(mon);
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/9] monitor: simplify monitor_qmp_setup_handlers_bh

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> When we reach monitor_qmp_setup_handlers_bh() we must be using the
> IOThread then, so no need to check against it any more.  Instead, we
> assert.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 14af7b7ea6..0e3ac52d87 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4583,15 +4583,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
>      Monitor *mon = opaque;
>      GMainContext *context;
>  
> -    if (mon->use_io_thread) {
> -        /* Use @mon_iothread context */
> -        context = monitor_get_io_context();
> -        assert(context);
> -    } else {
> -        /* Use default main loop context */
> -        context = NULL;
> -    }
> -
> +    assert(mon->use_io_thread);
> +    /* Use @mon_iothread context */
> +    context = monitor_get_io_context();
> +    assert(context);
>      qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
>                               monitor_qmp_event, NULL, mon, context, true);
>      monitor_list_append(mon);

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH 2/9] qapi: allow build_params to return "void"

[Peter Xu]

When there is no parameter at all for a function prototype, return
"void" for the parameter list.  This will happen after the next patch
where we removed the Error* for some of the emit functions.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 scripts/qapi/common.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 8b6708dbf1..105c82742f 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1963,7 +1963,7 @@ extern const QEnumLookup %(c_name)s_lookup;
 def build_params(arg_type, boxed, extra):
     if not arg_type:
         assert not boxed
-        return extra
+        return extra if extra else "void"
     ret = ''
     sep = ''
     if boxed:
@@ -1980,7 +1980,7 @@ def build_params(arg_type, boxed, extra):
                               c_name(memb.name))
     if extra:
         ret += sep + extra
-    return ret
+    return ret if ret else "void"
 
 
 #
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/9] qapi: allow build_params to return "void"

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> When there is no parameter at all for a function prototype, return
> "void" for the parameter list.  This will happen after the next patch
> where we removed the Error* for some of the emit functions.

Error **, actually.  Let's say

  qapi: Fix build_params() for empty parameter list

  build_params() returns '' instead of 'void' when there are no
  parameters.  Can't happen now, but the next commit will change that.

>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  scripts/qapi/common.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 8b6708dbf1..105c82742f 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1963,7 +1963,7 @@ extern const QEnumLookup %(c_name)s_lookup;
>  def build_params(arg_type, boxed, extra):
>      if not arg_type:
>          assert not boxed
> -        return extra
> +        return extra if extra else "void"
>      ret = ''
>      sep = ''
>      if boxed:
> @@ -1980,7 +1980,7 @@ def build_params(arg_type, boxed, extra):
>                                c_name(memb.name))
>      if extra:
>          ret += sep + extra
> -    return ret
> +    return ret if ret else "void"
>  
>  
>  #

Please use ' instead of " for local consistency.

Duplicating "if ret else 'void'" minimizes churn, but it's slightly
ugly.  I append my attempt to avoid it.  What do you think?


diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 8b6708dbf1..01384c5360 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1961,15 +1961,13 @@ extern const QEnumLookup %(c_name)s_lookup;
 
 
 def build_params(arg_type, boxed, extra):
-    if not arg_type:
-        assert not boxed
-        return extra
     ret = ''
     sep = ''
     if boxed:
+        assert arg_type
         ret += '%s arg' % arg_type.c_param_type()
         sep = ', '
-    else:
+    elif arg_type:
         assert not arg_type.variants
         for memb in arg_type.members:
             ret += sep
@@ -1980,7 +1978,7 @@ def build_params(arg_type, boxed, extra):
                               c_name(memb.name))
     if extra:
         ret += sep + extra
-    return ret
+    return ret if ret else 'void'
 
 
 #

Re: [Qemu-devel] [PATCH 2/9] qapi: allow build_params to return "void"

[Peter Xu]

On Thu, Jul 05, 2018 at 08:02:21AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > When there is no parameter at all for a function prototype, return
> > "void" for the parameter list.  This will happen after the next patch
> > where we removed the Error* for some of the emit functions.
> 
> Error **, actually.  Let's say
> 
>   qapi: Fix build_params() for empty parameter list
> 
>   build_params() returns '' instead of 'void' when there are no
>   parameters.  Can't happen now, but the next commit will change that.

Sure.

> 
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  scripts/qapi/common.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 8b6708dbf1..105c82742f 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -1963,7 +1963,7 @@ extern const QEnumLookup %(c_name)s_lookup;
> >  def build_params(arg_type, boxed, extra):
> >      if not arg_type:
> >          assert not boxed
> > -        return extra
> > +        return extra if extra else "void"
> >      ret = ''
> >      sep = ''
> >      if boxed:
> > @@ -1980,7 +1980,7 @@ def build_params(arg_type, boxed, extra):
> >                                c_name(memb.name))
> >      if extra:
> >          ret += sep + extra
> > -    return ret
> > +    return ret if ret else "void"
> >  
> >  
> >  #
> 
> Please use ' instead of " for local consistency.
> 
> Duplicating "if ret else 'void'" minimizes churn, but it's slightly
> ugly.  I append my attempt to avoid it.  What do you think?
> 
> 
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 8b6708dbf1..01384c5360 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1961,15 +1961,13 @@ extern const QEnumLookup %(c_name)s_lookup;
>  
>  
>  def build_params(arg_type, boxed, extra):
> -    if not arg_type:
> -        assert not boxed
> -        return extra
>      ret = ''
>      sep = ''
>      if boxed:
> +        assert arg_type
>          ret += '%s arg' % arg_type.c_param_type()
>          sep = ', '
> -    else:
> +    elif arg_type:
>          assert not arg_type.variants
>          for memb in arg_type.members:
>              ret += sep
> @@ -1980,7 +1978,7 @@ def build_params(arg_type, boxed, extra):
>                                c_name(memb.name))
>      if extra:
>          ret += sep + extra
> -    return ret
> +    return ret if ret else 'void'

Yeah, this looks good to me.

Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH 3/9] qapi: remove error checks for event emission

[Peter Xu]

In the whole QAPI event emission code we're passing in an Error* object
along the whole stack.  That's never useful since it never fails after
all.  Remove that.

Then, remove that parameter from all the callers to send an event.

Suggested-by: Eric Blake <eblake@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/devel/qapi-code-gen.txt |  6 ++----
 include/qapi/qmp-event.h     |  3 +--
 block/block-backend.c        |  8 +++-----
 block/qcow2.c                |  2 +-
 block/quorum.c               |  4 ++--
 block/write-threshold.c      |  3 +--
 blockjob.c                   | 13 +++++--------
 cpus.c                       |  8 ++++----
 dump.c                       |  3 +--
 hw/acpi/core.c               |  2 +-
 hw/acpi/cpu.c                |  2 +-
 hw/acpi/memory_hotplug.c     |  5 ++---
 hw/char/virtio-console.c     |  3 +--
 hw/core/qdev.c               |  3 +--
 hw/net/virtio-net.c          |  2 +-
 hw/ppc/spapr_rtc.c           |  2 +-
 hw/timer/mc146818rtc.c       |  2 +-
 hw/virtio/virtio-balloon.c   |  3 +--
 hw/watchdog/watchdog.c       | 15 +++++++--------
 job.c                        |  2 +-
 migration/migration.c        |  4 ++--
 migration/ram.c              |  2 +-
 monitor.c                    |  5 ++---
 scsi/pr-manager-helper.c     |  3 +--
 tests/test-qmp-event.c       | 11 +++++------
 ui/spice-core.c              | 10 ++++------
 ui/vnc.c                     |  7 +++----
 vl.c                         | 16 +++++++---------
 scripts/qapi/events.py       | 23 ++++++-----------------
 29 files changed, 69 insertions(+), 103 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 8decd6f17d..fb43c64ab2 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1327,10 +1327,9 @@ Example:
     $ cat qapi-generated/example-qapi-events.c
 [Uninteresting stuff omitted...]
 
-    void qapi_event_send_my_event(Error **errp)
+    void qapi_event_send_my_event(void)
     {
         QDict *qmp;
-        Error *err = NULL;
         QMPEventFuncEmit emit;
 
         emit = qmp_event_get_func_emit();
@@ -1340,9 +1339,8 @@ Example:
 
         qmp = qmp_event_build_dict("MY_EVENT");
 
-        emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &err);
+        emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp);
 
-        error_propagate(errp, err);
         qobject_unref(qmp);
     }
 
diff --git a/include/qapi/qmp-event.h b/include/qapi/qmp-event.h
index 0c87ad833e..23e588ccf8 100644
--- a/include/qapi/qmp-event.h
+++ b/include/qapi/qmp-event.h
@@ -14,8 +14,7 @@
 #ifndef QMP_EVENT_H
 #define QMP_EVENT_H
 
-
-typedef void (*QMPEventFuncEmit)(unsigned event, QDict *dict, Error **errp);
+typedef void (*QMPEventFuncEmit)(unsigned event, QDict *dict);
 
 void qmp_event_set_func_emit(QMPEventFuncEmit emit);
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 6b75bca317..4c24faba18 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -980,8 +980,7 @@ void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp)
 
         if (tray_was_open != tray_is_open) {
             char *id = blk_get_attached_dev_id(blk);
-            qapi_event_send_device_tray_moved(blk_name(blk), id, tray_is_open,
-                                              &error_abort);
+            qapi_event_send_device_tray_moved(blk_name(blk), id, tray_is_open);
             g_free(id);
         }
     }
@@ -1665,8 +1664,7 @@ static void send_qmp_error_event(BlockBackend *blk,
     qapi_event_send_block_io_error(blk_name(blk), !!bs,
                                    bs ? bdrv_get_node_name(bs) : NULL, optype,
                                    action, blk_iostatus_is_enabled(blk),
-                                   error == ENOSPC, strerror(error),
-                                   &error_abort);
+                                   error == ENOSPC, strerror(error));
 }
 
 /* This is done by device models because, while the block layer knows
@@ -1782,7 +1780,7 @@ void blk_eject(BlockBackend *blk, bool eject_flag)
      * the frontend experienced a tray event. */
     id = blk_get_attached_dev_id(blk);
     qapi_event_send_device_tray_moved(blk_name(blk), id,
-                                      eject_flag, &error_abort);
+                                      eject_flag);
     g_free(id);
 }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 2f9e58e0c4..0fcf5ae716 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4570,7 +4570,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
                                           *node_name != '\0', node_name,
                                           message, offset >= 0, offset,
                                           size >= 0, size,
-                                          fatal, &error_abort);
+                                          fatal);
     g_free(message);
 
     if (fatal) {
diff --git a/block/quorum.c b/block/quorum.c
index 9152da8c58..eb526cc0f1 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -199,7 +199,7 @@ static void quorum_report_bad(QuorumOpType type, uint64_t offset,
     }
 
     qapi_event_send_quorum_report_bad(type, !!msg, msg, node_name, start_sector,
-                                      end_sector - start_sector, &error_abort);
+                                      end_sector - start_sector);
 }
 
 static void quorum_report_failure(QuorumAIOCB *acb)
@@ -210,7 +210,7 @@ static void quorum_report_failure(QuorumAIOCB *acb)
                                       BDRV_SECTOR_SIZE);
 
     qapi_event_send_quorum_failure(reference, start_sector,
-                                   end_sector - start_sector, &error_abort);
+                                   end_sector - start_sector);
 }
 
 static int quorum_vote_error(QuorumAIOCB *acb);
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 1d48fc2077..85b78dc2a9 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -63,8 +63,7 @@ static int coroutine_fn before_write_notify(NotifierWithReturn *notifier,
         qapi_event_send_block_write_threshold(
             bs->node_name,
             amount,
-            bs->write_threshold_offset,
-            &error_abort);
+            bs->write_threshold_offset);
 
         /* autodisable to avoid flooding the monitor */
         write_threshold_disable(bs);
diff --git a/blockjob.c b/blockjob.c
index be5903aa96..bf7ef48f98 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -315,8 +315,7 @@ static void block_job_event_cancelled(Notifier *n, void *opaque)
                                         job->job.id,
                                         job->job.progress_total,
                                         job->job.progress_current,
-                                        job->speed,
-                                        &error_abort);
+                                        job->speed);
 }
 
 static void block_job_event_completed(Notifier *n, void *opaque)
@@ -338,8 +337,7 @@ static void block_job_event_completed(Notifier *n, void *opaque)
                                         job->job.progress_current,
                                         job->speed,
                                         !!msg,
-                                        msg,
-                                        &error_abort);
+                                        msg);
 }
 
 static void block_job_event_pending(Notifier *n, void *opaque)
@@ -351,8 +349,7 @@ static void block_job_event_pending(Notifier *n, void *opaque)
     }
 
     qapi_event_send_block_job_pending(job_type(&job->job),
-                                      job->job.id,
-                                      &error_abort);
+                                      job->job.id);
 }
 
 static void block_job_event_ready(Notifier *n, void *opaque)
@@ -367,7 +364,7 @@ static void block_job_event_ready(Notifier *n, void *opaque)
                                     job->job.id,
                                     job->job.progress_total,
                                     job->job.progress_current,
-                                    job->speed, &error_abort);
+                                    job->speed);
 }
 
 
@@ -494,7 +491,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
         qapi_event_send_block_job_error(job->job.id,
                                         is_read ? IO_OPERATION_TYPE_READ :
                                         IO_OPERATION_TYPE_WRITE,
-                                        action, &error_abort);
+                                        action);
     }
     if (action == BLOCK_ERROR_ACTION_STOP) {
         job_pause(&job->job);
diff --git a/cpus.c b/cpus.c
index b5844b7103..488a2b5c62 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1011,7 +1011,7 @@ static int do_vm_stop(RunState state, bool send_stop)
         runstate_set(state);
         vm_state_notify(0, state);
         if (send_stop) {
-            qapi_event_send_stop(&error_abort);
+            qapi_event_send_stop();
         }
     }
 
@@ -2059,13 +2059,13 @@ int vm_prepare_start(void)
      * the STOP event.
      */
     if (runstate_is_running()) {
-        qapi_event_send_stop(&error_abort);
-        qapi_event_send_resume(&error_abort);
+        qapi_event_send_stop();
+        qapi_event_send_resume();
         return -1;
     }
 
     /* We are sending this now, but the CPUs will be resumed shortly later */
-    qapi_event_send_resume(&error_abort);
+    qapi_event_send_resume();
 
     replay_enable_events();
     cpu_enable_ticks();
diff --git a/dump.c b/dump.c
index 04467b353e..2d8f6b3231 100644
--- a/dump.c
+++ b/dump.c
@@ -1890,8 +1890,7 @@ static void dump_process(DumpState *s, Error **errp)
     /* should never fail */
     assert(result);
     qapi_event_send_dump_completed(result, !!local_err, (local_err ? \
-                                   error_get_pretty(local_err) : NULL),
-                                   &error_abort);
+                                   error_get_pretty(local_err) : NULL));
     qapi_free_DumpQueryResult(result);
 
     error_propagate(errp, local_err);
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index b8d39012cd..aafdc61648 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -570,7 +570,7 @@ static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
             break;
         default:
             if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
-                qapi_event_send_suspend_disk(&error_abort);
+                qapi_event_send_suspend_disk();
                 qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
             }
             break;
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 5ae595ecbe..d19b7722f0 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -160,7 +160,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data,
            cdev = &cpu_st->devs[cpu_st->selector];
            cdev->ost_status = data;
            info = acpi_cpu_device_status(cpu_st->selector, cdev);
-           qapi_event_send_acpi_device_ost(info, &error_abort);
+           qapi_event_send_acpi_device_ost(info);
            qapi_free_ACPIOSTInfo(info);
            trace_cpuhp_acpi_write_ost_status(cpu_st->selector,
                                              cdev->ost_status);
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 0ff1712c4c..8c7c1013f3 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -161,7 +161,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
         /* TODO: implement memory removal on guest signal */
 
         info = acpi_memory_device_status(mem_st->selector, mdev);
-        qapi_event_send_acpi_device_ost(info, &error_abort);
+        qapi_event_send_acpi_device_ost(info);
         qapi_free_ACPIOSTInfo(info);
         break;
     case 0x14: /* set is_* fields  */
@@ -185,8 +185,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
             if (local_err) {
                 trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
                 qapi_event_send_mem_unplug_error(dev->id,
-                                                 error_get_pretty(local_err),
-                                                 &error_abort);
+                                                 error_get_pretty(local_err));
                 error_free(local_err);
                 break;
             }
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 679a824888..2cbe1d4ed5 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -114,8 +114,7 @@ static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
     }
 
     if (dev->id) {
-        qapi_event_send_vserport_change(dev->id, guest_connected,
-                                        &error_abort);
+        qapi_event_send_vserport_change(dev->id, guest_connected);
     }
 }
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cf0db4b6da..9cdef2ed98 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -996,8 +996,7 @@ static void device_finalize(Object *obj)
     if (dev->pending_deleted_event) {
         g_assert(dev->canonical_path);
 
-        qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path,
-                                       &error_abort);
+        qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path);
         g_free(dev->canonical_path);
         dev->canonical_path = NULL;
     }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f154756e85..4bdd5b8532 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -329,7 +329,7 @@ static void rxfilter_notify(NetClientState *nc)
     if (nc->rxfilter_notify_enabled) {
         gchar *path = object_get_canonical_path(OBJECT(n->qdev));
         qapi_event_send_nic_rx_filter_changed(!!n->netclient_name,
-                                              n->netclient_name, path, &error_abort);
+                                              n->netclient_name, path);
         g_free(path);
 
         /* disable event notification to avoid events flooding */
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index a37360537e..cd049f389d 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -118,7 +118,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     /* Generate a monitor event for the change */
-    qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
+    qapi_event_send_rtc_change(qemu_timedate_diff(&tm));
 
     host_ns = qemu_clock_get_ns(rtc_clock);
 
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 6f1f723b1f..b941aa1966 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -635,7 +635,7 @@ static void rtc_set_time(RTCState *s)
     s->base_rtc = mktimegm(&tm);
     s->last_update = qemu_clock_get_ns(rtc_clock);
 
-    qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
+    qapi_event_send_rtc_change(qemu_timedate_diff(&tm));
 }
 
 static void rtc_set_cmos(RTCState *s, const struct tm *tm)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1f7a87f094..fcd41d314e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -367,8 +367,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
     dev->actual = le32_to_cpu(config.actual);
     if (dev->actual != oldactual) {
         qapi_event_send_balloon_change(vm_ram_size -
-                        ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT),
-                        &error_abort);
+                        ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
     }
     trace_virtio_balloon_set_config(dev->actual, oldactual);
 }
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 6e8ba061d8..33e6c20184 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -102,17 +102,17 @@ void watchdog_perform_action(void)
 {
     switch (watchdog_action) {
     case WATCHDOG_ACTION_RESET:     /* same as 'system_reset' in monitor */
-        qapi_event_send_watchdog(WATCHDOG_ACTION_RESET, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_RESET);
         qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
         break;
 
     case WATCHDOG_ACTION_SHUTDOWN:  /* same as 'system_powerdown' in monitor */
-        qapi_event_send_watchdog(WATCHDOG_ACTION_SHUTDOWN, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_SHUTDOWN);
         qemu_system_powerdown_request();
         break;
 
     case WATCHDOG_ACTION_POWEROFF:  /* same as 'quit' command in monitor */
-        qapi_event_send_watchdog(WATCHDOG_ACTION_POWEROFF, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_POWEROFF);
         exit(0);
 
     case WATCHDOG_ACTION_PAUSE:     /* same as 'stop' command in monitor */
@@ -120,22 +120,21 @@ void watchdog_perform_action(void)
          * you would get a deadlock.  Bypass the problem.
          */
         qemu_system_vmstop_request_prepare();
-        qapi_event_send_watchdog(WATCHDOG_ACTION_PAUSE, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_PAUSE);
         qemu_system_vmstop_request(RUN_STATE_WATCHDOG);
         break;
 
     case WATCHDOG_ACTION_DEBUG:
-        qapi_event_send_watchdog(WATCHDOG_ACTION_DEBUG, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_DEBUG);
         fprintf(stderr, "watchdog: timer fired\n");
         break;
 
     case WATCHDOG_ACTION_NONE:
-        qapi_event_send_watchdog(WATCHDOG_ACTION_NONE, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_NONE);
         break;
 
     case WATCHDOG_ACTION_INJECT_NMI:
-        qapi_event_send_watchdog(WATCHDOG_ACTION_INJECT_NMI,
-                                 &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_INJECT_NMI);
         nmi_monitor_handle(0, NULL);
         break;
 
diff --git a/job.c b/job.c
index fa671b431a..03dfc59bcd 100644
--- a/job.c
+++ b/job.c
@@ -174,7 +174,7 @@ static void job_state_transition(Job *job, JobStatus s1)
     job->status = s1;
 
     if (!job_is_internal(job) && s1 != s0) {
-        qapi_event_send_job_status_change(job->id, job->status, &error_abort);
+        qapi_event_send_job_status_change(job->id, job->status);
     }
 }
 
diff --git a/migration/migration.c b/migration/migration.c
index 94d71f8b24..728d731433 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -203,7 +203,7 @@ void migration_incoming_state_destroy(void)
 static void migrate_generate_event(int new_state)
 {
     if (migrate_use_events()) {
-        qapi_event_send_migration(new_state, &error_abort);
+        qapi_event_send_migration(new_state);
     }
 }
 
@@ -301,7 +301,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;
 
-    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
+    qapi_event_send_migration(MIGRATION_STATUS_SETUP);
     if (!strcmp(uri, "defer")) {
         deferred_incoming_migration(errp);
     } else if (strstart(uri, "tcp:", &p)) {
diff --git a/migration/ram.c b/migration/ram.c
index 1cd98d6398..830406bc4f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1661,7 +1661,7 @@ static void migration_bitmap_sync(RAMState *rs)
         rs->bytes_xfer_prev = bytes_xfer_now;
     }
     if (migrate_use_events()) {
-        qapi_event_send_migration_pass(ram_counters.dirty_sync_count, NULL);
+        qapi_event_send_migration_pass(ram_counters.dirty_sync_count);
     }
 }
 
diff --git a/monitor.c b/monitor.c
index 0e3ac52d87..80cada1162 100644
--- a/monitor.c
+++ b/monitor.c
@@ -632,7 +632,7 @@ static void monitor_qapi_event_handler(void *opaque);
  * applying any rate limiting if required.
  */
 static void
-monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
+monitor_qapi_event_queue(QAPIEvent event, QDict *qdict)
 {
     MonitorQAPIEventConf *evconf;
     MonitorQAPIEventState *evstate;
@@ -4269,8 +4269,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
              * that command was dropped.
              */
             qapi_event_send_command_dropped(id,
-                                            COMMAND_DROP_REASON_QUEUE_FULL,
-                                            &error_abort);
+                                            COMMAND_DROP_REASON_QUEUE_FULL);
             qmp_request_free(req_obj);
             return;
         }
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index 519a296905..fd119bd0c3 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -44,8 +44,7 @@ static void pr_manager_send_status_changed_event(PRManagerHelper *pr_mgr)
     char *id = object_get_canonical_path_component(OBJECT(pr_mgr));
 
     if (id) {
-        qapi_event_send_pr_manager_status_changed(id, !!pr_mgr->ioc,
-                                                  &error_abort);
+        qapi_event_send_pr_manager_status_changed(id, !!pr_mgr->ioc);
     }
 }
 
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index 8677094ad1..9cddd72adb 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -95,7 +95,7 @@ static bool qdict_cmp_simple(QDict *a, QDict *b)
 
 /* This function is hooked as final emit function, which can verify the
    correctness. */
-static void event_test_emit(test_QAPIEvent event, QDict *d, Error **errp)
+static void event_test_emit(test_QAPIEvent event, QDict *d)
 {
     QDict *t;
     int64_t s, ms;
@@ -156,7 +156,7 @@ static void test_event_a(TestEventData *data,
     QDict *d;
     d = data->expect;
     qdict_put_str(d, "event", "EVENT_A");
-    qapi_event_send_event_a(&error_abort);
+    qapi_event_send_event_a();
 }
 
 static void test_event_b(TestEventData *data,
@@ -165,7 +165,7 @@ static void test_event_b(TestEventData *data,
     QDict *d;
     d = data->expect;
     qdict_put_str(d, "event", "EVENT_B");
-    qapi_event_send_event_b(&error_abort);
+    qapi_event_send_event_b();
 }
 
 static void test_event_c(TestEventData *data,
@@ -191,7 +191,7 @@ static void test_event_c(TestEventData *data,
     qdict_put_str(d, "event", "EVENT_C");
     qdict_put(d, "data", d_data);
 
-    qapi_event_send_event_c(true, 1, true, &b, "test2", &error_abort);
+    qapi_event_send_event_c(true, 1, true, &b, "test2");
 
     g_free(b.string);
 }
@@ -233,8 +233,7 @@ static void test_event_d(TestEventData *data,
     qdict_put_str(d, "event", "EVENT_D");
     qdict_put(d, "data", d_data);
 
-    qapi_event_send_event_d(&a, "test3", false, NULL, true, ENUM_ONE_VALUE3,
-                           &error_abort);
+    qapi_event_send_event_d(&a, "test3", false, NULL, true, ENUM_ONE_VALUE3);
 
     g_free(struct1.string);
     g_free(a.string);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index f8c0878529..a4fbbc3898 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -218,8 +218,7 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
     switch (event) {
     case SPICE_CHANNEL_EVENT_CONNECTED:
         qapi_event_send_spice_connected(qapi_SpiceServerInfo_base(server),
-                                        qapi_SpiceChannel_base(client),
-                                        &error_abort);
+                                        qapi_SpiceChannel_base(client));
         break;
     case SPICE_CHANNEL_EVENT_INITIALIZED:
         if (auth) {
@@ -228,13 +227,12 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
         }
         add_channel_info(client, info);
         channel_list_add(info);
-        qapi_event_send_spice_initialized(server, client, &error_abort);
+        qapi_event_send_spice_initialized(server, client);
         break;
     case SPICE_CHANNEL_EVENT_DISCONNECTED:
         channel_list_del(info);
         qapi_event_send_spice_disconnected(qapi_SpiceServerInfo_base(server),
-                                           qapi_SpiceChannel_base(client),
-                                           &error_abort);
+                                           qapi_SpiceChannel_base(client));
         break;
     default:
         break;
@@ -287,7 +285,7 @@ static void migrate_connect_complete_cb(SpiceMigrateInstance *sin)
 
 static void migrate_end_complete_cb(SpiceMigrateInstance *sin)
 {
-    qapi_event_send_spice_migrate_completed(&error_abort);
+    qapi_event_send_spice_migrate_completed();
     spice_migration_completed = true;
 }
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 359693238b..2345bd054a 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -296,14 +296,13 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
 
     switch (event) {
     case QAPI_EVENT_VNC_CONNECTED:
-        qapi_event_send_vnc_connected(si, qapi_VncClientInfo_base(vs->info),
-                                      &error_abort);
+        qapi_event_send_vnc_connected(si, qapi_VncClientInfo_base(vs->info));
         break;
     case QAPI_EVENT_VNC_INITIALIZED:
-        qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
+        qapi_event_send_vnc_initialized(si, vs->info);
         break;
     case QAPI_EVENT_VNC_DISCONNECTED:
-        qapi_event_send_vnc_disconnected(si, vs->info, &error_abort);
+        qapi_event_send_vnc_disconnected(si, vs->info);
         break;
     default:
         break;
diff --git a/vl.c b/vl.c
index 9442beee21..29b1830259 100644
--- a/vl.c
+++ b/vl.c
@@ -1646,8 +1646,7 @@ void qemu_system_reset(ShutdownCause reason)
         qemu_devices_reset();
     }
     if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
-        qapi_event_send_reset(shutdown_caused_by_guest(reason),
-                              &error_abort);
+        qapi_event_send_reset(shutdown_caused_by_guest(reason));
     }
     cpu_synchronize_all_post_reset();
 }
@@ -1660,11 +1659,11 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
         current_cpu->crash_occurred = true;
     }
     qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE,
-                                   !!info, info, &error_abort);
+                                   !!info, info);
     vm_stop(RUN_STATE_GUEST_PANICKED);
     if (!no_shutdown) {
         qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
-                                       !!info, info, &error_abort);
+                                       !!info, info);
         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_PANIC);
     }
 
@@ -1705,7 +1704,7 @@ static void qemu_system_suspend(void)
     pause_all_vcpus();
     notifier_list_notify(&suspend_notifiers, NULL);
     runstate_set(RUN_STATE_SUSPENDED);
-    qapi_event_send_suspend(&error_abort);
+    qapi_event_send_suspend();
 }
 
 void qemu_system_suspend_request(void)
@@ -1775,7 +1774,7 @@ void qemu_system_shutdown_request(ShutdownCause reason)
 
 static void qemu_system_powerdown(void)
 {
-    qapi_event_send_powerdown(&error_abort);
+    qapi_event_send_powerdown();
     notifier_list_notify(&powerdown_notifiers, NULL);
 }
 
@@ -1818,8 +1817,7 @@ static bool main_loop_should_exit(void)
     request = qemu_shutdown_requested();
     if (request) {
         qemu_kill_report();
-        qapi_event_send_shutdown(shutdown_caused_by_guest(request),
-                                 &error_abort);
+        qapi_event_send_shutdown(shutdown_caused_by_guest(request));
         if (no_shutdown) {
             vm_stop(RUN_STATE_SHUTDOWN);
         } else {
@@ -1842,7 +1840,7 @@ static bool main_loop_should_exit(void)
         notifier_list_notify(&wakeup_notifiers, &wakeup_reason);
         wakeup_reason = QEMU_WAKEUP_REASON_NONE;
         resume_all_vcpus();
-        qapi_event_send_wakeup(&error_abort);
+        qapi_event_send_wakeup();
     }
     if (qemu_powerdown_requested()) {
         qemu_system_powerdown();
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 5657524688..3953dc9837 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -18,7 +18,7 @@ from qapi.common import *
 def build_event_send_proto(name, arg_type, boxed):
     return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
         'c_name': c_name(name.lower()),
-        'param': build_params(arg_type, boxed, 'Error **errp')}
+        'param': build_params(arg_type, boxed, "")}
 
 
 def gen_event_send_decl(name, arg_type, boxed):
@@ -70,7 +70,6 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
 %(proto)s
 {
     QDict *qmp;
-    Error *err = NULL;
     QMPEventFuncEmit emit;
 ''',
                 proto=build_event_send_proto(name, arg_type, boxed))
@@ -103,45 +102,35 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
 ''')
         if not arg_type.is_implicit():
             ret += mcgen('''
-    visit_type_%(c_name)s(v, "%(name)s", &arg, &err);
+    visit_type_%(c_name)s(v, "%(name)s", &arg, NULL);
 ''',
                          name=name, c_name=arg_type.c_name())
         else:
             ret += mcgen('''
 
-    visit_start_struct(v, "%(name)s", NULL, 0, &err);
-    if (err) {
-        goto out;
-    }
-    visit_type_%(c_name)s_members(v, &param, &err);
-    if (!err) {
-        visit_check_struct(v, &err);
-    }
+    visit_start_struct(v, "%(name)s", NULL, 0, NULL);
+    visit_type_%(c_name)s_members(v, &param, NULL);
+    visit_check_struct(v, NULL);
     visit_end_struct(v, NULL);
 ''',
                          name=name, c_name=arg_type.c_name())
         ret += mcgen('''
-    if (err) {
-        goto out;
-    }
 
     visit_complete(v, &obj);
     qdict_put_obj(qmp, "data", obj);
 ''')
 
     ret += mcgen('''
-    emit(%(c_enum)s, qmp, &err);
+    emit(%(c_enum)s, qmp);
 
 ''',
                  c_enum=c_enum_const(event_enum_name, name))
 
     if arg_type and not arg_type.is_empty():
         ret += mcgen('''
-out:
     visit_free(v);
 ''')
     ret += mcgen('''
-    error_propagate(errp, err);
     qobject_unref(qmp);
 }
 ''')
-- 
2.17.1

Re: [Qemu-devel] [PATCH 3/9] qapi: remove error checks for event emission

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> In the whole QAPI event emission code we're passing in an Error* object
> along the whole stack.  That's never useful since it never fails after
> all.  Remove that.
>
> Then, remove that parameter from all the callers to send an event.
>
> Suggested-by: Eric Blake <eblake@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
[...]
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 5657524688..3953dc9837 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -18,7 +18,7 @@ from qapi.common import *
>  def build_event_send_proto(name, arg_type, boxed):
>      return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
>          'c_name': c_name(name.lower()),
> -        'param': build_params(arg_type, boxed, 'Error **errp')}
> +        'param': build_params(arg_type, boxed, "")}

Use ' instead of " for consistency.

But let's make @extra optional in the previous patch (default it to
None), so this spot becomes simpler.

>  
>  
>  def gen_event_send_decl(name, arg_type, boxed):
> @@ -70,7 +70,6 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
>  %(proto)s
>  {
>      QDict *qmp;
> -    Error *err = NULL;
>      QMPEventFuncEmit emit;
>  ''',
>                  proto=build_event_send_proto(name, arg_type, boxed))
> @@ -103,45 +102,35 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
>  ''')
>          if not arg_type.is_implicit():
>              ret += mcgen('''
> -    visit_type_%(c_name)s(v, "%(name)s", &arg, &err);
> +    visit_type_%(c_name)s(v, "%(name)s", &arg, NULL);

&error_abort, please.  This should never fail, and when it does,
something's seriously wrong.

>  ''',
>                           name=name, c_name=arg_type.c_name())
>          else:
>              ret += mcgen('''
>  
> -    visit_start_struct(v, "%(name)s", NULL, 0, &err);
> -    if (err) {
> -        goto out;
> -    }
> -    visit_type_%(c_name)s_members(v, &param, &err);
> -    if (!err) {
> -        visit_check_struct(v, &err);
> -    }
> +    visit_start_struct(v, "%(name)s", NULL, 0, NULL);
> +    visit_type_%(c_name)s_members(v, &param, NULL);
> +    visit_check_struct(v, NULL);

Likewise.

>      visit_end_struct(v, NULL);
>  ''',
>                           name=name, c_name=arg_type.c_name())
>          ret += mcgen('''
> -    if (err) {
> -        goto out;
> -    }
>  
>      visit_complete(v, &obj);
>      qdict_put_obj(qmp, "data", obj);
>  ''')
>  
>      ret += mcgen('''
> -    emit(%(c_enum)s, qmp, &err);
> +    emit(%(c_enum)s, qmp);
>  
>  ''',
>                   c_enum=c_enum_const(event_enum_name, name))
>  
>      if arg_type and not arg_type.is_empty():
>          ret += mcgen('''
> -out:
>      visit_free(v);
>  ''')
>      ret += mcgen('''
> -    error_propagate(errp, err);
>      qobject_unref(qmp);
>  }
>  ''')

Re: [Qemu-devel] [PATCH 3/9] qapi: remove error checks for event emission

[Peter Xu]

On Thu, Jul 05, 2018 at 10:43:13AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > In the whole QAPI event emission code we're passing in an Error* object
> > along the whole stack.  That's never useful since it never fails after
> > all.  Remove that.
> >
> > Then, remove that parameter from all the callers to send an event.
> >
> > Suggested-by: Eric Blake <eblake@redhat.com>
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> [...]
> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > index 5657524688..3953dc9837 100644
> > --- a/scripts/qapi/events.py
> > +++ b/scripts/qapi/events.py
> > @@ -18,7 +18,7 @@ from qapi.common import *
> >  def build_event_send_proto(name, arg_type, boxed):
> >      return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
> >          'c_name': c_name(name.lower()),
> > -        'param': build_params(arg_type, boxed, 'Error **errp')}
> > +        'param': build_params(arg_type, boxed, "")}
> 
> Use ' instead of " for consistency.
> 
> But let's make @extra optional in the previous patch (default it to
> None), so this spot becomes simpler.

Will do.

> 
> >  
> >  
> >  def gen_event_send_decl(name, arg_type, boxed):
> > @@ -70,7 +70,6 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
> >  %(proto)s
> >  {
> >      QDict *qmp;
> > -    Error *err = NULL;
> >      QMPEventFuncEmit emit;
> >  ''',
> >                  proto=build_event_send_proto(name, arg_type, boxed))
> > @@ -103,45 +102,35 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
> >  ''')
> >          if not arg_type.is_implicit():
> >              ret += mcgen('''
> > -    visit_type_%(c_name)s(v, "%(name)s", &arg, &err);
> > +    visit_type_%(c_name)s(v, "%(name)s", &arg, NULL);
> 
> &error_abort, please.  This should never fail, and when it does,
> something's seriously wrong.

Ok.

> 
> >  ''',
> >                           name=name, c_name=arg_type.c_name())
> >          else:
> >              ret += mcgen('''
> >  
> > -    visit_start_struct(v, "%(name)s", NULL, 0, &err);
> > -    if (err) {
> > -        goto out;
> > -    }
> > -    visit_type_%(c_name)s_members(v, &param, &err);
> > -    if (!err) {
> > -        visit_check_struct(v, &err);
> > -    }
> > +    visit_start_struct(v, "%(name)s", NULL, 0, NULL);
> > +    visit_type_%(c_name)s_members(v, &param, NULL);
> > +    visit_check_struct(v, NULL);
> 
> Likewise.

Done.  Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH 4/9] monitor: move need_resume flag into monitor struct

[Peter Xu]

It was put into the request object to show whether we'll need to resume
the monitor after dispatching the command.  Now we move that into the
monitor struct so that it might be even used in other places in the
future, e.g., out-of-band message flow controls.

One thing to mention is that there is no lock needed before when
accessing the flag since the request object will always be owned by a
single thread.  After we move it into monitor struct we need to protect
that flag since it might be accessed by multiple threads now.  Renaming
the qmp_queue_lock into qmp_lock to protect the flag as well.

No functional change.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 72 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/monitor.c b/monitor.c
index 80cada1162..215029bc22 100644
--- a/monitor.c
+++ b/monitor.c
@@ -176,14 +176,20 @@ typedef struct {
     bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
     bool capab[QMP_CAPABILITY__MAX];         /* offered and accepted */
     /*
-     * Protects qmp request/response queue.
+     * Protects qmp request/response queue, and need_resume flag.
      * Take monitor_lock first when you need both.
      */
-    QemuMutex qmp_queue_lock;
+    QemuMutex qmp_lock;
     /* Input queue that holds all the parsed QMP requests */
     GQueue *qmp_requests;
     /* Output queue contains all the QMP responses in order */
     GQueue *qmp_responses;
+    /*
+     * Whether we need to resume the monitor afterward.  This flag is
+     * used to emulate the old QMP server behavior that the current
+     * command must be completed before execution of the next one.
+     */
+    bool need_resume;
 } MonitorQMP;
 
 /*
@@ -261,12 +267,6 @@ struct QMPRequest {
      */
     QObject *req;
     Error *err;
-    /*
-     * Whether we need to resume the monitor afterward.  This flag is
-     * used to emulate the old QMP server behavior that the current
-     * command must be completed before execution of the next one.
-     */
-    bool need_resume;
 };
 typedef struct QMPRequest QMPRequest;
 
@@ -367,7 +367,7 @@ static void qmp_request_free(QMPRequest *req)
     g_free(req);
 }
 
-/* Caller must hold mon->qmp.qmp_queue_lock */
+/* Caller must hold mon->qmp.qmp_lock */
 static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
 {
     while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
@@ -375,7 +375,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
     }
 }
 
-/* Caller must hold the mon->qmp.qmp_queue_lock */
+/* Caller must hold the mon->qmp.qmp_lock */
 static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
 {
     while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
@@ -385,12 +385,23 @@ static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
 
 static void monitor_qmp_cleanup_queues(Monitor *mon)
 {
-    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_lock(&mon->qmp.qmp_lock);
     monitor_qmp_cleanup_req_queue_locked(mon);
     monitor_qmp_cleanup_resp_queue_locked(mon);
-    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_unlock(&mon->qmp.qmp_lock);
 }
 
+/* Try to resume the monitor if it was suspended due to any reason */
+static void monitor_qmp_try_resume(Monitor *mon)
+{
+    assert(monitor_is_qmp(mon));
+    qemu_mutex_lock(&mon->qmp.qmp_lock);
+    if (mon->qmp.need_resume) {
+        monitor_resume(mon);
+        mon->qmp.need_resume = false;
+    }
+    qemu_mutex_unlock(&mon->qmp.qmp_lock);
+}
 
 static void monitor_flush_locked(Monitor *mon);
 
@@ -525,9 +536,9 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
          * Push a reference to the response queue.  The I/O thread
          * drains that queue and emits.
          */
-        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+        qemu_mutex_lock(&mon->qmp.qmp_lock);
         g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
-        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        qemu_mutex_unlock(&mon->qmp.qmp_lock);
         qemu_bh_schedule(qmp_respond_bh);
     } else {
         /*
@@ -548,9 +559,9 @@ static QDict *monitor_qmp_response_pop_one(Monitor *mon)
 {
     QDict *data;
 
-    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_lock(&mon->qmp.qmp_lock);
     data = g_queue_pop_head(mon->qmp.qmp_responses);
-    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_unlock(&mon->qmp.qmp_lock);
 
     return data;
 }
@@ -769,7 +780,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
 {
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->mon_lock);
-    qemu_mutex_init(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_init(&mon->qmp.qmp_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
@@ -789,7 +800,7 @@ static void monitor_data_destroy(Monitor *mon)
     readline_free(mon->rs);
     qobject_unref(mon->outbuf);
     qemu_mutex_destroy(&mon->mon_lock);
-    qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_destroy(&mon->qmp.qmp_lock);
     monitor_qmp_cleanup_req_queue_locked(mon);
     monitor_qmp_cleanup_resp_queue_locked(mon);
     g_queue_free(mon->qmp.qmp_requests);
@@ -4151,9 +4162,9 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
     qemu_mutex_lock(&monitor_lock);
 
     QTAILQ_FOREACH(mon, &mon_list, entry) {
-        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+        qemu_mutex_lock(&mon->qmp.qmp_lock);
         req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
-        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        qemu_mutex_unlock(&mon->qmp.qmp_lock);
         if (req_obj) {
             break;
         }
@@ -4176,26 +4187,26 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+    Monitor *mon;
     QDict *rsp;
 
     if (!req_obj) {
         return;
     }
 
+    mon = req_obj->mon;
     if (req_obj->req) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);
+        monitor_qmp_respond(mon, rsp, NULL);
         qobject_unref(rsp);
     }
 
-    if (req_obj->need_resume) {
-        /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(req_obj->mon);
-    }
+    monitor_qmp_try_resume(mon);
+
     qmp_request_free(req_obj);
 
     /* Reschedule instead of looping so the main loop stays responsive */
@@ -4244,10 +4255,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     req_obj->id = id;
     req_obj->req = req;
     req_obj->err = err;
-    req_obj->need_resume = false;
 
     /* Protect qmp_requests and fetching its length. */
-    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_lock(&mon->qmp.qmp_lock);
 
     /*
      * If OOB is not enabled on the current monitor, we'll emulate the
@@ -4257,11 +4267,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
      */
     if (!qmp_oob_enabled(mon)) {
         monitor_suspend(mon);
-        req_obj->need_resume = true;
+        mon->qmp.need_resume = true;
     } else {
         /* Drop the request if queue is full. */
         if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+            qemu_mutex_unlock(&mon->qmp.qmp_lock);
             /*
              * FIXME @id's scope is just @mon, and broadcasting it is
              * wrong.  If another monitor's client has a command with
@@ -4281,7 +4291,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
      * etc. will be delivered to the handler side.
      */
     g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
-    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_unlock(&mon->qmp.qmp_lock);
 
     /* Kick the dispatcher routine */
     qemu_bh_schedule(qmp_dispatcher_bh);
-- 
2.17.1

Re: [Qemu-devel] [PATCH 4/9] monitor: move need_resume flag into monitor struct

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> It was put into the request object to show whether we'll need to resume
> the monitor after dispatching the command.  Now we move that into the
> monitor struct so that it might be even used in other places in the
> future, e.g., out-of-band message flow controls.
>
> One thing to mention is that there is no lock needed before when
> accessing the flag since the request object will always be owned by a
> single thread.  After we move it into monitor struct we need to protect
> that flag since it might be accessed by multiple threads now.  Renaming
> the qmp_queue_lock into qmp_lock to protect the flag as well.
>
> No functional change.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Marc-André's "[PATCH v3 04/38] monitor: no need to save need_resume" and
"[PATCH v3 05/38] monitor: further simplify previous patch" also mess
with need_resume.  Marc-André, could you have a look at this patch and
the next one?

Re: [Qemu-devel] [PATCH 4/9] monitor: move need_resume flag into monitor struct

[Peter Xu]

On Thu, Jul 05, 2018 at 10:51:33AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > It was put into the request object to show whether we'll need to resume
> > the monitor after dispatching the command.  Now we move that into the
> > monitor struct so that it might be even used in other places in the
> > future, e.g., out-of-band message flow controls.
> >
> > One thing to mention is that there is no lock needed before when
> > accessing the flag since the request object will always be owned by a
> > single thread.  After we move it into monitor struct we need to protect
> > that flag since it might be accessed by multiple threads now.  Renaming
> > the qmp_queue_lock into qmp_lock to protect the flag as well.
> >
> > No functional change.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Marc-André's "[PATCH v3 04/38] monitor: no need to save need_resume" and
> "[PATCH v3 05/38] monitor: further simplify previous patch" also mess
> with need_resume.  Marc-André, could you have a look at this patch and
> the next one?

Sorry I should have looked at those before hand.  I think I must be
waiting for another post to split the patches into two (after
Marc-Andre poked me with that thread) but then I forgot about that.

So now I suspect we'd better keep that flag since in the next patch
the suspend operation can happen conditionally now.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/9] monitor: move need_resume flag into monitor struct

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jul 05, 2018 at 10:51:33AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > It was put into the request object to show whether we'll need to resume
>> > the monitor after dispatching the command.  Now we move that into the
>> > monitor struct so that it might be even used in other places in the
>> > future, e.g., out-of-band message flow controls.
>> >
>> > One thing to mention is that there is no lock needed before when
>> > accessing the flag since the request object will always be owned by a
>> > single thread.  After we move it into monitor struct we need to protect
>> > that flag since it might be accessed by multiple threads now.  Renaming
>> > the qmp_queue_lock into qmp_lock to protect the flag as well.
>> >
>> > No functional change.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> 
>> Marc-André's "[PATCH v3 04/38] monitor: no need to save need_resume" and
>> "[PATCH v3 05/38] monitor: further simplify previous patch" also mess
>> with need_resume.  Marc-André, could you have a look at this patch and
>> the next one?
>
> Sorry I should have looked at those before hand.  I think I must be
> waiting for another post to split the patches into two (after
> Marc-Andre poked me with that thread) but then I forgot about that.
>
> So now I suspect we'd better keep that flag since in the next patch
> the suspend operation can happen conditionally now.

Could you two together figure out how to combine your work?  Would take
me off this critical path...

Re: [Qemu-devel] [PATCH 4/9] monitor: move need_resume flag into monitor struct

[Marc-André Lureau]

Hi

On Thu, Jul 5, 2018 at 1:09 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Xu <peterx@redhat.com> writes:
>
>> On Thu, Jul 05, 2018 at 10:51:33AM +0200, Markus Armbruster wrote:
>>> Peter Xu <peterx@redhat.com> writes:
>>>
>>> > It was put into the request object to show whether we'll need to resume
>>> > the monitor after dispatching the command.  Now we move that into the
>>> > monitor struct so that it might be even used in other places in the
>>> > future, e.g., out-of-band message flow controls.
>>> >
>>> > One thing to mention is that there is no lock needed before when
>>> > accessing the flag since the request object will always be owned by a
>>> > single thread.  After we move it into monitor struct we need to protect
>>> > that flag since it might be accessed by multiple threads now.  Renaming
>>> > the qmp_queue_lock into qmp_lock to protect the flag as well.
>>> >
>>> > No functional change.
>>> >
>>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>>>
>>> Marc-André's "[PATCH v3 04/38] monitor: no need to save need_resume" and
>>> "[PATCH v3 05/38] monitor: further simplify previous patch" also mess
>>> with need_resume.  Marc-André, could you have a look at this patch and
>>> the next one?
>>
>> Sorry I should have looked at those before hand.  I think I must be
>> waiting for another post to split the patches into two (after
>> Marc-Andre poked me with that thread) but then I forgot about that.
>>
>> So now I suspect we'd better keep that flag since in the next patch
>> the suspend operation can happen conditionally now.
>
> Could you two together figure out how to combine your work?  Would take
> me off this critical path...
>

With Peter patches, the monitor is also suspended when the queue of
command is too long (when oob-enabled). Thus the variable now covers
one of two different cases.

My patches only covered the first case simplification (legacy /
!oob-enabled suspend).

The second case could probably use a similar simplification, looking
at the queue length, but at this point I think I'd prefer to keep the
variable for clarity and sanity state checking.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 4/9] monitor: move need_resume flag into monitor struct

[Peter Xu]

On Thu, Jul 05, 2018 at 01:32:46PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jul 5, 2018 at 1:09 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > Peter Xu <peterx@redhat.com> writes:
> >
> >> On Thu, Jul 05, 2018 at 10:51:33AM +0200, Markus Armbruster wrote:
> >>> Peter Xu <peterx@redhat.com> writes:
> >>>
> >>> > It was put into the request object to show whether we'll need to resume
> >>> > the monitor after dispatching the command.  Now we move that into the
> >>> > monitor struct so that it might be even used in other places in the
> >>> > future, e.g., out-of-band message flow controls.
> >>> >
> >>> > One thing to mention is that there is no lock needed before when
> >>> > accessing the flag since the request object will always be owned by a
> >>> > single thread.  After we move it into monitor struct we need to protect
> >>> > that flag since it might be accessed by multiple threads now.  Renaming
> >>> > the qmp_queue_lock into qmp_lock to protect the flag as well.
> >>> >
> >>> > No functional change.
> >>> >
> >>> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >>>
> >>> Marc-André's "[PATCH v3 04/38] monitor: no need to save need_resume" and
> >>> "[PATCH v3 05/38] monitor: further simplify previous patch" also mess
> >>> with need_resume.  Marc-André, could you have a look at this patch and
> >>> the next one?
> >>
> >> Sorry I should have looked at those before hand.  I think I must be
> >> waiting for another post to split the patches into two (after
> >> Marc-Andre poked me with that thread) but then I forgot about that.
> >>
> >> So now I suspect we'd better keep that flag since in the next patch
> >> the suspend operation can happen conditionally now.
> >
> > Could you two together figure out how to combine your work?  Would take
> > me off this critical path...
> >
> 
> With Peter patches, the monitor is also suspended when the queue of
> command is too long (when oob-enabled). Thus the variable now covers
> one of two different cases.
> 
> My patches only covered the first case simplification (legacy /
> !oob-enabled suspend).
> 
> The second case could probably use a similar simplification, looking
> at the queue length, but at this point I think I'd prefer to keep the
> variable for clarity and sanity state checking.

Thanks for confirming that.

Meanwhile, I'm not 100% sure whether the trick can be played again
here (e.g., resume when queue length is N-1 for either of the queue).
If without the flag, there should be a very tricky condition (when
both queues are with length N-1) that we might resume the monitor
twice while we should only do that once.

Regards,

-- 
Peter Xu

[Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Peter Xu]

When we received too many qmp commands, previously we'll send
COMMAND_DROPPED events to monitors, then we'll drop the requests.  It
can only solve the flow control of the request queue, however it'll not
really work since we might queue unlimited events in the response queue
which is a potential risk.

Now instead of sending such an event, we stop consuming the client input
when we noticed that the queue is reaching its limitation before hand.
Then after we handled commands, we'll try to resume the monitor when
needed.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/monitor.c b/monitor.c
index 215029bc22..ebf862914f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -164,6 +164,8 @@ struct MonFdset {
     QLIST_ENTRY(MonFdset) next;
 };
 
+#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+
 typedef struct {
     JSONMessageParser parser;
     /*
@@ -396,10 +398,21 @@ static void monitor_qmp_try_resume(Monitor *mon)
 {
     assert(monitor_is_qmp(mon));
     qemu_mutex_lock(&mon->qmp.qmp_lock);
+
+    if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
+        /*
+         * This should not happen, but in case if it happens, we
+         * should still keep the monitor in suspend state
+         */
+        qemu_mutex_unlock(&mon->qmp.qmp_lock);
+        return;
+    }
+
     if (mon->qmp.need_resume) {
         monitor_resume(mon);
         mon->qmp.need_resume = false;
     }
+
     qemu_mutex_unlock(&mon->qmp.qmp_lock);
 }
 
@@ -4213,7 +4226,14 @@ static void monitor_qmp_bh_dispatcher(void *data)
     qemu_bh_schedule(qmp_dispatcher_bh);
 }
 
-#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+/* Called with Monitor.qmp.qmp_lock held. */
+static void monitor_qmp_suspend_locked(Monitor *mon)
+{
+    assert(monitor_is_qmp(mon));
+    assert(mon->qmp.need_resume == false);
+    monitor_suspend(mon);
+    mon->qmp.need_resume = true;
+}
 
 static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 {
@@ -4266,22 +4286,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
      * OOB is not enabled, the server will never drop any command.
      */
     if (!qmp_oob_enabled(mon)) {
-        monitor_suspend(mon);
-        mon->qmp.need_resume = true;
+        monitor_qmp_suspend_locked(mon);
     } else {
-        /* Drop the request if queue is full. */
-        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-            qemu_mutex_unlock(&mon->qmp.qmp_lock);
-            /*
-             * FIXME @id's scope is just @mon, and broadcasting it is
-             * wrong.  If another monitor's client has a command with
-             * the same ID in flight, the event will incorrectly claim
-             * that command was dropped.
-             */
-            qapi_event_send_command_dropped(id,
-                                            COMMAND_DROP_REASON_QUEUE_FULL);
-            qmp_request_free(req_obj);
-            return;
+        /*
+         * If the queue is reaching the length limitation, we queue
+         * this command, meanwhile we suspend the monitor to block new
+         * commands.  We'll resume ourselves until the queue has more
+         * space.
+         */
+        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX - 1) {
+            monitor_qmp_suspend_locked(mon);
         }
     }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Eric Blake]

On 07/04/2018 03:45 AM, Peter Xu wrote:
> When we received too many qmp commands, previously we'll send
> COMMAND_DROPPED events to monitors, then we'll drop the requests.  It
> can only solve the flow control of the request queue, however it'll not
> really work since we might queue unlimited events in the response queue
> which is a potential risk.
> 
> Now instead of sending such an event, we stop consuming the client input
> when we noticed that the queue is reaching its limitation before hand.
> Then after we handled commands, we'll try to resume the monitor when
> needed.

Is this the right thing to be doing?

If there is some event that we forgot to rate limit, and the client 
isn't consuming the output queue fast enough to keep up with events, we 
are making it so the client can't even send an OOB command that might 
otherwise stop the flood of events.  Refusing to parse input when the 
client isn't parsing output makes sense when the output is a result of 
the input, but when the output is the result of events unrelated to the 
input, it might still be nice to be responsive (even if with 
COMMAND_DROPPED events) to OOB input.  Then again, if events are not 
being parsed quickly by the client, it's debatable whether the client 
will parse COMMAND_DROPPED any sooner (even if COMMAND_DROPPED jumps the 
queue ahead of other events).

So I don't have a strong opinion on whether this patch is right or 
wrong, so much as voicing a potential concern to make sure I'm not 
overlooking something.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Peter Xu]

On Thu, Jul 05, 2018 at 11:47:30AM -0500, Eric Blake wrote:
> On 07/04/2018 03:45 AM, Peter Xu wrote:
> > When we received too many qmp commands, previously we'll send
> > COMMAND_DROPPED events to monitors, then we'll drop the requests.  It
> > can only solve the flow control of the request queue, however it'll not
> > really work since we might queue unlimited events in the response queue
> > which is a potential risk.

[1]

> > 
> > Now instead of sending such an event, we stop consuming the client input
> > when we noticed that the queue is reaching its limitation before hand.
> > Then after we handled commands, we'll try to resume the monitor when
> > needed.
> 
> Is this the right thing to be doing?
> 
> If there is some event that we forgot to rate limit, and the client isn't
> consuming the output queue fast enough to keep up with events, we are making
> it so the client can't even send an OOB command that might otherwise stop
> the flood of events.  Refusing to parse input when the client isn't parsing
> output makes sense when the output is a result of the input, but when the
> output is the result of events unrelated to the input, it might still be
> nice to be responsive (even if with COMMAND_DROPPED events) to OOB input.
> Then again, if events are not being parsed quickly by the client, it's
> debatable whether the client will parse COMMAND_DROPPED any sooner (even if
> COMMAND_DROPPED jumps the queue ahead of other events).
> 
> So I don't have a strong opinion on whether this patch is right or wrong, so
> much as voicing a potential concern to make sure I'm not overlooking
> something.

I can't say current solution is ideal, but AFAIU at least we can avoid
one potential risk to consume all the memories of QEMU which I
mentioned at [1], which might be triggered by a problematic client.
So I would at least think this a better approach than before, and
which we might consider to have for QEMU 3.0.

One question that we haven't yet solved is that, what if QEMU
generates some events internally very quickly and without stopping,
even without any interference from client?  What should we do with
that?  That's not yet covered by this patch.  For now, the limitation
will not apply to those events (please see monitor_qapi_event_emit()),
and my understanding is that we should trust QEMU that it won't do
such thing (and also we have event rate limiting framework to ease the
problem a bit).  However I'm not sure of that.  If we want to solve
this finally, IMHO we might need a separate patch (or patchset),
though that should be for internal events only, not really related to
the clients any more.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jul 05, 2018 at 11:47:30AM -0500, Eric Blake wrote:
>> On 07/04/2018 03:45 AM, Peter Xu wrote:
>> > When we received too many qmp commands, previously we'll send
>> > COMMAND_DROPPED events to monitors, then we'll drop the requests.  It
>> > can only solve the flow control of the request queue, however it'll not
>> > really work since we might queue unlimited events in the response queue
>> > which is a potential risk.
>
> [1]

Let's ignore events other than COMMAND_DROPPED for a minute.

The issue COMMAND_DROPPED tries to address is flow control: QMP client
sends commands faster than they can be processed, or accepts responses
slower than they're produced.  Buffers grow without bounds.

Either is unlikely to happen with a well-behaved QMP client, but
software being software, it's better not to let a misbehaving QMP client
make QEMU eat unbounded amounts of memory.  We limit memory consumption
elsewhere in QMP, e.g. the JSON parser.

COMMAND_DROPPED works fine for the input half: if the client sends
commands too quickly, the request queue gets full, and we start dropping
commands.

However, if the client accepts responses too slowly (or not at all)
without *also* sending commands too quickly, the output buffer still
grows without bounds.

We could detect this and drop commands.  Replaces swamping the output
buffer with responses by swamping it with COMMAND_DROPPED events.

We could change the meaning of COMMAND_DROPPED from "command with this
ID dropped" to "commands are being dropped, first one has this ID".
Since only in-band commands are dropped, and their responses are sent in
issue order, the next in-band reply tells us which commands were
dropped.

The simplest solution is to cork input by suspending the monitor (this
patch).  Reuses the non-OOB code.  Doesn't drop commands.  Sacrifices
monitor availability for OOB commands.

Done ignoring events other than COMMAND_DROPPED.

>> > Now instead of sending such an event, we stop consuming the client input
>> > when we noticed that the queue is reaching its limitation before hand.
>> > Then after we handled commands, we'll try to resume the monitor when
>> > needed.
>> 
>> Is this the right thing to be doing?
>> 
>> If there is some event that we forgot to rate limit, and the client isn't
>> consuming the output queue fast enough to keep up with events, we are making
>> it so the client can't even send an OOB command that might otherwise stop
>> the flood of events.

True.  This is yet another flow control scenario, only this time, it's
QEMU misbehaving.  

How much good the ability to issue commands will do in such a situation
is unclear.  We may not have a command to stop a flood, let alone an OOB
command.  But that's no excuse for ignoring the issue in our design.

>>                       Refusing to parse input when the client isn't parsing
>> output makes sense when the output is a result of the input, but when the
>> output is the result of events unrelated to the input, it might still be
>> nice to be responsive (even if with COMMAND_DROPPED events) to OOB input.

You're right, we should at least try find a way to keep the monitor
available when events beyond the client's control swamp the output
buffer.

>> Then again, if events are not being parsed quickly by the client, it's
>> debatable whether the client will parse COMMAND_DROPPED any sooner (even if
>> COMMAND_DROPPED jumps the queue ahead of other events).

The current code doesn't really support response queue jumping.  The
response queue gets drained quickly into the output buffer.  The
response queue therefore stays small (as long as bottom halves run as
they should).  It's the output buffer that grows without bound.  Jumping
there isn't as trivial as inserting at the front end of the response
queue, and probably should not be done.  If we want to jump, we should
reconsider how we drain the response queue.

>> So I don't have a strong opinion on whether this patch is right or wrong, so
>> much as voicing a potential concern to make sure I'm not overlooking
>> something.
>
> I can't say current solution is ideal, but AFAIU at least we can avoid
> one potential risk to consume all the memories of QEMU which I
> mentioned at [1], which might be triggered by a problematic client.
> So I would at least think this a better approach than before, and
> which we might consider to have for QEMU 3.0.
>
> One question that we haven't yet solved is that, what if QEMU
> generates some events internally very quickly and without stopping,
> even without any interference from client?  What should we do with
> that?  That's not yet covered by this patch.  For now, the limitation
> will not apply to those events (please see monitor_qapi_event_emit()),
> and my understanding is that we should trust QEMU that it won't do
> such thing (and also we have event rate limiting framework to ease the
> problem a bit).  However I'm not sure of that.  If we want to solve
> this finally, IMHO we might need a separate patch (or patchset),
> though that should be for internal events only, not really related to
> the clients any more.

monitor_qapi_event_queue() implements event rate limiting.  It's
controlled by monitor_qapi_event_conf[].  monitor_qapi_event_emit() is
the helper for monitor_qapi_event_queue() & friends that that does the
actual emission, without a rate limit.

Rate limiting assumes we can safely drop excess events.  Care has to be
taken not to lump events together that should remain separate.  For an
example, see commit 6d425eb94d3.

Any event an untrusted source (the guest, basically) can trigger must be
rate limited.

Trusted sources must not cause event floods.  Rate limiting is one way
to prevent floods.  We currently don't use it for that.  Instead, we put
the burden on the trusted sources.

We can talk about a second line of defense against event floods from
trusted sources.  We haven't felt enough of a need so far.

The question remains how to design for QMP OOB availability.  Perhaps we
can ignore events (other than COMMAND_DROPPED, if we keep that) when
deciding whether a QMP connection has exceeded its buffer allowance.
That way, OOB doesn't create a new way to eat memory without bounds.
The existing way "QMP client can't keep up with an event flood" remains
unaddressed.

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> When we received too many qmp commands, previously we'll send
> COMMAND_DROPPED events to monitors, then we'll drop the requests.  It
> can only solve the flow control of the request queue, however it'll not
> really work since we might queue unlimited events in the response queue
> which is a potential risk.
>
> Now instead of sending such an event, we stop consuming the client input
> when we noticed that the queue is reaching its limitation before hand.
> Then after we handled commands, we'll try to resume the monitor when
> needed.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 46 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 215029bc22..ebf862914f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -164,6 +164,8 @@ struct MonFdset {
>      QLIST_ENTRY(MonFdset) next;
>  };
>  
> +#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> +
>  typedef struct {
>      JSONMessageParser parser;
>      /*
> @@ -396,10 +398,21 @@ static void monitor_qmp_try_resume(Monitor *mon)
>  {
>      assert(monitor_is_qmp(mon));
>      qemu_mutex_lock(&mon->qmp.qmp_lock);
> +
> +    if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> +        /*
> +         * This should not happen, but in case if it happens, we
> +         * should still keep the monitor in suspend state
> +         */
> +        qemu_mutex_unlock(&mon->qmp.qmp_lock);
> +        return;
> +    }
> +
>      if (mon->qmp.need_resume) {
>          monitor_resume(mon);
>          mon->qmp.need_resume = false;
>      }
> +
>      qemu_mutex_unlock(&mon->qmp.qmp_lock);
>  }
>  
> @@ -4213,7 +4226,14 @@ static void monitor_qmp_bh_dispatcher(void *data)
>      qemu_bh_schedule(qmp_dispatcher_bh);
>  }
>  
> -#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> +/* Called with Monitor.qmp.qmp_lock held. */
> +static void monitor_qmp_suspend_locked(Monitor *mon)
> +{
> +    assert(monitor_is_qmp(mon));
> +    assert(mon->qmp.need_resume == false);
> +    monitor_suspend(mon);
> +    mon->qmp.need_resume = true;
> +}
>  
>  static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>  {
> @@ -4266,22 +4286,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>       * OOB is not enabled, the server will never drop any command.
>       */
>      if (!qmp_oob_enabled(mon)) {
> -        monitor_suspend(mon);
> -        mon->qmp.need_resume = true;
> +        monitor_qmp_suspend_locked(mon);
>      } else {
> -        /* Drop the request if queue is full. */
> -        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> -            qemu_mutex_unlock(&mon->qmp.qmp_lock);
> -            /*
> -             * FIXME @id's scope is just @mon, and broadcasting it is
> -             * wrong.  If another monitor's client has a command with
> -             * the same ID in flight, the event will incorrectly claim
> -             * that command was dropped.
> -             */
> -            qapi_event_send_command_dropped(id,
> -                                            COMMAND_DROP_REASON_QUEUE_FULL);
> -            qmp_request_free(req_obj);
> -            return;
> +        /*
> +         * If the queue is reaching the length limitation, we queue
> +         * this command, meanwhile we suspend the monitor to block new
> +         * commands.  We'll resume ourselves until the queue has more
> +         * space.
> +         */
> +        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX - 1) {
> +            monitor_qmp_suspend_locked(mon);
>          }
>      }

If I understand the code correctly, the response queue
mon->qmp..qmp_requests gets drained into the output buffer mon->outbuf
by bottom half monitor_qmp_bh_responder().  The response queue remains
small, it's the output buffer that grows without bounds.  Your test for
"limit exceeded" measures the response queue.  It needs to measure the
output buffer instead.

We could drain the response queue only when the output buffer isn't too
full.  I think that makes sense only if we have a compelling use for
keeping responses in QDict form for a longer time.

Marc-André has a patch to cut out the response queue.  Whether it still
makes sense I don't know.
[PATCH v3 03/38] Revert "qmp: isolate responses into io thread"

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Peter Xu]

On Fri, Jul 06, 2018 at 10:09:58AM +0200, Markus Armbruster wrote:
> If I understand the code correctly, the response queue
> mon->qmp..qmp_requests gets drained into the output buffer mon->outbuf
> by bottom half monitor_qmp_bh_responder().  The response queue remains
> small, it's the output buffer that grows without bounds.  Your test for
> "limit exceeded" measures the response queue.  It needs to measure the
> output buffer instead.
> 
> We could drain the response queue only when the output buffer isn't too
> full.  I think that makes sense only if we have a compelling use for
> keeping responses in QDict form for a longer time.

Indeed.  I didn't really notice that we're having an unlimited out_buf
there.

To double confirm it's working, I firstly added some tracepoints:

=============
diff --git a/monitor.c b/monitor.c
index 9eb9f06599..18ab609eab 100644
--- a/monitor.c
+++ b/monitor.c
@@ -375,6 +375,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
     while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
         qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
     }
+    trace_monitor_qmp_request_queue(mon, 0);
 }

 /* Caller must hold the mon->qmp.qmp_lock */
@@ -383,6 +384,7 @@ static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
     while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
         qobject_unref((QDict *)g_queue_pop_head(mon->qmp.qmp_responses));
     }
+    trace_monitor_qmp_response_queue(mon, 0);
 }

 static void monitor_qmp_cleanup_queues(Monitor *mon)
@@ -408,6 +410,7 @@ static void monitor_qmp_try_resume_locked(Monitor *mon)
     if (mon->qmp.need_resume) {
         monitor_resume(mon);
         mon->qmp.need_resume = false;
+        trace_monitor_qmp_resume(mon);
     }
 }

@@ -555,6 +558,7 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
          */
         qemu_mutex_lock(&mon->qmp.qmp_lock);
         g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
+        trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
         qemu_mutex_unlock(&mon->qmp.qmp_lock);
         qemu_bh_schedule(qmp_respond_bh);
     } else {
@@ -578,6 +582,7 @@ static QDict *monitor_qmp_response_pop_one(Monitor *mon)

     qemu_mutex_lock(&mon->qmp.qmp_lock);
     data = g_queue_pop_head(mon->qmp.qmp_responses);
+    trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
     /* In case if we were suspended due to response queue full */
     monitor_qmp_try_resume_locked(mon);
     qemu_mutex_unlock(&mon->qmp.qmp_lock);
@@ -4183,6 +4188,7 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
     QTAILQ_FOREACH(mon, &mon_list, entry) {
         qemu_mutex_lock(&mon->qmp.qmp_lock);
         req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
+        trace_monitor_qmp_request_queue(mon, mon->qmp.qmp_requests->length);
         qemu_mutex_unlock(&mon->qmp.qmp_lock);
         if (req_obj) {
             break;
@@ -4239,6 +4245,7 @@ static void monitor_qmp_suspend_locked(Monitor *mon)
     assert(mon->qmp.need_resume == false);
     monitor_suspend(mon);
     mon->qmp.need_resume = true;
+    trace_monitor_qmp_suspend(mon);
 }

 static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
@@ -4312,6 +4319,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
      * etc. will be delivered to the handler side.
      */
     g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
+    trace_monitor_qmp_request_queue(mon, mon->qmp.qmp_requests->length);
     qemu_mutex_unlock(&mon->qmp.qmp_lock);

     /* Kick the dispatcher routine */
diff --git a/trace-events b/trace-events
index c445f54773..bd9dade938 100644
--- a/trace-events
+++ b/trace-events
@@ -50,6 +50,10 @@ handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
 monitor_suspend(void *ptr, int cnt) "mon %p: %d"
 monitor_qmp_cmd_in_band(const char *id) "%s"
 monitor_qmp_cmd_out_of_band(const char *id) "%s"
+monitor_qmp_suspend(void *mon) "mon=%p"
+monitor_qmp_resume(void *mon) "mon=%p"
+monitor_qmp_request_queue(void *mon, int len) "mon=%p len=%d"
+monitor_qmp_response_queue(void *mon, int len) "mon=%p len=%d"

 # dma-helpers.c
 dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
=============

Then, I explicitly hanged the response BH by:

=============
diff --git a/monitor.c b/monitor.c
index 18ab609eab..1f38084a4c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -560,7 +560,7 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
         g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
         trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
         qemu_mutex_unlock(&mon->qmp.qmp_lock);
-        qemu_bh_schedule(qmp_respond_bh);
+        //qemu_bh_schedule(qmp_respond_bh);
     } else {
         /*
          * Not using monitor I/O thread, i.e. we are in the main thread.
=============

Then this is what I got for command:

$ cat cmd_list | qemu-system-x86_64 -qmp stdio -nographic -nodefaults -trace enable="monitor_qmp*"

Result:

9815@1530867603.464498:monitor_qmp_response_queue mon=0x55de6bfc4800 len=1
9815@1530867603.464782:monitor_qmp_suspend mon=0x55de6bfc4800
9815@1530867603.464788:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
9815@1530867603.489017:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489030:monitor_qmp_cmd_in_band
9815@1530867603.489050:monitor_qmp_response_queue mon=0x55de6bfc4800 len=2
9815@1530867603.489057:monitor_qmp_resume mon=0x55de6bfc4800         <------- [0]
9815@1530867603.489099:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489205:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
9815@1530867603.489311:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489317:monitor_qmp_cmd_in_band
9815@1530867603.489329:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
9815@1530867603.489419:monitor_qmp_response_queue mon=0x55de6bfc4800 len=3
9815@1530867603.489432:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489437:monitor_qmp_cmd_in_band
9815@1530867603.489450:monitor_qmp_response_queue mon=0x55de6bfc4800 len=4
9815@1530867603.489459:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489465:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
9815@1530867603.489468:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489471:monitor_qmp_cmd_in_band
9815@1530867603.489484:monitor_qmp_response_queue mon=0x55de6bfc4800 len=5
9815@1530867603.489492:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489575:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
9815@1530867603.489584:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489595:monitor_qmp_cmd_in_band
9815@1530867603.489614:monitor_qmp_response_queue mon=0x55de6bfc4800 len=6
9815@1530867603.489624:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489693:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
9815@1530867603.489703:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489708:monitor_qmp_cmd_in_band
9815@1530867603.489725:monitor_qmp_response_queue mon=0x55de6bfc4800 len=7
9815@1530867603.489736:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489818:monitor_qmp_suspend mon=0x55de6bfc4800  <-------------[1]
9815@1530867603.489823:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
9815@1530867603.489837:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
9815@1530867603.489847:monitor_qmp_cmd_in_band
9815@1530867603.489874:monitor_qmp_response_queue mon=0x55de6bfc4800 len=8
9815@1530867603.489892:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
[hangs here]

I think we have [1] right after response queue reaches N-1, I suppose
that means current patch is working.  Note that the suspend at [0] is
only the first "qmp_capability" command, since before we enable OOB
we'll still emulate the old monitor so that's by design.

> 
> Marc-André has a patch to cut out the response queue.  Whether it still
> makes sense I don't know.
> [PATCH v3 03/38] Revert "qmp: isolate responses into io thread"

I think we discussed this before, I agree with Marc-Andre that most of
the codes that are related to response flushing should be thread safe
now.  Actually it was not when we started to draft the out-of-band
thing, and that's the major reason why I refactored my old code to
explicitly separate the dispatcher and the responder, so that
responder can be isolated and be together with the parser.

I don't have obvious reason to remove this layer of isolation if it
works well (and actually the code is not that much, and IMHO the
response queue is a clear interface that maybe we can use in the
future too), I think my major concern now is that we're in rc phase
for QEMU-3.0 so that it at least seems to be a big change to monitor
layer.  We might consider to revert back to no-responder way if we
really want, but I guess we'd better do as much testing as when we
started to play with out-of-band to make sure we won't miss anything
important (and I guess the patch will need a non-trivial rebase after
we worked upon the queues recently).  Considering that, I don't really
see much help on removing that layer.  Or, do we have any other reason
to remove the response queue that I'm not aware of?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jul 06, 2018 at 10:09:58AM +0200, Markus Armbruster wrote:
>> If I understand the code correctly, the response queue
>> mon->qmp..qmp_requests gets drained into the output buffer mon->outbuf
>> by bottom half monitor_qmp_bh_responder().  The response queue remains
>> small, it's the output buffer that grows without bounds.  Your test for
>> "limit exceeded" measures the response queue.  It needs to measure the
>> output buffer instead.
>> 
>> We could drain the response queue only when the output buffer isn't too
>> full.  I think that makes sense only if we have a compelling use for
>> keeping responses in QDict form for a longer time.
>
> Indeed.  I didn't really notice that we're having an unlimited out_buf
> there.
>
> To double confirm it's working, I firstly added some tracepoints:
>
> =============
> diff --git a/monitor.c b/monitor.c
> index 9eb9f06599..18ab609eab 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -375,6 +375,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
>      while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
>          qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
>      }
> +    trace_monitor_qmp_request_queue(mon, 0);
>  }
>
>  /* Caller must hold the mon->qmp.qmp_lock */
> @@ -383,6 +384,7 @@ static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
>      while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
>          qobject_unref((QDict *)g_queue_pop_head(mon->qmp.qmp_responses));
>      }
> +    trace_monitor_qmp_response_queue(mon, 0);
>  }
>
>  static void monitor_qmp_cleanup_queues(Monitor *mon)
> @@ -408,6 +410,7 @@ static void monitor_qmp_try_resume_locked(Monitor *mon)
>      if (mon->qmp.need_resume) {
>          monitor_resume(mon);
>          mon->qmp.need_resume = false;
> +        trace_monitor_qmp_resume(mon);
>      }
>  }
>
> @@ -555,6 +558,7 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
>           */
>          qemu_mutex_lock(&mon->qmp.qmp_lock);
>          g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
> +        trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
>          qemu_mutex_unlock(&mon->qmp.qmp_lock);
>          qemu_bh_schedule(qmp_respond_bh);
>      } else {
> @@ -578,6 +582,7 @@ static QDict *monitor_qmp_response_pop_one(Monitor *mon)
>
>      qemu_mutex_lock(&mon->qmp.qmp_lock);
>      data = g_queue_pop_head(mon->qmp.qmp_responses);
> +    trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
>      /* In case if we were suspended due to response queue full */
>      monitor_qmp_try_resume_locked(mon);
>      qemu_mutex_unlock(&mon->qmp.qmp_lock);
> @@ -4183,6 +4188,7 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
>      QTAILQ_FOREACH(mon, &mon_list, entry) {
>          qemu_mutex_lock(&mon->qmp.qmp_lock);
>          req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
> +        trace_monitor_qmp_request_queue(mon, mon->qmp.qmp_requests->length);
>          qemu_mutex_unlock(&mon->qmp.qmp_lock);
>          if (req_obj) {
>              break;
> @@ -4239,6 +4245,7 @@ static void monitor_qmp_suspend_locked(Monitor *mon)
>      assert(mon->qmp.need_resume == false);
>      monitor_suspend(mon);
>      mon->qmp.need_resume = true;
> +    trace_monitor_qmp_suspend(mon);
>  }
>
>  static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> @@ -4312,6 +4319,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>       * etc. will be delivered to the handler side.
>       */
>      g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
> +    trace_monitor_qmp_request_queue(mon, mon->qmp.qmp_requests->length);
>      qemu_mutex_unlock(&mon->qmp.qmp_lock);
>
>      /* Kick the dispatcher routine */
> diff --git a/trace-events b/trace-events
> index c445f54773..bd9dade938 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -50,6 +50,10 @@ handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
>  monitor_suspend(void *ptr, int cnt) "mon %p: %d"
>  monitor_qmp_cmd_in_band(const char *id) "%s"
>  monitor_qmp_cmd_out_of_band(const char *id) "%s"
> +monitor_qmp_suspend(void *mon) "mon=%p"
> +monitor_qmp_resume(void *mon) "mon=%p"
> +monitor_qmp_request_queue(void *mon, int len) "mon=%p len=%d"
> +monitor_qmp_response_queue(void *mon, int len) "mon=%p len=%d"
>
>  # dma-helpers.c
>  dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
> =============
>
> Then, I explicitly hanged the response BH by:
>
> =============
> diff --git a/monitor.c b/monitor.c
> index 18ab609eab..1f38084a4c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -560,7 +560,7 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
>          g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
>          trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
>          qemu_mutex_unlock(&mon->qmp.qmp_lock);
> -        qemu_bh_schedule(qmp_respond_bh);
> +        //qemu_bh_schedule(qmp_respond_bh);
>      } else {
>          /*
>           * Not using monitor I/O thread, i.e. we are in the main thread.

This stops draining of the response queue into the output buffer.

> =============
>
> Then this is what I got for command:
>
> $ cat cmd_list | qemu-system-x86_64 -qmp stdio -nographic -nodefaults -trace enable="monitor_qmp*"
>
> Result:
>
> 9815@1530867603.464498:monitor_qmp_response_queue mon=0x55de6bfc4800 len=1
> 9815@1530867603.464782:monitor_qmp_suspend mon=0x55de6bfc4800
> 9815@1530867603.464788:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> 9815@1530867603.489017:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489030:monitor_qmp_cmd_in_band
> 9815@1530867603.489050:monitor_qmp_response_queue mon=0x55de6bfc4800 len=2
> 9815@1530867603.489057:monitor_qmp_resume mon=0x55de6bfc4800         <------- [0]
> 9815@1530867603.489099:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489205:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> 9815@1530867603.489311:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489317:monitor_qmp_cmd_in_band
> 9815@1530867603.489329:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> 9815@1530867603.489419:monitor_qmp_response_queue mon=0x55de6bfc4800 len=3
> 9815@1530867603.489432:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489437:monitor_qmp_cmd_in_band
> 9815@1530867603.489450:monitor_qmp_response_queue mon=0x55de6bfc4800 len=4
> 9815@1530867603.489459:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489465:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> 9815@1530867603.489468:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489471:monitor_qmp_cmd_in_band
> 9815@1530867603.489484:monitor_qmp_response_queue mon=0x55de6bfc4800 len=5
> 9815@1530867603.489492:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489575:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> 9815@1530867603.489584:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489595:monitor_qmp_cmd_in_band
> 9815@1530867603.489614:monitor_qmp_response_queue mon=0x55de6bfc4800 len=6
> 9815@1530867603.489624:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489693:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> 9815@1530867603.489703:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489708:monitor_qmp_cmd_in_band
> 9815@1530867603.489725:monitor_qmp_response_queue mon=0x55de6bfc4800 len=7
> 9815@1530867603.489736:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489818:monitor_qmp_suspend mon=0x55de6bfc4800  <-------------[1]
> 9815@1530867603.489823:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> 9815@1530867603.489837:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> 9815@1530867603.489847:monitor_qmp_cmd_in_band
> 9815@1530867603.489874:monitor_qmp_response_queue mon=0x55de6bfc4800 len=8
> 9815@1530867603.489892:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> [hangs here]
>
> I think we have [1] right after response queue reaches N-1, I suppose
> that means current patch is working.  Note that the suspend at [0] is
> only the first "qmp_capability" command, since before we enable OOB
> we'll still emulate the old monitor so that's by design.

Your experiment shows the patch succeeds at limiting the response queue
length.  However, the "response queue mon->qmp..qmp_requests gets
drained into the output buffer mon->outbuf by bottom half
monitor_qmp_bh_responder().  The response queue remains small, it's the
output buffer that grows without bounds".

To demonstrate that, let me add another tracepoint:

diff --git a/monitor.c b/monitor.c
index 3bea239bc7..e35e15b43e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -451,6 +451,7 @@ static void monitor_flush_locked(Monitor *mon)
 
     buf = qstring_get_str(mon->outbuf);
     len = qstring_get_length(mon->outbuf);
+    trace_monitor_flush_locked(mon, len);
 
     if (len && !mon->mux_out) {
         rc = qemu_chr_fe_write(&mon->chr, (const uint8_t *) buf, len);
diff --git a/trace-events b/trace-events
index bd9dade938..b6d5d28041 100644
--- a/trace-events
+++ b/trace-events
@@ -54,6 +54,7 @@ monitor_qmp_suspend(void *mon) "mon=%p"
 monitor_qmp_resume(void *mon) "mon=%p"
 monitor_qmp_request_queue(void *mon, int len) "mon=%p len=%d"
 monitor_qmp_response_queue(void *mon, int len) "mon=%p len=%d"
+monitor_flush_locked(void *mon, int len) "mon=%p len=%d"
 
 # dma-helpers.c
 dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"

Start this QEMU:

    $ upstream-qemu -chardev socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp -display none -nodefaults -trace enable="monitor_qmp*" -trace enable=monitor_flush_locked
    3236@1530882919.092979:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882919.093018:monitor_qmp_response_queue mon=0x56192215baf0 len=2
    3236@1530882919.093076:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882919.093095:monitor_flush_locked mon=0x56192215baf0 len=107
    3236@1530882919.093103:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882919.093112:monitor_flush_locked mon=0x56192215baf0 len=82
    3236@1530882919.093118:monitor_qmp_response_queue mon=0x56192215baf0 len=0

Now let me feed a couple of commands to that without reading any of
their responses.  To do that, run in another terminal:

    $ socat -u STDIO UNIX:test-qmp
    { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }

Note that -u makes socat use the first address (STDIO) only for reading,
and the second one (the QMP socket) only for writing.  This QMP client
fails to read any responses.  QEMU prints:

    3236@1530882921.894996:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882921.895049:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882921.895171:monitor_flush_locked mon=0x56192215baf0 len=136
    3236@1530882921.895221:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882923.954233:monitor_qmp_suspend mon=0x56192215baf0
    3236@1530882923.954262:monitor_qmp_request_queue mon=0x56192215baf0 len=1
    3236@1530882923.954314:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882923.954341:monitor_qmp_cmd_in_band 
    3236@1530882923.954407:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882923.954430:monitor_qmp_resume mon=0x56192215baf0
    3236@1530882923.954476:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882923.954501:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882923.954548:monitor_flush_locked mon=0x56192215baf0 len=16
    3236@1530882923.954587:monitor_qmp_response_queue mon=0x56192215baf0 len=0

Back in socat, execute a few commands, one after the other, with short
pauses in between:

    { "execute": "query-qmp-schema" }
    { "execute": "query-qmp-schema" }
    { "execute": "query-qmp-schema" }
    { "execute": "query-qmp-schema" }
    { "execute": "query-qmp-schema" }
    { "execute": "quit" }

QEMU prints:

    3236@1530882931.529666:monitor_qmp_request_queue mon=0x56192215baf0 len=1
    3236@1530882931.529768:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882931.529799:monitor_qmp_cmd_in_band 
    3236@1530882931.537403:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882931.537474:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882931.537481:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882931.544809:monitor_flush_locked mon=0x56192215baf0 len=129073
    3236@1530882931.548465:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882935.345300:monitor_qmp_request_queue mon=0x56192215baf0 len=1
    3236@1530882935.345399:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882935.345431:monitor_qmp_cmd_in_band 
    3236@1530882935.355347:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882935.355453:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882935.355513:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882935.366228:monitor_flush_locked mon=0x56192215baf0 len=129073
    3236@1530882935.369826:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882936.985228:monitor_qmp_request_queue mon=0x56192215baf0 len=1
    3236@1530882936.985338:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882936.985371:monitor_qmp_cmd_in_band 
    3236@1530882936.991813:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882936.991893:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882936.991900:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882936.999026:monitor_flush_locked mon=0x56192215baf0 len=148514
    3236@1530882937.002865:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882937.905712:monitor_qmp_request_queue mon=0x56192215baf0 len=1
    3236@1530882937.905873:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882937.905898:monitor_qmp_cmd_in_band 
    3236@1530882937.911889:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882937.911958:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882937.911964:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882937.919079:monitor_flush_locked mon=0x56192215baf0 len=277587
    3236@1530882937.922546:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882939.313235:monitor_qmp_request_queue mon=0x56192215baf0 len=1
    3236@1530882939.313551:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882939.313586:monitor_qmp_cmd_in_band 
    3236@1530882939.323542:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882939.323619:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882939.323651:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882939.335255:monitor_flush_locked mon=0x56192215baf0 len=406660
    3236@1530882939.338829:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882945.161013:monitor_qmp_request_queue mon=0x56192215baf0 len=1
    3236@1530882945.161122:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882945.161153:monitor_qmp_cmd_in_band 
    3236@1530882945.163191:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882945.163283:monitor_qmp_response_queue mon=0x56192215baf0 len=2
    3236@1530882945.163360:monitor_qmp_response_queue mon=0x56192215baf0 len=1
    3236@1530882945.163407:monitor_flush_locked mon=0x56192215baf0 len=406676
    3236@1530882945.163439:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882945.163489:monitor_flush_locked mon=0x56192215baf0 len=406787
    3236@1530882945.163518:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882945.163695:monitor_qmp_response_queue mon=0x56192215baf0 len=0
    3236@1530882945.163725:monitor_flush_locked mon=0x56192215baf0 len=406787
    3236@1530882945.163866:monitor_qmp_request_queue mon=0x56192215baf0 len=0
    3236@1530882945.163879:monitor_qmp_response_queue mon=0x56192215baf0 len=0

The response queue length stays small (the bottom half drains it just
fine), but the output buffer keeps growing.

>> Marc-André has a patch to cut out the response queue.  Whether it still
>> makes sense I don't know.
>> [PATCH v3 03/38] Revert "qmp: isolate responses into io thread"
>
> I think we discussed this before, I agree with Marc-Andre that most of
> the codes that are related to response flushing should be thread safe
> now.  Actually it was not when we started to draft the out-of-band
> thing, and that's the major reason why I refactored my old code to
> explicitly separate the dispatcher and the responder, so that
> responder can be isolated and be together with the parser.
>
> I don't have obvious reason to remove this layer of isolation if it
> works well (and actually the code is not that much, and IMHO the
> response queue is a clear interface that maybe we can use in the
> future too), I think my major concern now is that we're in rc phase
> for QEMU-3.0 so that it at least seems to be a big change to monitor
> layer.  We might consider to revert back to no-responder way if we
> really want, but I guess we'd better do as much testing as when we
> started to play with out-of-band to make sure we won't miss anything
> important (and I guess the patch will need a non-trivial rebase after
> we worked upon the queues recently).  Considering that, I don't really
> see much help on removing that layer.  Or, do we have any other reason
> to remove the response queue that I'm not aware of?

I think we first need to figure out where we want to go.  That includes
how to provide flow control.  Then we can decide how far to go in 3.0,
and whether we need to detour.

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Peter Xu]

On Fri, Jul 06, 2018 at 03:19:56PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Fri, Jul 06, 2018 at 10:09:58AM +0200, Markus Armbruster wrote:
> >> If I understand the code correctly, the response queue
> >> mon->qmp..qmp_requests gets drained into the output buffer mon->outbuf
> >> by bottom half monitor_qmp_bh_responder().  The response queue remains
> >> small, it's the output buffer that grows without bounds.  Your test for
> >> "limit exceeded" measures the response queue.  It needs to measure the
> >> output buffer instead.
> >> 
> >> We could drain the response queue only when the output buffer isn't too
> >> full.  I think that makes sense only if we have a compelling use for
> >> keeping responses in QDict form for a longer time.
> >
> > Indeed.  I didn't really notice that we're having an unlimited out_buf
> > there.
> >
> > To double confirm it's working, I firstly added some tracepoints:
> >
> > =============
> > diff --git a/monitor.c b/monitor.c
> > index 9eb9f06599..18ab609eab 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -375,6 +375,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
> >      while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
> >          qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
> >      }
> > +    trace_monitor_qmp_request_queue(mon, 0);
> >  }
> >
> >  /* Caller must hold the mon->qmp.qmp_lock */
> > @@ -383,6 +384,7 @@ static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
> >      while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
> >          qobject_unref((QDict *)g_queue_pop_head(mon->qmp.qmp_responses));
> >      }
> > +    trace_monitor_qmp_response_queue(mon, 0);
> >  }
> >
> >  static void monitor_qmp_cleanup_queues(Monitor *mon)
> > @@ -408,6 +410,7 @@ static void monitor_qmp_try_resume_locked(Monitor *mon)
> >      if (mon->qmp.need_resume) {
> >          monitor_resume(mon);
> >          mon->qmp.need_resume = false;
> > +        trace_monitor_qmp_resume(mon);
> >      }
> >  }
> >
> > @@ -555,6 +558,7 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
> >           */
> >          qemu_mutex_lock(&mon->qmp.qmp_lock);
> >          g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
> > +        trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
> >          qemu_mutex_unlock(&mon->qmp.qmp_lock);
> >          qemu_bh_schedule(qmp_respond_bh);
> >      } else {
> > @@ -578,6 +582,7 @@ static QDict *monitor_qmp_response_pop_one(Monitor *mon)
> >
> >      qemu_mutex_lock(&mon->qmp.qmp_lock);
> >      data = g_queue_pop_head(mon->qmp.qmp_responses);
> > +    trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
> >      /* In case if we were suspended due to response queue full */
> >      monitor_qmp_try_resume_locked(mon);
> >      qemu_mutex_unlock(&mon->qmp.qmp_lock);
> > @@ -4183,6 +4188,7 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
> >      QTAILQ_FOREACH(mon, &mon_list, entry) {
> >          qemu_mutex_lock(&mon->qmp.qmp_lock);
> >          req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
> > +        trace_monitor_qmp_request_queue(mon, mon->qmp.qmp_requests->length);
> >          qemu_mutex_unlock(&mon->qmp.qmp_lock);
> >          if (req_obj) {
> >              break;
> > @@ -4239,6 +4245,7 @@ static void monitor_qmp_suspend_locked(Monitor *mon)
> >      assert(mon->qmp.need_resume == false);
> >      monitor_suspend(mon);
> >      mon->qmp.need_resume = true;
> > +    trace_monitor_qmp_suspend(mon);
> >  }
> >
> >  static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > @@ -4312,6 +4319,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >       * etc. will be delivered to the handler side.
> >       */
> >      g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
> > +    trace_monitor_qmp_request_queue(mon, mon->qmp.qmp_requests->length);
> >      qemu_mutex_unlock(&mon->qmp.qmp_lock);
> >
> >      /* Kick the dispatcher routine */
> > diff --git a/trace-events b/trace-events
> > index c445f54773..bd9dade938 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -50,6 +50,10 @@ handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
> >  monitor_suspend(void *ptr, int cnt) "mon %p: %d"
> >  monitor_qmp_cmd_in_band(const char *id) "%s"
> >  monitor_qmp_cmd_out_of_band(const char *id) "%s"
> > +monitor_qmp_suspend(void *mon) "mon=%p"
> > +monitor_qmp_resume(void *mon) "mon=%p"
> > +monitor_qmp_request_queue(void *mon, int len) "mon=%p len=%d"
> > +monitor_qmp_response_queue(void *mon, int len) "mon=%p len=%d"
> >
> >  # dma-helpers.c
> >  dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
> > =============
> >
> > Then, I explicitly hanged the response BH by:
> >
> > =============
> > diff --git a/monitor.c b/monitor.c
> > index 18ab609eab..1f38084a4c 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -560,7 +560,7 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)
> >          g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(rsp));
> >          trace_monitor_qmp_response_queue(mon, mon->qmp.qmp_responses->length);
> >          qemu_mutex_unlock(&mon->qmp.qmp_lock);
> > -        qemu_bh_schedule(qmp_respond_bh);
> > +        //qemu_bh_schedule(qmp_respond_bh);
> >      } else {
> >          /*
> >           * Not using monitor I/O thread, i.e. we are in the main thread.
> 
> This stops draining of the response queue into the output buffer.
> 
> > =============
> >
> > Then this is what I got for command:
> >
> > $ cat cmd_list | qemu-system-x86_64 -qmp stdio -nographic -nodefaults -trace enable="monitor_qmp*"
> >
> > Result:
> >
> > 9815@1530867603.464498:monitor_qmp_response_queue mon=0x55de6bfc4800 len=1
> > 9815@1530867603.464782:monitor_qmp_suspend mon=0x55de6bfc4800
> > 9815@1530867603.464788:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> > 9815@1530867603.489017:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489030:monitor_qmp_cmd_in_band
> > 9815@1530867603.489050:monitor_qmp_response_queue mon=0x55de6bfc4800 len=2
> > 9815@1530867603.489057:monitor_qmp_resume mon=0x55de6bfc4800         <------- [0]
> > 9815@1530867603.489099:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489205:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> > 9815@1530867603.489311:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489317:monitor_qmp_cmd_in_band
> > 9815@1530867603.489329:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> > 9815@1530867603.489419:monitor_qmp_response_queue mon=0x55de6bfc4800 len=3
> > 9815@1530867603.489432:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489437:monitor_qmp_cmd_in_band
> > 9815@1530867603.489450:monitor_qmp_response_queue mon=0x55de6bfc4800 len=4
> > 9815@1530867603.489459:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489465:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> > 9815@1530867603.489468:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489471:monitor_qmp_cmd_in_band
> > 9815@1530867603.489484:monitor_qmp_response_queue mon=0x55de6bfc4800 len=5
> > 9815@1530867603.489492:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489575:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> > 9815@1530867603.489584:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489595:monitor_qmp_cmd_in_band
> > 9815@1530867603.489614:monitor_qmp_response_queue mon=0x55de6bfc4800 len=6
> > 9815@1530867603.489624:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489693:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> > 9815@1530867603.489703:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489708:monitor_qmp_cmd_in_band
> > 9815@1530867603.489725:monitor_qmp_response_queue mon=0x55de6bfc4800 len=7
> > 9815@1530867603.489736:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489818:monitor_qmp_suspend mon=0x55de6bfc4800  <-------------[1]
> > 9815@1530867603.489823:monitor_qmp_request_queue mon=0x55de6bfc4800 len=1
> > 9815@1530867603.489837:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > 9815@1530867603.489847:monitor_qmp_cmd_in_band
> > 9815@1530867603.489874:monitor_qmp_response_queue mon=0x55de6bfc4800 len=8
> > 9815@1530867603.489892:monitor_qmp_request_queue mon=0x55de6bfc4800 len=0
> > [hangs here]
> >
> > I think we have [1] right after response queue reaches N-1, I suppose
> > that means current patch is working.  Note that the suspend at [0] is
> > only the first "qmp_capability" command, since before we enable OOB
> > we'll still emulate the old monitor so that's by design.
> 
> Your experiment shows the patch succeeds at limiting the response queue
> length.  However, the "response queue mon->qmp..qmp_requests gets
> drained into the output buffer mon->outbuf by bottom half
> monitor_qmp_bh_responder().  The response queue remains small, it's the
> output buffer that grows without bounds".
> 
> To demonstrate that, let me add another tracepoint:
> 
> diff --git a/monitor.c b/monitor.c
> index 3bea239bc7..e35e15b43e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -451,6 +451,7 @@ static void monitor_flush_locked(Monitor *mon)
>  
>      buf = qstring_get_str(mon->outbuf);
>      len = qstring_get_length(mon->outbuf);
> +    trace_monitor_flush_locked(mon, len);
>  
>      if (len && !mon->mux_out) {
>          rc = qemu_chr_fe_write(&mon->chr, (const uint8_t *) buf, len);
> diff --git a/trace-events b/trace-events
> index bd9dade938..b6d5d28041 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -54,6 +54,7 @@ monitor_qmp_suspend(void *mon) "mon=%p"
>  monitor_qmp_resume(void *mon) "mon=%p"
>  monitor_qmp_request_queue(void *mon, int len) "mon=%p len=%d"
>  monitor_qmp_response_queue(void *mon, int len) "mon=%p len=%d"
> +monitor_flush_locked(void *mon, int len) "mon=%p len=%d"
>  
>  # dma-helpers.c
>  dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
> 
> Start this QEMU:
> 
>     $ upstream-qemu -chardev socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp -display none -nodefaults -trace enable="monitor_qmp*" -trace enable=monitor_flush_locked
>     3236@1530882919.092979:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882919.093018:monitor_qmp_response_queue mon=0x56192215baf0 len=2
>     3236@1530882919.093076:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882919.093095:monitor_flush_locked mon=0x56192215baf0 len=107
>     3236@1530882919.093103:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882919.093112:monitor_flush_locked mon=0x56192215baf0 len=82
>     3236@1530882919.093118:monitor_qmp_response_queue mon=0x56192215baf0 len=0
> 
> Now let me feed a couple of commands to that without reading any of
> their responses.  To do that, run in another terminal:
> 
>     $ socat -u STDIO UNIX:test-qmp
>     { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
> 
> Note that -u makes socat use the first address (STDIO) only for reading,
> and the second one (the QMP socket) only for writing.  This QMP client
> fails to read any responses.

This is useful. :)

> QEMU prints:
> 
>     3236@1530882921.894996:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882921.895049:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882921.895171:monitor_flush_locked mon=0x56192215baf0 len=136
>     3236@1530882921.895221:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882923.954233:monitor_qmp_suspend mon=0x56192215baf0
>     3236@1530882923.954262:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>     3236@1530882923.954314:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882923.954341:monitor_qmp_cmd_in_band 
>     3236@1530882923.954407:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882923.954430:monitor_qmp_resume mon=0x56192215baf0
>     3236@1530882923.954476:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882923.954501:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882923.954548:monitor_flush_locked mon=0x56192215baf0 len=16
>     3236@1530882923.954587:monitor_qmp_response_queue mon=0x56192215baf0 len=0
> 
> Back in socat, execute a few commands, one after the other, with short
> pauses in between:
> 
>     { "execute": "query-qmp-schema" }
>     { "execute": "query-qmp-schema" }
>     { "execute": "query-qmp-schema" }
>     { "execute": "query-qmp-schema" }
>     { "execute": "query-qmp-schema" }
>     { "execute": "quit" }
> 
> QEMU prints:
> 
>     3236@1530882931.529666:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>     3236@1530882931.529768:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882931.529799:monitor_qmp_cmd_in_band 
>     3236@1530882931.537403:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882931.537474:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882931.537481:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882931.544809:monitor_flush_locked mon=0x56192215baf0 len=129073
>     3236@1530882931.548465:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882935.345300:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>     3236@1530882935.345399:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882935.345431:monitor_qmp_cmd_in_band 
>     3236@1530882935.355347:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882935.355453:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882935.355513:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882935.366228:monitor_flush_locked mon=0x56192215baf0 len=129073
>     3236@1530882935.369826:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882936.985228:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>     3236@1530882936.985338:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882936.985371:monitor_qmp_cmd_in_band 
>     3236@1530882936.991813:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882936.991893:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882936.991900:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882936.999026:monitor_flush_locked mon=0x56192215baf0 len=148514
>     3236@1530882937.002865:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882937.905712:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>     3236@1530882937.905873:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882937.905898:monitor_qmp_cmd_in_band 
>     3236@1530882937.911889:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882937.911958:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882937.911964:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882937.919079:monitor_flush_locked mon=0x56192215baf0 len=277587
>     3236@1530882937.922546:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882939.313235:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>     3236@1530882939.313551:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882939.313586:monitor_qmp_cmd_in_band 
>     3236@1530882939.323542:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882939.323619:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882939.323651:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882939.335255:monitor_flush_locked mon=0x56192215baf0 len=406660
>     3236@1530882939.338829:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882945.161013:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>     3236@1530882945.161122:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882945.161153:monitor_qmp_cmd_in_band 
>     3236@1530882945.163191:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882945.163283:monitor_qmp_response_queue mon=0x56192215baf0 len=2
>     3236@1530882945.163360:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>     3236@1530882945.163407:monitor_flush_locked mon=0x56192215baf0 len=406676
>     3236@1530882945.163439:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882945.163489:monitor_flush_locked mon=0x56192215baf0 len=406787
>     3236@1530882945.163518:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882945.163695:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>     3236@1530882945.163725:monitor_flush_locked mon=0x56192215baf0 len=406787
>     3236@1530882945.163866:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>     3236@1530882945.163879:monitor_qmp_response_queue mon=0x56192215baf0 len=0
> 
> The response queue length stays small (the bottom half drains it just
> fine), but the output buffer keeps growing.

Yes, I think I understood this part.  Indeed we have an unbound output
buffer, I think we should fix it somehow, though for now I am not sure
that'll be a trivial fix for 3.0.  What I wanted to demostrate is that
the queue limitation is working, so basically we should fix both
places at last, and this patch fixes the first problem.

After all IIUC this output buffer problem exists even before the
out-of-band work, and it's there for at least years.

> 
> >> Marc-André has a patch to cut out the response queue.  Whether it still
> >> makes sense I don't know.
> >> [PATCH v3 03/38] Revert "qmp: isolate responses into io thread"
> >
> > I think we discussed this before, I agree with Marc-Andre that most of
> > the codes that are related to response flushing should be thread safe
> > now.  Actually it was not when we started to draft the out-of-band
> > thing, and that's the major reason why I refactored my old code to
> > explicitly separate the dispatcher and the responder, so that
> > responder can be isolated and be together with the parser.
> >
> > I don't have obvious reason to remove this layer of isolation if it
> > works well (and actually the code is not that much, and IMHO the
> > response queue is a clear interface that maybe we can use in the
> > future too), I think my major concern now is that we're in rc phase
> > for QEMU-3.0 so that it at least seems to be a big change to monitor
> > layer.  We might consider to revert back to no-responder way if we
> > really want, but I guess we'd better do as much testing as when we
> > started to play with out-of-band to make sure we won't miss anything
> > important (and I guess the patch will need a non-trivial rebase after
> > we worked upon the queues recently).  Considering that, I don't really
> > see much help on removing that layer.  Or, do we have any other reason
> > to remove the response queue that I'm not aware of?
> 
> I think we first need to figure out where we want to go.  That includes
> how to provide flow control.  Then we can decide how far to go in 3.0,
> and whether we need to detour.

For my own opinion, I'll see it a bit awkward (as I mentioned) to
revert the response queue part.  Again, it's mostly working well
there, we just fix things up when needed.  It's not really a big part
of code to maintain, and it still looks sane to me to have such an
isolation so that we can have the dispatcher totally separate from the
chardev IO path (I'd say if I design a QMP interface from scratch,
I'll do that from the first day).  So I am not against reverting that
part at all, I just don't see much gain from that as well, especially
if we want to make QMP more modular in the future.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jul 06, 2018 at 03:19:56PM +0200, Markus Armbruster wrote:
[...]
>> Your experiment shows the patch succeeds at limiting the response queue
>> length.  However, the "response queue mon->qmp..qmp_requests gets
>> drained into the output buffer mon->outbuf by bottom half
>> monitor_qmp_bh_responder().  The response queue remains small, it's the
>> output buffer that grows without bounds".
>> 
>> To demonstrate that, let me add another tracepoint:
>> 
>> diff --git a/monitor.c b/monitor.c
>> index 3bea239bc7..e35e15b43e 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -451,6 +451,7 @@ static void monitor_flush_locked(Monitor *mon)
>>  
>>      buf = qstring_get_str(mon->outbuf);
>>      len = qstring_get_length(mon->outbuf);
>> +    trace_monitor_flush_locked(mon, len);
>>  
>>      if (len && !mon->mux_out) {
>>          rc = qemu_chr_fe_write(&mon->chr, (const uint8_t *) buf, len);
>> diff --git a/trace-events b/trace-events
>> index bd9dade938..b6d5d28041 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -54,6 +54,7 @@ monitor_qmp_suspend(void *mon) "mon=%p"
>>  monitor_qmp_resume(void *mon) "mon=%p"
>>  monitor_qmp_request_queue(void *mon, int len) "mon=%p len=%d"
>>  monitor_qmp_response_queue(void *mon, int len) "mon=%p len=%d"
>> +monitor_flush_locked(void *mon, int len) "mon=%p len=%d"
>>  
>>  # dma-helpers.c
>>  dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
>> 
>> Start this QEMU:
>> 
>>     $ upstream-qemu -chardev socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp -display none -nodefaults -trace enable="monitor_qmp*" -trace enable=monitor_flush_locked
>>     3236@1530882919.092979:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882919.093018:monitor_qmp_response_queue mon=0x56192215baf0 len=2
>>     3236@1530882919.093076:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882919.093095:monitor_flush_locked mon=0x56192215baf0 len=107
>>     3236@1530882919.093103:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882919.093112:monitor_flush_locked mon=0x56192215baf0 len=82
>>     3236@1530882919.093118:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>> 
>> Now let me feed a couple of commands to that without reading any of
>> their responses.  To do that, run in another terminal:
>> 
>>     $ socat -u STDIO UNIX:test-qmp
>>     { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
>> 
>> Note that -u makes socat use the first address (STDIO) only for reading,
>> and the second one (the QMP socket) only for writing.  This QMP client
>> fails to read any responses.
>
> This is useful. :)

socat is one of my favorites :)

>> QEMU prints:
>> 
>>     3236@1530882921.894996:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882921.895049:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882921.895171:monitor_flush_locked mon=0x56192215baf0 len=136
>>     3236@1530882921.895221:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882923.954233:monitor_qmp_suspend mon=0x56192215baf0
>>     3236@1530882923.954262:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>>     3236@1530882923.954314:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882923.954341:monitor_qmp_cmd_in_band 
>>     3236@1530882923.954407:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882923.954430:monitor_qmp_resume mon=0x56192215baf0
>>     3236@1530882923.954476:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882923.954501:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882923.954548:monitor_flush_locked mon=0x56192215baf0 len=16
>>     3236@1530882923.954587:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>> 
>> Back in socat, execute a few commands, one after the other, with short
>> pauses in between:
>> 
>>     { "execute": "query-qmp-schema" }
>>     { "execute": "query-qmp-schema" }
>>     { "execute": "query-qmp-schema" }
>>     { "execute": "query-qmp-schema" }
>>     { "execute": "query-qmp-schema" }
>>     { "execute": "quit" }
>> 
>> QEMU prints:
>> 
>>     3236@1530882931.529666:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>>     3236@1530882931.529768:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882931.529799:monitor_qmp_cmd_in_band 
>>     3236@1530882931.537403:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882931.537474:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882931.537481:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882931.544809:monitor_flush_locked mon=0x56192215baf0 len=129073
>>     3236@1530882931.548465:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882935.345300:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>>     3236@1530882935.345399:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882935.345431:monitor_qmp_cmd_in_band 
>>     3236@1530882935.355347:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882935.355453:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882935.355513:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882935.366228:monitor_flush_locked mon=0x56192215baf0 len=129073
>>     3236@1530882935.369826:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882936.985228:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>>     3236@1530882936.985338:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882936.985371:monitor_qmp_cmd_in_band 
>>     3236@1530882936.991813:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882936.991893:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882936.991900:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882936.999026:monitor_flush_locked mon=0x56192215baf0 len=148514
>>     3236@1530882937.002865:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882937.905712:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>>     3236@1530882937.905873:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882937.905898:monitor_qmp_cmd_in_band 
>>     3236@1530882937.911889:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882937.911958:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882937.911964:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882937.919079:monitor_flush_locked mon=0x56192215baf0 len=277587
>>     3236@1530882937.922546:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882939.313235:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>>     3236@1530882939.313551:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882939.313586:monitor_qmp_cmd_in_band 
>>     3236@1530882939.323542:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882939.323619:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882939.323651:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882939.335255:monitor_flush_locked mon=0x56192215baf0 len=406660
>>     3236@1530882939.338829:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882945.161013:monitor_qmp_request_queue mon=0x56192215baf0 len=1
>>     3236@1530882945.161122:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882945.161153:monitor_qmp_cmd_in_band 
>>     3236@1530882945.163191:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882945.163283:monitor_qmp_response_queue mon=0x56192215baf0 len=2
>>     3236@1530882945.163360:monitor_qmp_response_queue mon=0x56192215baf0 len=1
>>     3236@1530882945.163407:monitor_flush_locked mon=0x56192215baf0 len=406676
>>     3236@1530882945.163439:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882945.163489:monitor_flush_locked mon=0x56192215baf0 len=406787
>>     3236@1530882945.163518:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882945.163695:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>>     3236@1530882945.163725:monitor_flush_locked mon=0x56192215baf0 len=406787
>>     3236@1530882945.163866:monitor_qmp_request_queue mon=0x56192215baf0 len=0
>>     3236@1530882945.163879:monitor_qmp_response_queue mon=0x56192215baf0 len=0
>> 
>> The response queue length stays small (the bottom half drains it just
>> fine), but the output buffer keeps growing.
>
> Yes, I think I understood this part.  Indeed we have an unbound output
> buffer, I think we should fix it somehow, though for now I am not sure
> that'll be a trivial fix for 3.0.  What I wanted to demostrate is that
> the queue limitation is working, so basically we should fix both
> places at last, and this patch fixes the first problem.

The response queue can only grow if the bottom half doesn't run in a
timely manner.  But if bottom halves don't run in a timely manner, we
likely got much bigger problems than queue growth.

> After all IIUC this output buffer problem exists even before the
> out-of-band work, and it's there for at least years.

I agree it's not a blocker bug for OOB.  I'd be fine with a FIXME
comment.

>> >> Marc-André has a patch to cut out the response queue.  Whether it still
>> >> makes sense I don't know.
>> >> [PATCH v3 03/38] Revert "qmp: isolate responses into io thread"
>> >
>> > I think we discussed this before, I agree with Marc-Andre that most of
>> > the codes that are related to response flushing should be thread safe
>> > now.  Actually it was not when we started to draft the out-of-band
>> > thing, and that's the major reason why I refactored my old code to
>> > explicitly separate the dispatcher and the responder, so that
>> > responder can be isolated and be together with the parser.
>> >
>> > I don't have obvious reason to remove this layer of isolation if it
>> > works well (and actually the code is not that much, and IMHO the
>> > response queue is a clear interface that maybe we can use in the
>> > future too), I think my major concern now is that we're in rc phase
>> > for QEMU-3.0 so that it at least seems to be a big change to monitor
>> > layer.  We might consider to revert back to no-responder way if we
>> > really want, but I guess we'd better do as much testing as when we
>> > started to play with out-of-band to make sure we won't miss anything
>> > important (and I guess the patch will need a non-trivial rebase after
>> > we worked upon the queues recently).  Considering that, I don't really
>> > see much help on removing that layer.  Or, do we have any other reason
>> > to remove the response queue that I'm not aware of?
>> 
>> I think we first need to figure out where we want to go.  That includes
>> how to provide flow control.  Then we can decide how far to go in 3.0,
>> and whether we need to detour.
>
> For my own opinion, I'll see it a bit awkward (as I mentioned) to
> revert the response queue part.  Again, it's mostly working well
> there, we just fix things up when needed.  It's not really a big part
> of code to maintain, and it still looks sane to me to have such an
> isolation so that we can have the dispatcher totally separate from the
> chardev IO path (I'd say if I design a QMP interface from scratch,
> I'll do that from the first day).  So I am not against reverting that
> part at all, I just don't see much gain from that as well, especially
> if we want to make QMP more modular in the future.

Since it's not broken, there's no need to mess with it for 3.0, and no
need to have it block OOB.

Marc-André, I trust you'll respin the monitor core parts of "[PATCH v3
00/38] RFC: monitor: add asynchronous command type" as a separate series
for 3.1.  Please include a patch to rip out the response queue if you
think it's a worthwhile simplification.  Our discussion of the response
queue will then be grounded by working patches.

Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> On Fri, Jul 06, 2018 at 03:19:56PM +0200, Markus Armbruster wrote:
[...]
> Marc-André, I trust you'll respin the monitor core parts of "[PATCH v3
> 00/38] RFC: monitor: add asynchronous command type" as a separate series
> for 3.1.  Please include a patch to rip out the response queue if you
> think it's a worthwhile simplification.  Our discussion of the response
> queue will then be grounded by working patches.

I've since (re-)found that respin in my review queue.  I even reviewed
the patch that rips out the response queue.

[Qemu-devel] [PATCH 6/9] qapi: remove COMMAND_DROPPED event

[Peter Xu]

Now it was not used any more.  Drop it, especially if we can do that
before we release QEMU 3.0.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qapi/misc.json | 40 ----------------------------------------
 1 file changed, 40 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index f1860418e8..ceda21746b 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3432,46 +3432,6 @@
 ##
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability' }
 
-##
-# @CommandDropReason:
-#
-# Reasons that caused one command to be dropped.
-#
-# @queue-full: the command queue is full. This can only occur when
-#              the client sends a new non-oob command before the
-#              response to the previous non-oob command has been
-#              received.
-#
-# Since: 2.12
-##
-{ 'enum': 'CommandDropReason',
-  'data': [ 'queue-full' ] }
-
-##
-# @COMMAND_DROPPED:
-#
-# Emitted when a command is dropped due to some reason.  Commands can
-# only be dropped when the oob capability is enabled.
-#
-# @id: The dropped command's "id" field.
-# FIXME Broken by design.  Events are broadcast to all monitors.  If
-# another monitor's client has a command with the same ID in flight,
-# the event will incorrectly claim that command was dropped.
-#
-# @reason: The reason why the command is dropped.
-#
-# Since: 2.12
-#
-# Example:
-#
-# { "event": "COMMAND_DROPPED",
-#   "data": {"result": {"id": "libvirt-102",
-#                       "reason": "queue-full" } } }
-#
-##
-{ 'event': 'COMMAND_DROPPED' ,
-  'data': { 'id': 'any', 'reason': 'CommandDropReason' } }
-
 ##
 # @set-numa-node:
 #
-- 
2.17.1

[Qemu-devel] [PATCH 7/9] monitor: restrict response queue length too

[Peter Xu]

Before this patch we were only monitoring the request queue, but it's
still possible that a client only sends requests but it never eats any
reply from us.  In that case our response queue might grow with
unlimited responses and put us at risk.

Now we play the similar trick as we have done to the request queue to
make sure we apply the same queue length rule to the response queue as
well.  Then we also need to peek at the queue length after we unqueue a
response now, to make sure we'll kick the monitor to alive if it was
suspended due to "response queue full".

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index ebf862914f..9b78cf1c63 100644
--- a/monitor.c
+++ b/monitor.c
@@ -393,18 +393,15 @@ static void monitor_qmp_cleanup_queues(Monitor *mon)
     qemu_mutex_unlock(&mon->qmp.qmp_lock);
 }
 
-/* Try to resume the monitor if it was suspended due to any reason */
-static void monitor_qmp_try_resume(Monitor *mon)
+/* Callers must be with Monitor.qmp.qmp_lock held. */
+static void monitor_qmp_try_resume_locked(Monitor *mon)
 {
-    assert(monitor_is_qmp(mon));
-    qemu_mutex_lock(&mon->qmp.qmp_lock);
-
-    if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
+    if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX ||
+        mon->qmp.qmp_responses->length >= QMP_REQ_QUEUE_LEN_MAX) {
         /*
          * This should not happen, but in case if it happens, we
          * should still keep the monitor in suspend state
          */
-        qemu_mutex_unlock(&mon->qmp.qmp_lock);
         return;
     }
 
@@ -412,7 +409,14 @@ static void monitor_qmp_try_resume(Monitor *mon)
         monitor_resume(mon);
         mon->qmp.need_resume = false;
     }
+}
 
+/* Try to resume the monitor if it was suspended due to any reason */
+static void monitor_qmp_try_resume(Monitor *mon)
+{
+    assert(monitor_is_qmp(mon));
+    qemu_mutex_lock(&mon->qmp.qmp_lock);
+    monitor_qmp_try_resume_locked(mon);
     qemu_mutex_unlock(&mon->qmp.qmp_lock);
 }
 
@@ -574,6 +578,8 @@ static QDict *monitor_qmp_response_pop_one(Monitor *mon)
 
     qemu_mutex_lock(&mon->qmp.qmp_lock);
     data = g_queue_pop_head(mon->qmp.qmp_responses);
+    /* In case if we were suspended due to response queue full */
+    monitor_qmp_try_resume_locked(mon);
     qemu_mutex_unlock(&mon->qmp.qmp_lock);
 
     return data;
@@ -4289,12 +4295,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         monitor_qmp_suspend_locked(mon);
     } else {
         /*
-         * If the queue is reaching the length limitation, we queue
-         * this command, meanwhile we suspend the monitor to block new
-         * commands.  We'll resume ourselves until the queue has more
-         * space.
+         * If any of the req/resp queue is reaching the length
+         * limitation, we queue this command, meanwhile we suspend the
+         * monitor to block new commands.  We'll resume ourselves
+         * until both of the queues have more spaces.
          */
-        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX - 1) {
+        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX - 1 ||
+            mon->qmp.qmp_responses->length >= QMP_REQ_QUEUE_LEN_MAX - 1) {
             monitor_qmp_suspend_locked(mon);
         }
     }
-- 
2.17.1

[Qemu-devel] [PATCH 8/9] monitor: remove "x-oob", turn oob on by default

[Peter Xu]

OOB commands were introduced in commit cf869d53172.  Unfortunately, we
ran into a regression, and had to disable them by default for 2.12
(commit be933ffc23).

  http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html

The regression has since been fixed (commit 951702f39c7 "monitor: bind
dispatch bh to iohandler context").  Time to re-enable OOB.

This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"),
and turns it on again for non-MUX QMPs.  Note that we can't enable
Out-Of-Band for monitors with MUX-typed chardev backends, because not
all the chardev frontends can run without main thread, or can run in
multiple threads.

Some trivial touch-up in the test code is required to make sure qmp-test
won't break.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/monitor/monitor.h |  1 -
 monitor.c                 | 22 ++++++----------------
 tests/libqtest.c          |  2 +-
 tests/qmp-test.c          |  2 +-
 vl.c                      |  5 -----
 5 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index d6ab70cae2..0cb0538a31 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -13,7 +13,6 @@ extern Monitor *cur_mon;
 #define MONITOR_USE_READLINE  0x02
 #define MONITOR_USE_CONTROL   0x04
 #define MONITOR_USE_PRETTY    0x08
-#define MONITOR_USE_OOB       0x10
 
 bool monitor_cur_is_qmp(void);
 
diff --git a/monitor.c b/monitor.c
index 9b78cf1c63..9eb9f06599 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4626,19 +4626,12 @@ void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
     bool use_readline = flags & MONITOR_USE_READLINE;
-    bool use_oob = flags & MONITOR_USE_OOB;
-
-    if (use_oob) {
-        if (CHARDEV_IS_MUX(chr)) {
-            error_report("Monitor out-of-band is not supported with "
-                         "MUX typed chardev backend");
-            exit(1);
-        }
-        if (use_readline) {
-            error_report("Monitor out-of-band is only supported by QMP");
-            exit(1);
-        }
-    }
+    /*
+     * Note: we can't enable Out-Of-Band for monitors with MUX-typed
+     * chardev backends, because not all the chardev frontends can run
+     * without main thread, or can run in multiple threads.
+     */
+    bool use_oob = (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr);
 
     monitor_data_init(mon, false, use_oob);
 
@@ -4735,9 +4728,6 @@ QemuOptsList qemu_mon_opts = {
         },{
             .name = "pretty",
             .type = QEMU_OPT_BOOL,
-        },{
-            .name = "x-oob",
-            .type = QEMU_OPT_BOOL,
         },
         { /* end of list */ }
     },
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 098af6aec4..c5cb3f925c 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -213,7 +213,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
                                   "-display none "
                                   "%s", qemu_binary, socket_path,
                                   getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
-                                  qmp_socket_path, use_oob ? ",x-oob=on" : "",
+                                  qmp_socket_path, "",
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
         exit(1);
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 57c1296573..033d610b0e 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -89,7 +89,7 @@ static void test_qmp_protocol(void)
     g_assert(q);
     test_version(qdict_get(q, "version"));
     capabilities = qdict_get_qlist(q, "capabilities");
-    g_assert(capabilities && qlist_empty(capabilities));
+    g_assert(capabilities);
     qobject_unref(resp);
 
     /* Test valid command before handshake */
diff --git a/vl.c b/vl.c
index 29b1830259..e087cc1b72 100644
--- a/vl.c
+++ b/vl.c
@@ -2322,11 +2322,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
     if (qemu_opt_get_bool(opts, "pretty", 0))
         flags |= MONITOR_USE_PRETTY;
 
-    /* OOB is off by default */
-    if (qemu_opt_get_bool(opts, "x-oob", 0)) {
-        flags |= MONITOR_USE_OOB;
-    }
-
     chardev = qemu_opt_get(opts, "chardev");
     chr = qemu_chr_find(chardev);
     if (chr == NULL) {
-- 
2.17.1

[Qemu-devel] [PATCH 9/9] Revert "tests: Add parameter to qtest_init_without_qmp_handshake"

[Peter Xu]

This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a.

Meanwhile, revert one line from fa198ad9bdef to make sure
qtest_init_without_qmp_handshake() will only pass in one parameter.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/libqtest.h |  4 +---
 tests/libqtest.c | 10 ++++------
 tests/qmp-test.c |  4 ++--
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index ac52872cbe..180d2cc6ff 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -56,14 +56,12 @@ QTestState *qtest_init(const char *extra_args);
 
 /**
  * qtest_init_without_qmp_handshake:
- * @use_oob: true to have the server advertise OOB support
  * @extra_args: other arguments to pass to QEMU.  CAUTION: these
  * arguments are subject to word splitting and shell evaluation.
  *
  * Returns: #QTestState instance.
  */
-QTestState *qtest_init_without_qmp_handshake(bool use_oob,
-                                             const char *extra_args);
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
 
 /**
  * qtest_quit:
diff --git a/tests/libqtest.c b/tests/libqtest.c
index c5cb3f925c..51af4a289e 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -173,8 +173,7 @@ static const char *qtest_qemu_binary(void)
     return qemu_bin;
 }
 
-QTestState *qtest_init_without_qmp_handshake(bool use_oob,
-                                             const char *extra_args)
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, i;
@@ -207,13 +206,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
         command = g_strdup_printf("exec %s "
                                   "-qtest unix:%s,nowait "
                                   "-qtest-log %s "
-                                  "-chardev socket,path=%s,nowait,id=char0 "
-                                  "-mon chardev=char0,mode=control%s "
+                                  "-qmp unix:%s,nowait "
                                   "-machine accel=qtest "
                                   "-display none "
                                   "%s", qemu_binary, socket_path,
                                   getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
-                                  qmp_socket_path, "",
+                                  qmp_socket_path,
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
         exit(1);
@@ -248,7 +246,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
 
 QTestState *qtest_init(const char *extra_args)
 {
-    QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
+    QTestState *s = qtest_init_without_qmp_handshake(extra_args);
 
     /* Read the QMP greeting and then do the handshake */
     qtest_qmp_discard_response(s, "");
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 033d610b0e..bd7118a5a9 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -81,7 +81,7 @@ static void test_qmp_protocol(void)
     QList *capabilities;
     QTestState *qts;
 
-    qts = qtest_init_without_qmp_handshake(false, common_args);
+    qts = qtest_init_without_qmp_handshake(common_args);
 
     /* Test greeting */
     resp = qtest_qmp_receive(qts);
@@ -196,7 +196,7 @@ static void test_qmp_oob(void)
     QList *capabilities;
     QString *qstr;
 
-    qts = qtest_init_without_qmp_handshake(true, common_args);
+    qts = qtest_init_without_qmp_handshake(common_args);
 
     /* Check the greeting message. */
     resp = qtest_qmp_receive(qts);
-- 
2.17.1

Re: [Qemu-devel] [PATCH 0/9] monitor: enable OOB by default

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Based-on: <20180703085358.13941-1-armbru@redhat.com>

Now in master.

> This work is based on Markus's latest out-of-band fixes:
>   "[PATCH v2 00/32] ] qmp: Fixes and cleanups around OOB commands"
>
> Major stuff: some cleanups that were previously suggested by Markus or
> Eric.  Meanwhile fix up the flow control issue.  Since my proposal
> here is to drop COMMAND_DROPPED event, then we don't need to introduce
> per-monitor event emission API any more.  Though Markus told me that
> he might use that code somewhere else, so I'll post that per-monitor
> event code out separately as RFC later.
>
> Patch 1-3: cleanups.

I expect these to be ready in the next version.

Since it's just cleanup, there's no need to rush these into 3.0 unless
they enable something we want in 3.0.

> Patch 4-7: solve the flow control issue reported by Markus that
> response queue might overflow even if we have limitation on the
> request queue.  Firstly we drop the COMMAND_DROP event since that
> won't work for response queue (since queuing an event will need to
> append to the response queue itself), then we use monitor suspend and
> resume to control the flow.  Last, we apply that to response queue
> too.

These need work.  We need to agree on how exactly flow control should
work.  Moreover, we need to reconcile your work with Marc-André's
"[PATCH 00/12] RFC: monitor: various code simplification and fixes"
(which I haven't fully reviewed yet).

> Patch 8-9: the original patches to enable out-of-band by default.

I figure these patches are good; we just need to decide whether we're
ready to enable OOB.  Let's review the known issues.

* We might want to make "id" mandatory with exec-oob

  Best to do that right in the first release that fully supports OOB.

* OOB offered but rejected by client is not obviously the same as
  OOB not offered

  I still need to digest and reply to your
  Message-ID: <20180629090115.GH2455@xz-mi>

* COMMAND_DROPPED is broken by design, and
* Flow control limits only request queue; response buffer can still
  grow without bounds

  You proposed to drop COMMAND_DROPPED, and do flow control by corking
  input, see above.

  Getting rid of broken COMMAND_DROPPED is a blocker.  Fully functional
  flow control isn't, since the QMP client is trusted.

* We lack test coverage for flow control
* test-qmp-cmds neglects to cover the OOB additions to qmp-dispatch.c

  I'm inclined to treating lack of test coverage as a blocker.

* scripts/qmp/ doesn't support OOB, yet.  qmp-shell.py in particular.

  Not a blocker.

Whatever we don't address right away we should at least mark FIXME in
the source code.

Assuming my list is complete, and my assessments correct, then we're
quite close to the point where we can enable OOB.  But since rc1 is
tomorrow already, I feel enabling it in 3.0 has become unrealistic.  I
understand we need it sooner rather than later for postcopy recovery.
However, the current state should be servicable for teaching OOB to
libvirt: just follow the recommendation to supply "id" (libvirt always
does anyway), pretend COMMAND_DROPPED doesn't exist, and pretend flow
control isn't an issue.

[...]

Re: [Qemu-devel] [PATCH 0/9] monitor: enable OOB by default

[Peter Xu]

On Mon, Jul 16, 2018 at 07:18:28PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Based-on: <20180703085358.13941-1-armbru@redhat.com>
> 
> Now in master.
> 
> > This work is based on Markus's latest out-of-band fixes:
> >   "[PATCH v2 00/32] ] qmp: Fixes and cleanups around OOB commands"
> >
> > Major stuff: some cleanups that were previously suggested by Markus or
> > Eric.  Meanwhile fix up the flow control issue.  Since my proposal
> > here is to drop COMMAND_DROPPED event, then we don't need to introduce
> > per-monitor event emission API any more.  Though Markus told me that
> > he might use that code somewhere else, so I'll post that per-monitor
> > event code out separately as RFC later.
> >
> > Patch 1-3: cleanups.
> 
> I expect these to be ready in the next version.
> 
> Since it's just cleanup, there's no need to rush these into 3.0 unless
> they enable something we want in 3.0.
> 
> > Patch 4-7: solve the flow control issue reported by Markus that
> > response queue might overflow even if we have limitation on the
> > request queue.  Firstly we drop the COMMAND_DROP event since that
> > won't work for response queue (since queuing an event will need to
> > append to the response queue itself), then we use monitor suspend and
> > resume to control the flow.  Last, we apply that to response queue
> > too.
> 
> These need work.  We need to agree on how exactly flow control should
> work.  Moreover, we need to reconcile your work with Marc-André's
> "[PATCH 00/12] RFC: monitor: various code simplification and fixes"
> (which I haven't fully reviewed yet).

Sure.

> 
> > Patch 8-9: the original patches to enable out-of-band by default.
> 
> I figure these patches are good; we just need to decide whether we're
> ready to enable OOB.  Let's review the known issues.
> 
> * We might want to make "id" mandatory with exec-oob
> 
>   Best to do that right in the first release that fully supports OOB.

Yeah, I'd say I would prefer it that way, but after all we're having
that optional now in master, so it's fine for me in either way.

> 
> * OOB offered but rejected by client is not obviously the same as
>   OOB not offered
> 
>   I still need to digest and reply to your
>   Message-ID: <20180629090115.GH2455@xz-mi>

As a summary of the reply - I think the only difference is where we
run the chardev IOs for the monitor:

- When OOB not offered: we run chardev IOs in main thread

- When OOB offered but client rejected: we run chardev IOs in iothread

The rest of the things should be all the same.

> 
> * COMMAND_DROPPED is broken by design, and
> * Flow control limits only request queue; response buffer can still
>   grow without bounds
> 
>   You proposed to drop COMMAND_DROPPED, and do flow control by corking
>   input, see above.
> 
>   Getting rid of broken COMMAND_DROPPED is a blocker.  Fully functional
>   flow control isn't, since the QMP client is trusted.
> 
> * We lack test coverage for flow control
> * test-qmp-cmds neglects to cover the OOB additions to qmp-dispatch.c
> 
>   I'm inclined to treating lack of test coverage as a blocker.

I will look at this one before repost after 3.0.

> 
> * scripts/qmp/ doesn't support OOB, yet.  qmp-shell.py in particular.
> 
>   Not a blocker.

I'll possibly temporarily put this one aside.  If not, I'll leave a
FIXME there (or I'll just do).

> 
> Whatever we don't address right away we should at least mark FIXME in
> the source code.
> 
> Assuming my list is complete, and my assessments correct, then we're
> quite close to the point where we can enable OOB.  But since rc1 is
> tomorrow already, I feel enabling it in 3.0 has become unrealistic.  I
> understand we need it sooner rather than later for postcopy recovery.
> However, the current state should be servicable for teaching OOB to
> libvirt: just follow the recommendation to supply "id" (libvirt always
> does anyway), pretend COMMAND_DROPPED doesn't exist, and pretend flow
> control isn't an issue.

I guess the thing is not about COMMAND_DROPPED; it's about we're going
to drop the x-oob parameter.  Now we can only use that parameter to
enable out-of-band, and after we enable it by default we possibly want
to remove that x-oob parameter.  So we'd better provide a constant way
for libvirt to enable out-of-band first (and now I'll assume the
"exec-oob" interface won't change again).  But yes I think we can do
that after 3.0.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/9] monitor: enable OOB by default

[Peter Xu]

On Tue, Jul 17, 2018 at 08:08:55PM +0800, Peter Xu wrote:

[...]

> > 
> > Whatever we don't address right away we should at least mark FIXME in
> > the source code.
> > 
> > Assuming my list is complete, and my assessments correct, then we're
> > quite close to the point where we can enable OOB.  But since rc1 is
> > tomorrow already, I feel enabling it in 3.0 has become unrealistic.  I
> > understand we need it sooner rather than later for postcopy recovery.
> > However, the current state should be servicable for teaching OOB to
> > libvirt: just follow the recommendation to supply "id" (libvirt always
> > does anyway), pretend COMMAND_DROPPED doesn't exist, and pretend flow
> > control isn't an issue.
> 
> I guess the thing is not about COMMAND_DROPPED; it's about we're going
> to drop the x-oob parameter.  Now we can only use that parameter to
> enable out-of-band, and after we enable it by default we possibly want
> to remove that x-oob parameter.  So we'd better provide a constant way
> for libvirt to enable out-of-band first (and now I'll assume the
> "exec-oob" interface won't change again).  But yes I think we can do
> that after 3.0.

Btw, note that patches 4-7 might still be candidates of bug fixes for
existing out-of-band feature.  It'll drop COMMAND_DROPPED event, but
IMHO it's still better than having a broken flow control for it (and
dropping event declaration is pretty safe even clients implemented
them).  Though the changes are not trivial, so I'll leave the decision
to you on whether we'll still consider them as 3.0 material.  Anyway,
please feel free to let me know if you want me to post them
separately.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/9] monitor: enable OOB by default

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Jul 17, 2018 at 08:08:55PM +0800, Peter Xu wrote:
>
> [...]
>
>> > 
>> > Whatever we don't address right away we should at least mark FIXME in
>> > the source code.
>> > 
>> > Assuming my list is complete, and my assessments correct, then we're
>> > quite close to the point where we can enable OOB.  But since rc1 is
>> > tomorrow already, I feel enabling it in 3.0 has become unrealistic.  I
>> > understand we need it sooner rather than later for postcopy recovery.
>> > However, the current state should be servicable for teaching OOB to
>> > libvirt: just follow the recommendation to supply "id" (libvirt always
>> > does anyway), pretend COMMAND_DROPPED doesn't exist, and pretend flow
>> > control isn't an issue.
>> 
>> I guess the thing is not about COMMAND_DROPPED; it's about we're going
>> to drop the x-oob parameter.  Now we can only use that parameter to
>> enable out-of-band, and after we enable it by default we possibly want
>> to remove that x-oob parameter.  So we'd better provide a constant way
>> for libvirt to enable out-of-band first (and now I'll assume the
>> "exec-oob" interface won't change again).  But yes I think we can do
>> that after 3.0.

Yes, x-oob will go away right when OOB transitions from experimental to
supported.

I don't expect the request interface (exec-oob) to change, except we
might still make "id" mandatory.  Libvirt won't care, as it always
supplies "id".

If having to enable OOB with x-oob hampers libvirt work, I'd be willing
to enable OOB by default early in the 3.1 development cycle.  We can
always go back to disabled by default for the 3.1 release in case we
fail at getting it ready for 3.1,

> Btw, note that patches 4-7 might still be candidates of bug fixes for
> existing out-of-band feature.  It'll drop COMMAND_DROPPED event, but
> IMHO it's still better than having a broken flow control for it (and
> dropping event declaration is pretty safe even clients implemented
> them).  Though the changes are not trivial, so I'll leave the decision
> to you on whether we'll still consider them as 3.0 material.  Anyway,
> please feel free to let me know if you want me to post them
> separately.

Yes, COMMAND_DROPPED is flawed, and I dislike shipping flawed stuff as
much as you do.  However, it's clearly documented as flawed, and to get
it, you have to enable OOB with x-oob, where the x- clearly marks this
as experimental.

I think I'd rather not rock the boat now.