On 07.02.2013 16:27, Daniel P. Berrange wrote:
On Thu, Feb 07, 2013 at 12:36:36PM +0100, Michal Privoznik wrote:
With current implementation, virCommandWait unregister the stdio callback and finish reading itself. However, the eventloop may already have been in process of executing the callback in which case one obtain these obscure error messages, like:
2013-02-06 11:41:43.766+0000: 19078: warning : virEventPollRemoveHandle:183 : Ignoring invalid remove watch -1 2013-02-06 11:41:43.766+0000: 19078: warning : virFileClose:65 : Tried to close invalid fd 25
The solution is to introduce a mutex and condition to virCommand, and wait in virCommandWait for the eventloop to process all IOs. This, however, requires all callees to unlock all mutexes held, otherwise the eventloop may try to lock one of them and experience deadlock. If that's the case, we will never be woken up to unlock the problematic mutex. --- src/qemu/qemu_driver.c | 58 +++++++++++++++++++++++++--- src/qemu/qemu_migration.c | 15 +++++++- src/util/vircommand.c | 98 ++++++++++++++++++++++++++++++++++++----------- src/util/virfile.c | 4 ++ 4 files changed, 146 insertions(+), 29 deletions(-)
ACK, reluctantly - I feel this async I/O code has grown rather more complex than it ought to have been.
The async I/O code in virCommand was supposed to be about simplifying life - but the requiring the callers to do all this mutex locking/unlocking seems to have made things worse than they were before we had async I/O code IMHO. I'm half inclined to say we should just revert the whole lot.
Daniel
I agree. The other solution that has came up to my mind is just to spawn virCommandProcessIO (which is doing its own poll()) in a separate thread and hence we don't need to require the unlock. virCommandWait would just join the thread then. However, I am not so comfortable with spawning threads around with no real reason. Having said that, I will push 2/2 as it is unrelated, and postpone pushing 1/2 for a while to let others express themselves. Michal