Re: [Qemu-devel] [PULL 0/8] Monitor patches for 2018-10-30

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> On Tue, Nov 20, 2018 at 06:44:27PM +0100, Markus Armbruster wrote:
>> 
>> We want the test to drive QEMU into the "queue full" state with an
>> out-of-band command unreceived, then watch the queue drain and the
>> out-of-band command overtaking part of it.
>> 
>> Driving QEMU into this state is easy enough: we can use a blocking
>> in-band command to plug the queue's drain.
>> 
>> Once we have QEMU in this state, we can unblock that command and watch
>> the show.  But we really have to wait until QEMU reaches this state
>> before we unblock.
>> 
>> Our problem is the test can't easily observe when QEMU reaches this
>> state.
>> 
>> You mentioned sending a "queue full" event, and that adding such an
>> event just for testing is hard to justify.  I agree.  Could such an
>> event have a non-testing use?  All I can think of is letting the
>> management application detect "we mismanaged the queue and compromised
>> monitor availability for out-of-band commands".  Doesn't sound
>> particularly convincing to me, but we can talk with libvirt guys to see
>> whether they want it.
>> 
>> Can the test observe the monitor is suspended just from the behavior of
>> its monitor socket?  Not reliably: sending lots of input eventually
>> fails with EAGAIN when the monitor is suspended, but it can also fail
>> when the monitor reads the input too slowly.
>> 
>> Can the test observe the monitor_suspend trace point?  Awkward, because
>> the trace backends are configurable.  We could have the test monitor the
>> "simple" backend's output file, skipping the test when "simple" isn't
>> available.  Hmm.
>> 
>> Here comes the heavy hammer: extend the qtest protocol.  When the
>> monitor suspends / resumes, send a suitable asynchronous message to the
>> qtest socket.  Have libqtest use that to track monitor suspend state.
>> The test can then busy-wait for the state to flip to suspended.
>> 
>> Other ideas?
>
> I don't have more to add (already a lot of ideas! :-).  I'd say I
> would prefer to start with the queue full event and we can discuss
> with libvirt on that after 3.1 when proper.  And besides libvirt
> (which I am not quite sure whether this event could be really useful
> in the near future) maybe it can also be used as a hint for any new
> QMP client when people want to complain about "hey, why my QMP client
> didn't get any respond from QEMU" - if the client didn't get any
> response but however it sees this event then we know the answer even
> before debugging more.

Good point.

Say we implement full flow control, i.e. we also suspend when the output
buffer becomes too large.  Then the event won't really help, because the
client won't see it until it fixed the situation by draining the output
buffer.  But it won't really hurt, either: even a client that misbehaves
in a really unfortunate way can't trigger many events without also
generating much more other output.  Orders of magnitude more.

>> >> > Markus, do you want me to repost a new version with this change?  Is
>> >> > it still possible to have the oob-default series for 3.1?
>> >> 
>> >> We're trying to, but we need to get the test to work.
>> >> 
>> >> Two problems:
>> >> 
>> >> 1. The test doesn't seem to succed at testing "queue full".
>> >> 
>> >> 2. The test might still be racy.
>> >> 
>> >> I suspect the code we test is solid, and it's "only" the test we screwed
>> >> up.
>> >
>> > I agree.  Do you have better idea to test queue full?  I'd be more
>> > than glad to try any suggestions here, otherwise I am thinking whether
>> > we can just simply drop the queue full test (which is the last patch
>> > of the oob series) for now but merge the rest.  We can introduce new
>> > queue full test after 3.1 if we want, though again I really doubt
>> > whether we can without new things introduced into the procedure.
>> >
>> > Thoughts?
>> 
>> I'm afraid dropping the test (for now?) is what we have to do.
>
> Agreed.

Okay, I'll prepare the pull request later today.