Re: [Qemu-devel] [PATCH v8 22/23] tests: qmp-test: verify command batching

[Peter Xu]

On Sat, Mar 10, 2018 at 08:45:46PM -0600, Eric Blake wrote:
> On 03/09/2018 03:00 AM, Peter Xu wrote:
> > OOB introduced DROP event for flow control.  This should not affect old
> > QMP clients.  Add a command batching check to make sure of it.
> > 
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >   tests/qmp-test.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> > 
> 
> > +    /*
> > +     * Test command batching.  In current test OOB is not enabled, we
> > +     * should be able to run as many commands in batch as we like.
> > +     * Using 16 (>8, which is OOB queue length) to make sure OOB won't
> > +     * break existing clients.  Note: this test does not control the
> > +     * scheduling of QEMU's QMP command processing threads so it may
> > +     * not really trigger batching inside QEMU.  This is just a
> > +     * best-effort test.
> > +     */
> > +    for (i = 0; i < 16; i++) {
> > +        qtest_async_qmp(qts, "{ 'execute': 'query-version' }");
> 
> Would the test be any more robust if we could generate a single string to
> send all at once, rather than multiple separate calls to qtest_async_qmp()
> (the overhead in generating separate strings means the monitor may have made
> progress before we send the next string).  But that can be a followup, if
> you want to pursue the idea.

Yes, a single string would be nicer.

As the comment said (which was actually pointed out by Stefan), the
test is only a best effort test considering that we cannot really
control how the QEMU QMP internal handles the requests.  E.g., even if
we send the strings in one write() call, it's still only buffered on
the receiver's side only, and it'll be QEMU who decide how to fetch
the buffer content.

I can fix this up if there is a new post, or do it separately
otherwise along with the other suggestion below.

> 
> > +    }
> > +    /* Verify the replies to make sure no command is dropped. */
> > +    for (i = 0; i < 16; i++) {
> > +        resp = qtest_qmp_receive(qts);
> > +        /* It should never be dropped.  Each of them should be a reply. */
> > +        g_assert(qdict_haskey(resp, "return"));
> > +        g_assert(!qdict_haskey(resp, "event"));
> > +        QDECREF(resp);
> 
> Should we also be sending unique ids, and ensure that the responses arrive
> with ids in the same order?  Again, idea for a followup.

Yes, it can be better.  Will apply the same idea as above, depending
on the fate of current series.

> 
> Reviewed-by: Eric Blake <address@hidden>

Thanks!

-- 
Peter Xu