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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> 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.

I didn't.  I decided it's too late for 3.1.  Sigh...

I'm keeping it queued for 4.0.