On Mon, Oct 16, 2017 at 04:11:58PM +0800, Peter Xu wrote: > On Thu, Oct 12, 2017 at 01:56:20PM +0100, Stefan Hajnoczi wrote: > > On Fri, Sep 29, 2017 at 11:38:37AM +0800, Peter Xu wrote: > > > Set maximum QMP request queue length to 8. If queue full, instead of > > > queue the command, we directly return a "request-dropped" event, telling > > > client that specific command is dropped. > > > > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > > --- > > > monitor.c | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/monitor.c b/monitor.c > > > index 1e9a6cb6a5..d9bed31248 100644 > > > --- a/monitor.c > > > +++ b/monitor.c > > > @@ -3971,6 +3971,8 @@ static void monitor_qmp_bh_dispatcher(void *data) > > > } > > > } > > > > > > +#define QMP_ASYNC_QUEUE_LEN_MAX (8) > > > > Why 8? > > I proposed this in previous discussion and no one objected, so I just > used it. It's here: > > https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg03989.html > (please don't go over the thread; I'll copy the related paragraphs) > > """ > ... > Regarding to queue size: I am afraid max_size=1 may not suffice? > Otherwise a simple batch of: > > {"execute": "query-status"} {"execute": "query-status"} > > Will trigger the failure. But I definitely agree it should not be > something very large. The total memory will be this: > > json limit * queue length limit * monitor count limit > (X) (Y) (Z) > > Now we have (X) already (in form of a few tunables for JSON token > counts, etc.), we don't have (Z), and we definitely need (Y). > > How about we add limits on Y=16 and Z=8? > > We can do some math if we want some more exact number though. > ... > """ > > Oops, I proposed "16", but I used "8"; I hope 8 is good enough, but I > am definitely not sure whether "1" is good.
I understand the concern about breaking existing clients but choosing an arbitrary magic number isn't a correct solution to that problem because existing clients may exceed the magic number! Instead I think QMP should only look ahead if the out-of-band feature has been negotatiated. This way existing clients continue to work. New clients will have to avoid sending a batch of requests or they must handle the queue size limit error. > For the naming: how about QMP_REQ_QUEUE_LEN_MAX? Yes. Stefan
