Re: [libvirt] [PATCH 2/3] libvirtaio: add allow for moving callbacks to other event loop
On Fri, Sep 01, 2017 at 01:16:51PM +0100, Daniel P. Berrange wrote:
On Fri, Sep 01, 2017 at 01:51:17PM +0200, Wojtek Porczyk wrote:
> On Fri, Sep 01, 2017 at 10:08:18AM +0100, Daniel P. Berrange wrote:
> > IIUC, you are trying to make it possible to register multiple event
> > loop impls. This is *not* supported usage of libvirt. You must
> > call 'virEventRegisterImpl' before opening any connection, and once
> > called you are forbidden to call it again.
>
> Yes, that's correct. Can't I do it, even after I close all the connections?
That would be racy because some cleanup is liable to happen asynchronously.
What cleanup? Some cleanup in C-level libvirt client, or expressed as
callbacks and therefore visible to implementation itself? The latter is
largely remedied by drain().
> Why then libvirt_virEventRegisterImpl
(libvirt-python/libvirt-override.c:5434)
> seems to accomodate for running it second time?
That's bogus code that we should remove - in fact the C library
should simply ignore any subsequent virEventRegisterImpl API
calls after the first one.
Thanks for clarification. I understand that it wasn't the intended way and
it's not meant to be supported and hard to test. But it works OK so far. It
only begs the question, how much of libvirt-override.c is also deemed to be
"bogus code". :)
> The reason for this is we have separate event loop for each test
case, but the
> whole suite runs in a single process. The Impl has to use the new loop for
> each test. Would it be better to just substitute the loop in a long-lived Impl
> instance?
Replacing 'loop' would achieve the same effet i guess, but you must
ensure there are no callbacks still registered by libvirt before doing
that.
Well, if someone correctly unregisters any event handlers and matches any
references to connection with apropriate number of virConnectionClose()s,
there should be no callbacks registered and no descriptors watched. There are
other libraries which behave in similar way. Python throws a Warning when
closing a non-empty loop (wrt descriptors and pending tasks), which we use as
convenient way to ensure that nothing there remains. That's how we currently
do it in tests anyway.
Generally the expectation is that you register an event loop and
then
run it forever until the process exits.
TBH asyncio explicitly disclaims that guarantee, and there are different
recommendations as to how people shoud run their loops. Some those include
running one loop per thread. Some non-stdlib loop implementations even allow
running loop inside the loop for various X toolkit related reasons.
If this patch is controversial, should I submit a series without this patch,
because other two are unrelated?
--
pozdrawiam / best regards _.-._
Wojtek Porczyk .-^' '^-.
Invisible Things Lab |'-.-^-.-'|
| | | |
I do not fear computers, | '-.-' |
I fear lack of them. '-._ : ,-'
-- Isaac Asimov `^-^-_>
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)