Please revert f4be03b3 (libvirtaio: Drop object(*args, **kwargs)) for theoretical reasons

[Wojtek Porczyk]

Hi Philipp,
(Cc: Daniel, because IIUC you reviewed !16 which got this merged),

I'm sorry I didn't notice this earlier, but the commit f4be03b3 dated
2020-04-20 [0] is wrong. The super().__init__(*args, **kwargs) in
Callback.__init__ was there on purpose, because of how Python's inheritance in
new-style classes works.

Let me explain this a bit, because it is not obvious.

Suppose you had diamond inheritance like this:

class A(object): pass
class B(A): pass
class C(A): pass
class D(B,C): pass

And those classes needed a common function with varying arguments:

class A(object):
def spam(self, a): print(f'A: {a}')
class B(A):
def spam(self, b): print(f'B: {b}')
class C(A):
def spam(self, c): print(f'C: {c}')
class D(B,C):
def spam(self, d): print(f'D: {d}')

The way to call all parent's functions exactly once (as per MRO) and accept
all arguments and also forbid unknown arguments is to accept **kwargs
everywhere and pass them to super().spam():

class A:
def spam(self, a):
print(f'A: {a}')
class B(A):
def spam(self, b, **kwargs):
print(f'B: {b}')
super().spam(**kwargs)
class C(A):
def spam(self, c, **kwargs):
print(f'C: {c}')
super().spam(**kwargs)
class D(B, C):
def spam(self, d, **kwargs):
print(f'D: {d}')
super().spam(**kwargs)

Let's run this:

>>> B().spam(a=1, b=2)
B: 2
A: 1
>>> D().spam(a=1, b=2, c=3, d=4)
D: 4
B: 2
C: 3
A: 1

You may notice that super() in B.spam refers to two different classes, either
A or C, depending on inheritance order in yet undefinded classes (as of B's
definition).

That's why the conclusion that super() in Callback.__init__ refers to object
is wrong. In this example, spam=__init__, A=object, B=Callback and C and D are
not yet written, but theoretically possible classes that could be written by
someone else. Why would they be needed, I don't know, but if someone written
them, s/he would be out of options to invent new arguments to C.__init__.

Note that super().__init__(*args, **kwargs) when super() refers to object
isn't harmful, and just ensures that args and kwargs are empty (i.e. no
unknown arguments were passed). In fact, this is exactly why object.__init__()
takes no arguments since Python 2.6 [1][2], as you correctly point out in the
commit message.

I don't think this breaks anything (I very much doubt anyone would need to
write code that would trigger this), nevertheless, as the commit is both
pointless and wrong, and as the original author of libvirtaio I'd like to ask
for this commit to be reverted. If this breaks some static analysis tool,
could you just suppress it for this particular line?


[0] 
https://gitlab.com/libvirt/libvirt-python/-/commit/f4be03b330125ab1e5a2bb10b4f12674aeff4691
[1] https://bugs.python.org/issue1683368
[2] https://docs.python.org/3/whatsnew/2.6.html#porting-to-python-2-6
(fourth point)

-- 
pozdrawiam / best regards
Wojtek Porczyk
Invisible Things Lab
 
 I do not fear computers,
 I fear lack of them.
-- Isaac Asimov


signature.asc
Description: PGP signature

Re: [libvirt] [PATCH v2 1/6] libvirtaio: add more debug logging

[Wojtek Porczyk]

On Mon, Sep 25, 2017 at 02:43:56PM +0100, Daniel P. Berrange wrote:
> On Thu, Aug 24, 2017 at 09:22:49PM +0200, Wojtek Porczyk wrote:
> > This logging is helpful for tracing problems with unclosed connections
> > and leaking file descriptors.
> > 
> > Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
> > ---
> >  libvirtaio.py | 33 +
> >  1 file changed, 25 insertions(+), 8 deletions(-)
> > 
> > diff --git a/libvirtaio.py b/libvirtaio.py
> > index 7c8c396..46de9ab 100644
> > --- a/libvirtaio.py
> > +++ b/libvirtaio.py
> > @@ -74,7 +74,7 @@ class Callback(object):
> >  def close(self):
> >  '''Schedule *ff* callback'''
> >  self.impl.log.debug('callback %d close(), scheduling ff', 
> > self.iden)
> > -self.impl.schedule_ff_callback(self.opaque)
> > +self.impl.schedule_ff_callback(self.iden, self.opaque)
> >  
> >  #
> >  # file descriptors
> > @@ -275,6 +275,10 @@ class virEventAsyncIOImpl(object):
> >  self.descriptors = DescriptorDict(self)
> >  self.log = logging.getLogger(self.__class__.__name__)
> >  
> > +def __repr__(self):
> > +return '<{} callbacks={} descriptors={}>'.format(
> > +type(self).__name__, self.callbacks, self.descriptors)
> > +
> >  def register(self):
> >  '''Register this instance as event loop implementation'''
> >  # pylint: disable=bad-whitespace
> > @@ -284,9 +288,18 @@ class virEventAsyncIOImpl(object):
> >  self._add_timeout, self._update_timeout, self._remove_timeout)
> >  return self
> >  
> > -def schedule_ff_callback(self, opaque):
> > +def schedule_ff_callback(self, iden, opaque):
> >  '''Schedule a ff callback from one of the handles or timers'''
> > -self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque)
> > +ensure_future(self._ff_callback(iden, opaque), loop=self.loop)
> 
> This use of ensure_future puts a min python version of 3.4 on the code. Is
> there a strong reason to prefer that over the previous call_soon code ?

1) There is no technical reason whatsoever. This is an artifact from the
previous iteration.

2) In fact ensure_future() does not make it incompatible with python<3.4
because of the try/except import at the beginning of the file (in python<3.4.3
the function is called asyncio.async, imported as ensure_future).

-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/6] libvirt-python/libvirtaio patches for 3.8.0

[Wojtek Porczyk]

On Mon, Sep 25, 2017 at 12:14:24PM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 22, 2017 at 05:54:04PM +0200, Wojtek Porczyk wrote:
> > Hi libvirt-list,
> > 
> > I'd like to submit a second iteration of libvirtaio series for 3.8.0.
> > There are some improvements to have better control over closing
> > connections, which are important in test suites which repeatedly open
> > and close connections. Also there are some minor bugfixes and better
> > logging.
> 
> There's something strange in this posting - the patches I received are all
> missing 'libvir-list@redhat.com' in the To: field - its almost like this
> was Bcc'd to the list which means anyone replying doesn't copy the list
> address on their reply. Even more odd though, I got a second copy of
> patch 3 which does have the To field set.

I'm sorry, this was my fault. I sent the whole series by [b]ouncing this in
mutt, because it otherwise mangles Message-Id and In-Reply-To: fields. The
previous posting of this series has this problem. I did it two times, since
there was a delay in delivery (I don't know why, maybe greylisting) and
I worried that the first batch was rejected for the lack of To: field, which
I forgot at the time of git format-patch.

Sorry again for the noise.


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 6/6] libvirtaio: add .drain() coroutine

[Wojtek Porczyk]

On Mon, Sep 25, 2017 at 02:53:01PM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 14, 2017 at 02:41:12AM +0200, Wojtek Porczyk wrote:
> > The intended use is to ensure that the implementation is empty, which is
> > one way to ensure that all connections were properly closed and file
> > descriptors reclaimed.
> > 
> > Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
> > ---
(snip)
> > +@asyncio.coroutine
> > +def drain(self):
> > +'''Wait for the implementation to become idle.
> > +
> > +This is a coroutine.
> > +'''
> > +self.log.debug('drain()')
> > +if self._pending:
> > +yield from self._finished.wait()
> > +self.log.debug('drain ended')
> 
> What is responsible for calling 'drain'  ?

Users of the library, and they do it at their pleasure. This is to allow the
loop to actually run the scheduled tasks. After calling virConnect.close() the
handles/timeouts aren't actually closed until the loop run the scheduled
callbacks. In practice a simple `yield` in a coroutine would be also
sufficient since respective Tasks are in the _ready queue and all run during
next loop cycle, but that's not guaranteed in asyncio specification.


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 5/6] libvirtaio: keep track of the current implementation

[Wojtek Porczyk]

Since 7534c19 it is not possible to register event implementation twice.
Instead, allow for retrieving the current one, should it be needed
afterwards.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirtaio.py | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index a7ede41..97a7f6c 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -30,7 +30,11 @@ Register the implementation of default loop:
 
 __author__ = 'Wojtek Porczyk <w...@invisiblethingslab.com>'
 __license__ = 'LGPL-2.1+'
-__all__ = ['virEventAsyncIOImpl', 'virEventRegisterAsyncIOImpl']
+__all__ = [
+'getCurrentImpl',
+'virEventAsyncIOImpl',
+'virEventRegisterAsyncIOImpl',
+]
 
 import asyncio
 import itertools
@@ -401,10 +405,18 @@ class virEventAsyncIOImpl(object):
 callback = self.callbacks.pop(timer)
 callback.close()
 
+
+_current_impl = None
+def getCurrentImpl():
+'''Return the current implementation, or None if not yet registered'''
+return _current_impl
+
 def virEventRegisterAsyncIOImpl(loop=None):
 '''Arrange for libvirt's callbacks to be dispatched via asyncio event loop
 
 The implementation object is returned, but in normal usage it can safely be
 discarded.
 '''
-return virEventAsyncIOImpl(loop=loop).register()
+global _current_impl
+_current_impl = virEventAsyncIOImpl(loop=loop).register()
+return _current_impl
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 3/6] libvirtaio: do not double-add callbacks

[Wojtek Porczyk]

This was a harmless bug, without any impact, but it is wrong to manage
the collection of callbacks from it's members.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirtaio.py | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index d962e64..239561d 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -63,11 +63,6 @@ class Callback(object):
 self.cb = cb
 self.opaque = opaque
 
-assert self.iden not in self.impl.callbacks, \
-'found {} callback: {!r}'.format(
-self.iden, self.impl.callbacks[self.iden])
-self.impl.callbacks[self.iden] = self
-
 def __repr__(self):
 return '<{} iden={}>'.format(self.__class__.__name__, self.iden)
 
@@ -324,6 +319,8 @@ class virEventAsyncIOImpl(object):
 '''
 callback = FDCallback(self, cb, opaque,
 descriptor=self.descriptors[fd], event=event)
+assert callback.iden not in self.callbacks
+
 self.log.debug('add_handle(fd=%d, event=%d, cb=..., opaque=...) = %d',
 fd, event, callback.iden)
 self.callbacks[callback.iden] = callback
@@ -376,6 +373,8 @@ class virEventAsyncIOImpl(object):
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddTimeoutFunc
 '''
 callback = TimeoutCallback(self, cb, opaque)
+assert callback.iden not in self.callbacks
+
 self.log.debug('add_timeout(timeout=%d, cb=..., opaque=...) = %d',
 timeout, callback.iden)
 self.callbacks[callback.iden] = callback
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 6/6] libvirtaio: add .drain() coroutine

[Wojtek Porczyk]

The intended use is to ensure that the implementation is empty, which is
one way to ensure that all connections were properly closed and file
descriptors reclaimed.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirtaio.py | 36 ++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index 97a7f6c..1c432dd 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -269,10 +269,27 @@ class virEventAsyncIOImpl(object):
 self.descriptors = DescriptorDict(self)
 self.log = logging.getLogger(self.__class__.__name__)
 
+# NOTE invariant: _finished.is_set() iff _pending == 0
+self._pending = 0
+self._finished = asyncio.Event(loop=loop)
+self._finished.set()
+
 def __repr__(self):
 return '<{} callbacks={} descriptors={}>'.format(
 type(self).__name__, self.callbacks, self.descriptors)
 
+def _pending_inc(self):
+'''Increase the count of pending affairs. Do not use directly.'''
+self._pending += 1
+self._finished.clear()
+
+def _pending_dec(self):
+'''Decrease the count of pending affairs. Do not use directly.'''
+assert self._pending > 0
+self._pending -= 1
+if self._pending == 0:
+self._finished.set()
+
 def register(self):
 '''Register this instance as event loop implementation'''
 # pylint: disable=bad-whitespace
@@ -293,7 +310,20 @@ class virEventAsyncIOImpl(object):
 This is a coroutine.
 '''
 self.log.debug('ff_callback(iden=%d, opaque=...)', iden)
-return libvirt.virEventInvokeFreeCallback(opaque)
+ret = libvirt.virEventInvokeFreeCallback(opaque)
+self._pending_dec()
+return ret
+
+@asyncio.coroutine
+def drain(self):
+'''Wait for the implementation to become idle.
+
+This is a coroutine.
+'''
+self.log.debug('drain()')
+if self._pending:
+yield from self._finished.wait()
+self.log.debug('drain ended')
 
 def is_idle(self):
 '''Returns False if there are leftovers from a connection
@@ -301,7 +331,7 @@ class virEventAsyncIOImpl(object):
 Those may happen if there are sematical problems while closing
 a connection. For example, not deregistered events before .close().
 '''
-return not self.callbacks
+return not self.callbacks and not self._pending
 
 def _add_handle(self, fd, event, cb, opaque):
 '''Register a callback for monitoring file handle events
@@ -324,6 +354,7 @@ class virEventAsyncIOImpl(object):
 fd, event, callback.iden)
 self.callbacks[callback.iden] = callback
 self.descriptors[fd].add_handle(callback)
+self._pending_inc()
 return callback.iden
 
 def _update_handle(self, watch, event):
@@ -378,6 +409,7 @@ class virEventAsyncIOImpl(object):
 timeout, callback.iden)
 self.callbacks[callback.iden] = callback
 callback.update(timeout=timeout)
+self._pending_inc()
 return callback.iden
 
 def _update_timeout(self, timer, timeout):
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 4/6] libvirtaio: fix closing of the objects

[Wojtek Porczyk]

- Descriptor.close() was a dead code, never used.
- TimeoutCallback.close(), as a cleanup function, should have called
super() as last statement, not first

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirtaio.py | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index 239561d..a7ede41 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -154,11 +154,6 @@ class Descriptor(object):
 self.update()
 return callback
 
-def close(self):
-''
-self.callbacks.clear()
-self.update()
-
 class DescriptorDict(dict):
 '''Descriptors collection
 
@@ -249,8 +244,8 @@ class TimeoutCallback(Callback):
 
 def close(self):
 '''Stop the timer and call ff callback'''
-super(TimeoutCallback, self).close()
 self.update(timeout=-1)
+super(TimeoutCallback, self).close()
 
 #
 # main implementation
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/6] libvirtaio: add more debug logging

[Wojtek Porczyk]

This logging is helpful for tracing problems with unclosed connections
and leaking file descriptors.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirtaio.py | 33 +
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index 7c8c396..46de9ab 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -74,7 +74,7 @@ class Callback(object):
 def close(self):
 '''Schedule *ff* callback'''
 self.impl.log.debug('callback %d close(), scheduling ff', self.iden)
-self.impl.schedule_ff_callback(self.opaque)
+self.impl.schedule_ff_callback(self.iden, self.opaque)
 
 #
 # file descriptors
@@ -275,6 +275,10 @@ class virEventAsyncIOImpl(object):
 self.descriptors = DescriptorDict(self)
 self.log = logging.getLogger(self.__class__.__name__)
 
+def __repr__(self):
+return '<{} callbacks={} descriptors={}>'.format(
+type(self).__name__, self.callbacks, self.descriptors)
+
 def register(self):
 '''Register this instance as event loop implementation'''
 # pylint: disable=bad-whitespace
@@ -284,9 +288,18 @@ class virEventAsyncIOImpl(object):
 self._add_timeout, self._update_timeout, self._remove_timeout)
 return self
 
-def schedule_ff_callback(self, opaque):
+def schedule_ff_callback(self, iden, opaque):
 '''Schedule a ff callback from one of the handles or timers'''
-self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque)
+ensure_future(self._ff_callback(iden, opaque), loop=self.loop)
+
+@asyncio.coroutine
+def _ff_callback(self, iden, opaque):
+'''Directly free the opaque object
+
+This is a coroutine.
+'''
+self.log.debug('ff_callback(iden=%d, opaque=...)', iden)
+return libvirt.virEventInvokeFreeCallback(opaque)
 
 def is_idle(self):
 '''Returns False if there are leftovers from a connection
@@ -309,10 +322,10 @@ class virEventAsyncIOImpl(object):
 .. seealso::
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFuncFunc
 '''
-self.log.debug('add_handle(fd=%d, event=%d, cb=%r, opaque=%r)',
-fd, event, cb, opaque)
 callback = FDCallback(self, cb, opaque,
 descriptor=self.descriptors[fd], event=event)
+self.log.debug('add_handle(fd=%d, event=%d, cb=..., opaque=...) = %d',
+fd, event, callback.iden)
 self.callbacks[callback.iden] = callback
 self.descriptors[fd].add_handle(callback)
 return callback.iden
@@ -339,7 +352,11 @@ class virEventAsyncIOImpl(object):
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventRemoveHandleFunc
 '''
 self.log.debug('remove_handle(watch=%d)', watch)
-callback = self.callbacks.pop(watch)
+try:
+callback = self.callbacks.pop(watch)
+except KeyError as err:
+self.log.warning('remove_handle(): no such handle: %r', 
err.args[0])
+raise
 fd = callback.descriptor.fd
 assert callback is self.descriptors[fd].remove_handle(watch)
 if len(self.descriptors[fd].callbacks) == 0:
@@ -358,9 +375,9 @@ class virEventAsyncIOImpl(object):
 .. seealso::
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddTimeoutFunc
 '''
-self.log.debug('add_timeout(timeout=%d, cb=%r, opaque=%r)',
-timeout, cb, opaque)
 callback = TimeoutCallback(self, cb, opaque)
+self.log.debug('add_timeout(timeout=%d, cb=..., opaque=...) = %d',
+timeout, callback.iden)
 self.callbacks[callback.iden] = callback
 callback.update(timeout=timeout)
 return callback.iden
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/6] libvirt-python/libvirtaio patches for 3.8.0

[Wojtek Porczyk]

Hi libvirt-list,

I'd like to submit a second iteration of libvirtaio series for 3.8.0.
There are some improvements to have better control over closing
connections, which are important in test suites which repeatedly open
and close connections. Also there are some minor bugfixes and better
logging.

Since v1 (31.08.2017) there are changes:
- dropped implementation re-registering
- added current implementation getter instead
- added unrelated bugfixes

Wojtek Porczyk (6):
  libvirtaio: add more debug logging
  libvirtaio: cache the list of callbacks when calling
  libvirtaio: do not double-add callbacks
  libvirtaio: fix closing of the objects
  libvirtaio: keep track of the current implementation
  libvirtaio: add .drain() coroutine

 libvirtaio.py | 101 +-
 1 file changed, 78 insertions(+), 23 deletions(-)

-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 2/6] libvirtaio: cache the list of callbacks when calling

[Wojtek Porczyk]

When the callback causes something that results in changes wrt
registered handles, python aborts iteration.

Relevant error message:

Exception in callback None()
handle: 
Traceback (most recent call last):
  File "/usr/lib64/python3.5/asyncio/events.py", line 126, in _run
self._callback(*self._args)
  File "/usr/lib64/python3.5/site-packages/libvirtaio.py", line 99, in 
_handle
for callback in self.callbacks.values():
RuntimeError: dictionary changed size during iteration

QubesOS/qubes-issues#2805
Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirtaio.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index 46de9ab..d962e64 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -96,7 +96,7 @@ class Descriptor(object):
 
 :param int event: The event (from libvirt's constants) being dispatched
 '''
-for callback in self.callbacks.values():
+for callback in list(self.callbacks.values()):
 if callback.event is not None and callback.event & event:
 callback.cb(callback.iden, self.fd, event, callback.opaque)
 
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] libvirtaio: add allow for moving callbacks to other event loop

[Wojtek Porczyk]

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 `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] libvirtaio: add allow for moving callbacks to other event loop

[Wojtek Porczyk]

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?
Why then libvirt_virEventRegisterImpl (libvirt-python/libvirt-override.c:5434)
seems to accomodate for running it second time?

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?


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/3] libvirtaio: add allow for moving callbacks to other event loop

[Wojtek Porczyk]

The virEvent implementation is tied to a particular loop. When spinning
another loop, the callbacks have to be moved to another implementation,
so they will have a chance to be invoked, should they be scheduled. If
not, file descriptors will be leaking.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirtaio.py | 64 +++
 1 file changed, 51 insertions(+), 13 deletions(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index fc868bd..d161cd1 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -195,9 +195,10 @@ class FDCallback(Callback):
 return '<{} iden={} fd={} event={}>'.format(
 self.__class__.__name__, self.iden, self.descriptor.fd, self.event)
 
-def update(self, event):
+def update(self, event=None):
 '''Update the callback and fix descriptor's watchers'''
-self.event = event
+if event is not None:
+self.event = event
 self.descriptor.update()
 
 #
@@ -238,20 +239,21 @@ class TimeoutCallback(Callback):
 self.cb(self.iden, self.opaque)
 self.impl.log.debug('timer %r callback ended', self.iden)
 
-def update(self, timeout):
+def update(self, timeout=None):
 '''Start or the timer, possibly updating timeout'''
-self.timeout = timeout
-
-if self.timeout >= 0 and self._task is None:
-self.impl.log.debug('timer %r start', self.iden)
-self._task = ensure_future(self._timer(),
-loop=self.impl.loop)
+if timeout is not None:
+self.timeout = timeout
 
-elif self.timeout < 0 and self._task is not None:
+if self._task is not None:
 self.impl.log.debug('timer %r stop', self.iden)
 self._task.cancel()  # pylint: disable=no-member
 self._task = None
 
+if self.timeout >= 0:
+self.impl.log.debug('timer %r start', self.iden)
+self._task = ensure_future(self._timer(),
+loop=self.impl.loop)
+
 def close(self):
 '''Stop the timer and call ff callback'''
 super(TimeoutCallback, self).close()
@@ -274,6 +276,7 @@ class virEventAsyncIOImpl(object):
 self.callbacks = {}
 self.descriptors = DescriptorDict(self)
 self.log = logging.getLogger(self.__class__.__name__)
+self.pending_tasks = set()
 
 def register(self):
 '''Register this instance as event loop implementation'''
@@ -284,9 +287,30 @@ class virEventAsyncIOImpl(object):
 self._add_timeout, self._update_timeout, self._remove_timeout)
 return self
 
+def takeover(self, other):
+'''Take over other implementation, probably registered on another loop
+
+:param virEventAsyncIOImpl other: other implementation to be taken over
+'''
+self.log.warning('%r taking over %r', self, other)
+
+while other.callbacks:
+iden, callback = other.callbacks.popitem()
+self.log.debug('  takeover %d %r', iden, callback)
+assert callback.iden == iden
+callback.impl = self
+self.callbacks[iden] = callback
+
+if isinstance(callback, FDCallback):
+fd = callback.descriptor.fd
+assert callback is other.descriptors[fd].remove_handle(iden)
+self.descriptors[fd].add_handle(callback)
+
 def schedule_ff_callback(self, iden, opaque):
 '''Schedule a ff callback from one of the handles or timers'''
-ensure_future(self._ff_callback(iden, opaque), loop=self.loop)
+fut = ensure_future(self._ff_callback(iden, opaque), loop=self.loop)
+self.pending_tasks.add(fut)
+fut.add_done_callback(self.pending_tasks.remove)
 
 @asyncio.coroutine
 def _ff_callback(self, iden, opaque):
@@ -297,13 +321,19 @@ class virEventAsyncIOImpl(object):
 self.log.debug('ff_callback(iden=%d, opaque=...)', iden)
 return libvirt.virEventInvokeFreeCallback(opaque)
 
+@asyncio.coroutine
+def drain(self):
+self.log.debug('drain()')
+if self.pending_tasks:
+yield from asyncio.wait(self.pending_tasks, loop=self.loop)
+
 def is_idle(self):
 '''Returns False if there are leftovers from a connection
 
 Those may happen if there are sematical problems while closing
 a connection. For example, not deregistered events before .close().
 '''
-return not self.callbacks
+return not self.callbacks and not self.pending_tasks
 
 def _add_handle(self, fd, event, cb, opaque):
 '''Register a callback for monitoring file handle events
@@ -403,10 +433,18 @@ class virEventAsyncIOImpl(object):
 callback = self.callbacks.pop(timer)
 callback.close()
 
+
+_current_impl = None
 def virEventRegisterAsyncIOImpl(loop=None):
 '''Arrange for libvirt's callbacks to be d

[libvirt] [PATCH 0/3] libvirt-python: misc libvirtaio for 3.8.0

[Wojtek Porczyk]

Hi libvir-list,

I would like to send three patches for libvirtaio. The first one adds some
logging, which is not critical but nice to have. The second one allows for
migration of active descriptors between loops. The third patch is a fix for
a bug related to concurrent access.

Thanks!


Wojtek Porczyk (3):
  libvirtaio: add more debug logging
  libvirtaio: add allow for moving callbacks to other event loop
  libvirtaio: cache the list of callbacks when calling

 libvirtaio.py | 93 +--
 1 file changed, 72 insertions(+), 21 deletions(-)

-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/3] libvirtaio: cache the list of callbacks when calling

[Wojtek Porczyk]

When the callback causes something that results in changes wrt
registered handles, python aborts iteration.

Relevant error message:

Exception in callback None()
handle: 
Traceback (most recent call last):
  File "/usr/lib64/python3.5/asyncio/events.py", line 126, in _run
self._callback(*self._args)
  File "/usr/lib64/python3.5/site-packages/libvirtaio.py", line 99, in 
_handle
for callback in self.callbacks.values():
RuntimeError: dictionary changed size during iteration

QubesOS/qubes-issues#2805
Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirtaio.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index d161cd1..46923d8 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -96,7 +96,7 @@ class Descriptor(object):
 
 :param int event: The event (from libvirt's constants) being dispatched
 '''
-for callback in self.callbacks.values():
+for callback in list(self.callbacks.values()):
 if callback.event is not None and callback.event & event:
 callback.cb(callback.iden, self.fd, event, callback.opaque)
 
-- 
2.9.4


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/3] libvirtaio: add more debug logging

[Wojtek Porczyk]

This logging is helpful for tracing problems with unclosed connections
and leaking file descriptors.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com
---
 libvirtaio.py | 29 +
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index 7c8c396..fc868bd 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -74,7 +74,7 @@ class Callback(object):
 def close(self):
 '''Schedule *ff* callback'''
 self.impl.log.debug('callback %d close(), scheduling ff', self.iden)
-self.impl.schedule_ff_callback(self.opaque)
+self.impl.schedule_ff_callback(self.iden, self.opaque)
 
 #
 # file descriptors
@@ -284,9 +284,18 @@ class virEventAsyncIOImpl(object):
 self._add_timeout, self._update_timeout, self._remove_timeout)
 return self
 
-def schedule_ff_callback(self, opaque):
+def schedule_ff_callback(self, iden, opaque):
 '''Schedule a ff callback from one of the handles or timers'''
-self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque)
+ensure_future(self._ff_callback(iden, opaque), loop=self.loop)
+
+@asyncio.coroutine
+def _ff_callback(self, iden, opaque):
+'''Directly free the opaque object
+
+This is a coroutine.
+'''
+self.log.debug('ff_callback(iden=%d, opaque=...)', iden)
+return libvirt.virEventInvokeFreeCallback(opaque)
 
 def is_idle(self):
 '''Returns False if there are leftovers from a connection
@@ -309,10 +318,10 @@ class virEventAsyncIOImpl(object):
 .. seealso::
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFuncFunc
 '''
-self.log.debug('add_handle(fd=%d, event=%d, cb=%r, opaque=%r)',
-fd, event, cb, opaque)
 callback = FDCallback(self, cb, opaque,
 descriptor=self.descriptors[fd], event=event)
+self.log.debug('add_handle(fd=%d, event=%d, cb=..., opaque=...) = %d',
+fd, event, callback.iden)
 self.callbacks[callback.iden] = callback
 self.descriptors[fd].add_handle(callback)
 return callback.iden
@@ -339,7 +348,11 @@ class virEventAsyncIOImpl(object):
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventRemoveHandleFunc
 '''
 self.log.debug('remove_handle(watch=%d)', watch)
-callback = self.callbacks.pop(watch)
+try:
+callback = self.callbacks.pop(watch)
+except KeyError as err:
+self.log.warning('remove_handle(): no such handle: %r', 
err.args[0])
+raise
 fd = callback.descriptor.fd
 assert callback is self.descriptors[fd].remove_handle(watch)
 if len(self.descriptors[fd].callbacks) == 0:
@@ -358,9 +371,9 @@ class virEventAsyncIOImpl(object):
 .. seealso::
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddTimeoutFunc
 '''
-self.log.debug('add_timeout(timeout=%d, cb=%r, opaque=%r)',
-timeout, cb, opaque)
 callback = TimeoutCallback(self, cb, opaque)
+self.log.debug('add_timeout(timeout=%d, cb=..., opaque=...) = %d',
+timeout, callback.iden)
 self.callbacks[callback.iden] = callback
 callback.update(timeout=timeout)
 return callback.iden
-- 
2.9.4


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/7] Provide an standard asyncio event loop impl

[Wojtek Porczyk]

On Fri, Apr 07, 2017 at 02:53:52PM +0200, Marek Marczykowski-Górecki wrote:
> On Fri, Apr 07, 2017 at 01:46:12PM +0200, Wojtek Porczyk wrote:
> > On Fri, Apr 07, 2017 at 10:22:38AM +0100, Daniel P. Berrange wrote:
> > > On Thu, Apr 06, 2017 at 10:47:48PM +0200, Wojtek Porczyk wrote:
> > > > I encountered one small problem, which I believe is orthogonal to the 
> > > > patch series:
> > > > 
> > > > On Tue, Apr 04, 2017 at 03:31:32PM +0100, Daniel P. Berrange wrote:
> > > > > diff --git a/examples/event-test.py b/examples/event-test.py
> > > > > index 751a140..ac9fbe1 100755
> > > > > --- a/examples/event-test.py
> > > > > +++ b/examples/event-test.py
> > > > 
> > > > > +netcallbacks = []
> > > > > +netcallbacks.append(vc.networkEventRegisterAny(None, 
> > > > > libvirt.VIR_NETWORK_EVENT_ID_LIFECYCLE, 
> > > > > myNetworkEventLifecycleCallback, None))
> > > > 
> > > > With vc = libvirt.open('xen:///') and with libvirt{,-python}-3.1.0 this 
> > > > line causes an
> > > > exception:
> > > > 
> > > >   libvirt.libvirtError: this function is not supported by the
> > > >connection driver: virConnectNetworkEventRegisterAny
> > > > 
> > > > Commenting it out solves the problem.
> > > 
> > > Yes, I must say the event test is only validated with the KVM driver, but 
> > > that said
> > > I thought the Xen driver would get our default nework driver activated 
> > > automatically,
> > > so I'm surprised you see that.
> > 
> > I don't know, maybe this is caused by our setup. For example we have no NIC 
> > in
> > dom0 (only loopback) and we also may have deliberately broken something 
> > around
> > the network.
> 
> Yes, our packages have ./configure (...) --without-network.

So looks like our fault. Sorry for that.


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/7] Provide an standard asyncio event loop impl

[Wojtek Porczyk]

On Fri, Apr 07, 2017 at 10:22:38AM +0100, Daniel P. Berrange wrote:
> On Thu, Apr 06, 2017 at 10:47:48PM +0200, Wojtek Porczyk wrote:
> > On Tue, Apr 04, 2017 at 03:31:27PM +0100, Daniel P. Berrange wrote:
> > > $ python3 ./examples/event-test.py --loop=asyncio --timeout=30 
> > > qemu:///session
> > 
> > Thank you for this update. I tested it backported to 3.1.0 with xen:/// 
> > using
> > event-test.py and also with our own code. Looks good to me.
> 
> Great, thank you.
> 
> > I encountered one small problem, which I believe is orthogonal to the patch 
> > series:
> > 
> > On Tue, Apr 04, 2017 at 03:31:32PM +0100, Daniel P. Berrange wrote:
> > > diff --git a/examples/event-test.py b/examples/event-test.py
> > > index 751a140..ac9fbe1 100755
> > > --- a/examples/event-test.py
> > > +++ b/examples/event-test.py
> > 
> > > +netcallbacks = []
> > > +netcallbacks.append(vc.networkEventRegisterAny(None, 
> > > libvirt.VIR_NETWORK_EVENT_ID_LIFECYCLE, myNetworkEventLifecycleCallback, 
> > > None))
> > 
> > With vc = libvirt.open('xen:///') and with libvirt{,-python}-3.1.0 this 
> > line causes an
> > exception:
> > 
> >   libvirt.libvirtError: this function is not supported by the
> >connection driver: virConnectNetworkEventRegisterAny
> > 
> > Commenting it out solves the problem.
> 
> Yes, I must say the event test is only validated with the KVM driver, but 
> that said
> I thought the Xen driver would get our default nework driver activated 
> automatically,
> so I'm surprised you see that.

I don't know, maybe this is caused by our setup. For example we have no NIC in
dom0 (only loopback) and we also may have deliberately broken something around
the network.

Cc: marmarek

-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/7] Provide an standard asyncio event loop impl

[Wojtek Porczyk]

On Fri, Apr 07, 2017 at 09:31:45AM +0200, Michal Privoznik wrote:
> On 04/06/2017 10:47 PM, Wojtek Porczyk wrote:
> >  libvirt.libvirtError: this function is not supported by the connection 
> > driver: virConnectNetworkEventRegisterAny
> 
> That's because we don't have a network driver for XEN. Usually, for stateful
> drivers (those which require daemon to run, e.g. qemu, lxc, ..) they can use
> our own bridge driver for network (it creates a bridge to which domain NICs
> are plugged in). However, then we have bunch of stateless driver for which
> libvirt is just a wrapper over their APIs. For those we would need a
> separate network driver that does the API wrapping. And it looks like
> there's none for XEN. Not sure whether there's something to wrap though. I
> mean I've no idea what XEN networking capabilities are.
> 
> But what we can do here is to surround networkEvenRegisterAny() call with
> try {} except () block and ignore ENOSUPP error.

Thanks for the explanation. Are there any other drivers which don't support
some events? If so, maybe all the event requests should be wrapped in that?


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/7] Provide an standard asyncio event loop impl

[Wojtek Porczyk]

On Tue, Apr 04, 2017 at 03:31:27PM +0100, Daniel P. Berrange wrote:
> $ python3 ./examples/event-test.py --loop=asyncio --timeout=30 qemu:///session

Thank you for this update. I tested it backported to 3.1.0 with xen:/// using
event-test.py and also with our own code. Looks good to me.


I encountered one small problem, which I believe is orthogonal to the patch 
series:

On Tue, Apr 04, 2017 at 03:31:32PM +0100, Daniel P. Berrange wrote:
> diff --git a/examples/event-test.py b/examples/event-test.py
> index 751a140..ac9fbe1 100755
> --- a/examples/event-test.py
> +++ b/examples/event-test.py

> +netcallbacks = []
> +netcallbacks.append(vc.networkEventRegisterAny(None, 
> libvirt.VIR_NETWORK_EVENT_ID_LIFECYCLE, myNetworkEventLifecycleCallback, 
> None))

With vc = libvirt.open('xen:///') and with libvirt{,-python}-3.1.0 this line 
causes an
exception:

  libvirt.libvirtError: this function is not supported by the connection 
driver: virConnectNetworkEventRegisterAny

Commenting it out solves the problem.


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/2] Add asyncio event loop implementation

[Wojtek Porczyk]

On Tue, Apr 04, 2017 at 03:27:49PM +0100, Daniel P. Berrange wrote:
> I'm going to send a patch series containing this fix & some other
> changes / fixes. Would appreciate if you can confirm my updated
> code still works from your pov.

Thanks! I'll look into this tomorrow.


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/2] libvirt-python: libvirtaio

[Wojtek Porczyk]

On Fri, Mar 17, 2017 at 02:35:15PM +0100, Wojtek Porczyk wrote:
> This is second attempt at merging libvirtaio, an event loop implementation
> which dispatches the callbacks via asyncio's event loop.

Hi, libvirt-list,

Any progress on this?
Or is there some sub-list or maintainer, who I should Cc or contact directly?


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 2/2] Add asyncio event loop implementation

[Wojtek Porczyk]

This is usable only on python >= 3.4 (or 3.3 with out-of-tree asyncio),
however it should be harmless for anyone with older python versions.

In simplest case, to have the callbacks queued on the default loop:

>>> import libvirtaio
>>> libvirtaio.virEventRegisterAsyncIOImpl()

The function is not present on non-compatible platforms.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirt-python.spec.in |   1 +
 libvirtaio.py  | 401 +
 sanitytest.py  |   2 +-
 setup.py   |  12 ++
 4 files changed, 415 insertions(+), 1 deletion(-)
 create mode 100644 libvirtaio.py

diff --git a/libvirt-python.spec.in b/libvirt-python.spec.in
index 3021ebd..0ee535e 100644
--- a/libvirt-python.spec.in
+++ b/libvirt-python.spec.in
@@ -86,6 +86,7 @@ rm -f %{buildroot}%{_libdir}/python*/site-packages/*egg-info
 %defattr(-,root,root)
 %doc ChangeLog AUTHORS NEWS README COPYING COPYING.LESSER examples/
 %{_libdir}/python3*/site-packages/libvirt.py*
+%{_libdir}/python3*/site-packages/libvirtaio.py*
 %{_libdir}/python3*/site-packages/libvirt_qemu.py*
 %{_libdir}/python3*/site-packages/libvirt_lxc.py*
 %{_libdir}/python3*/site-packages/__pycache__/libvirt.cpython-*.py*
diff --git a/libvirtaio.py b/libvirtaio.py
new file mode 100644
index 000..8428f71
--- /dev/null
+++ b/libvirtaio.py
@@ -0,0 +1,401 @@
+#
+# libvirtaio -- asyncio adapter for libvirt
+# Copyright (C) 2017  Wojtek Porczyk <w...@invisiblethingslab.com>
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, see
+# <http://www.gnu.org/licenses/>.
+#
+
+'''Libvirt event loop implementation using asyncio
+
+Register the implementation of default loop:
+
+>>> import libvirtaio
+>>> libvirtaio.virEventRegisterAsyncIOImpl()
+
+.. seealso::
+https://libvirt.org/html/libvirt-libvirt-event.html
+'''
+
+__author__ = 'Wojtek Porczyk <w...@invisiblethingslab.com>'
+__license__ = 'LGPL-2.1+'
+__all__ = ['virEventAsyncIOImpl', 'virEventRegisterAsyncIOImpl']
+
+import asyncio
+import itertools
+import logging
+import warnings
+
+import libvirt
+
+try:
+from asyncio import ensure_future
+except ImportError:
+from asyncio import async as ensure_future
+
+
+class Callback(object):
+'''Base class for holding callback
+
+:param virEventAsyncIOImpl impl: the implementation in which we run
+:param cb: the callback itself
+:param opaque: the opaque tuple passed by libvirt
+'''
+# pylint: disable=too-few-public-methods
+
+_iden_counter = itertools.count()
+
+def __init__(self, impl, cb, opaque, *args, **kwargs):
+super().__init__(*args, **kwargs)
+self.iden = next(self._iden_counter)
+self.impl = impl
+self.cb = cb
+self.opaque = opaque
+
+assert self.iden not in self.impl.callbacks, \
+'found {} callback: {!r}'.format(
+self.iden, self.impl.callbacks[self.iden])
+self.impl.callbacks[self.iden] = self
+
+def __repr__(self):
+return '<{} iden={}>'.format(self.__clas__.__name__, self.iden)
+
+def close(self):
+'''Schedule *ff* callback'''
+self.impl.log.debug('callback %d close(), scheduling ff', self.iden)
+self.impl.schedule_ff_callback(self.opaque)
+
+#
+# file descriptors
+#
+
+class Descriptor(object):
+'''Manager of one file descriptor
+
+:param virEventAsyncIOImpl impl: the implementation in which we run
+:param int fd: the file descriptor
+'''
+def __init__(self, impl, fd):
+self.impl = impl
+self.fd = fd
+self.callbacks = {}
+
+def _handle(self, event):
+'''Dispatch the event to the descriptors
+
+:param int event: The event (from libvirt's constants) being dispatched
+'''
+for callback in self.callbacks.values():
+if callback.event is not None and callback.event & event:
+callback.cb(callback.iden, self.fd, event, callback.opaque)
+
+def update(self):
+'''Register or unregister callbacks at event loop
+
+This should be called after change of any ``.event`` in callbacks.
+'''
+# It seems like loop.add_{reader,writer} can be run multiple times
+# and will still register the callback only once. Likewis

[libvirt] [PATCH v2 1/2] Allow for ff callbacks to be called by custom event implementations

[Wojtek Porczyk]

The documentation says:
> If the opaque user data requires free'ing when the handle is
> unregistered, then a 2nd callback can be supplied for this purpose.
> This callback needs to be invoked from a clean stack. If 'ff'
> callbacks are invoked directly from the virEventRemoveHandleFunc they
> will likely deadlock in libvirt.

And they did deadlock. In removeTimeout too. Now we supply a custom
function to pick it from the opaque blob and fire.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirt-override.c  | 36 
 libvirt-override.py | 39 +++
 sanitytest.py   |  5 +++--
 3 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 9e40f00..37f7ee2 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -5223,6 +5223,9 @@ libvirt_virEventAddHandleFunc(int fd,
 
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 3, cb_args, cleanup);
 
+/* If changing contents of the opaque object, please also change
+ * virEventExecuteFFCallback() in libvirt-override.py
+ */
 VIR_PY_TUPLE_SET_GOTO(cb_args, 0, libvirt_virEventHandleCallbackWrap(cb), 
cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 1, libvirt_virVoidPtrWrap(opaque), cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 2, libvirt_virFreeCallbackWrap(ff), 
cleanup);
@@ -5292,20 +5295,11 @@ libvirt_virEventRemoveHandleFunc(int watch)
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 0, libvirt_intWrap(watch), cleanup);
 
 result = PyEval_CallObject(removeHandleObj, pyobj_args);
-if (!result) {
+if (result) {
+retval = 0;
+} else {
 PyErr_Print();
 PyErr_Clear();
-} else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
-DEBUG("%s: %s must return opaque obj registered with %s"
-  "to avoid leaking libvirt memory\n",
-  __FUNCTION__, NAME(removeHandle), NAME(addHandle));
-} else {
-opaque = PyTuple_GetItem(result, 1);
-ff = PyTuple_GetItem(result, 2);
-cff = PyvirFreeCallback_Get(ff);
-if (cff)
-(*cff)(PyvirVoidPtr_Get(opaque));
-retval = 0;
 }
 
  cleanup:
@@ -5350,6 +5344,9 @@ libvirt_virEventAddTimeoutFunc(int timeout,
 
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 2, cb_args, cleanup);
 
+/* If changing contents of the opaque object, please also change
+ * virEventExecuteFFCallback() in libvirt-override.py
+ */
 VIR_PY_TUPLE_SET_GOTO(cb_args, 0, libvirt_virEventTimeoutCallbackWrap(cb), 
cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 1, libvirt_virVoidPtrWrap(opaque), cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 2, libvirt_virFreeCallbackWrap(ff), 
cleanup);
@@ -5416,20 +5413,11 @@ libvirt_virEventRemoveTimeoutFunc(int timer)
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 0, libvirt_intWrap(timer), cleanup);
 
 result = PyEval_CallObject(removeTimeoutObj, pyobj_args);
-if (!result) {
+if (result) {
+retval = 0;
+} else {
 PyErr_Print();
 PyErr_Clear();
-} else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
-DEBUG("%s: %s must return opaque obj registered with %s"
-  "to avoid leaking libvirt memory\n",
-  __FUNCTION__, NAME(removeTimeout), NAME(addTimeout));
-} else {
-opaque = PyTuple_GetItem(result, 1);
-ff = PyTuple_GetItem(result, 2);
-cff = PyvirFreeCallback_Get(ff);
-if (cff)
-(*cff)(PyvirVoidPtr_Get(opaque));
-retval = 0;
 }
 
  cleanup:
diff --git a/libvirt-override.py b/libvirt-override.py
index 63f8ecb..3d09d63 100644
--- a/libvirt-override.py
+++ b/libvirt-override.py
@@ -16,6 +16,7 @@ except ImportError:
 if str(cyg_e).count("No module named"):
 raise lib_e
 
+import ctypes
 import types
 
 # The root of all libvirt errors.
@@ -211,3 +212,41 @@ def virEventAddTimeout(timeout, cb, opaque):
 ret = libvirtmod.virEventAddTimeout(timeout, cbData)
 if ret == -1: raise libvirtError ('virEventAddTimeout() failed')
 return ret
+
+
+#
+# a caller for the ff callbacks for custom event loop implementations
+#
+
+ctypes.pythonapi.PyCapsule_GetPointer.restype = ctypes.c_void_p
+ctypes.pythonapi.PyCapsule_GetPointer.argtypes = (
+ctypes.py_object, ctypes.c_char_p)
+
+_virFreeCallback = ctypes.CFUNCTYPE(None, ctypes.c_void_p)
+
+def virEventExecuteFFCallback(opaque):
+"""
+Execute callback which frees the opaque buffer
+
+@opaque: the opaque object passed to addHandle or addTimeout
+
+WARNING: This function should not be called from any call by libvirt's
+core. It will most probably cause deadlock in C-level libvirt code.
+Instead it should be scheduled and called from implementation's stack.
+
+See 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFunc
+for mo

[libvirt] [PATCH v2 0/2] libvirt-python: libvirtaio

[Wojtek Porczyk]

Hello, libvirt-list,

This is second attempt at merging libvirtaio, an event loop implementation
which dispatches the callbacks via asyncio's event loop.

The first patch fixes the bug around freeing opaque objects [1][2], and the
second one is the actual implementation.

Since v1 series, as per Daniel Berrange's notes, the second patch has licence
comment changed to LGPL-2.1+ and there is no import into main libvirt module.
The first patch is unchanged.

[1] https://www.redhat.com/archives/libvir-list/2017-January/msg00863.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1433028

Wojtek Porczyk (2):
  Allow for ff callbacks to be called by custom event implementations
  Add asyncio event loop implementation

 libvirt-override.c |  36 ++---
 libvirt-override.py|  39 +
 libvirt-python.spec.in |   1 +
 libvirtaio.py  | 401 +
 sanitytest.py  |   5 +-
 setup.py   |  12 ++
 6 files changed, 468 insertions(+), 26 deletions(-)
 create mode 100644 libvirtaio.py

-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v1 2/2] Add asyncio event loop implementation

[Wojtek Porczyk]

This is usable only on python >= 3.4 (or 3.3 with out-of-tree asyncio),
however it should be harmless for anyone with older python versions.

In simplest case, to have the callbacks queued on the default loop:

>>> import libvirt
>>> libvirt.virEventRegisterAsyncIOImpl()

The function is not present on non-compatible platforms.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirt-override.py|   6 +
 libvirt-python.spec.in |   1 +
 libvirtaio.py  | 398 +
 sanitytest.py  |   2 +-
 setup.py   |  12 ++
 5 files changed, 418 insertions(+), 1 deletion(-)
 create mode 100644 libvirtaio.py

diff --git a/libvirt-override.py b/libvirt-override.py
index 3d09d63..6a28336 100644
--- a/libvirt-override.py
+++ b/libvirt-override.py
@@ -16,6 +16,12 @@ except ImportError:
 if str(cyg_e).count("No module named"):
 raise lib_e
 
+try:
+from libvirtaio import virEventAsyncIOImpl, virEventRegisterAsyncIOImpl
+except (ImportError, SyntaxError):
+# python < 3.3, or 3.3 and no out-of-tree asyncio
+pass
+
 import ctypes
 import types
 
diff --git a/libvirt-python.spec.in b/libvirt-python.spec.in
index 3021ebd..0ee535e 100644
--- a/libvirt-python.spec.in
+++ b/libvirt-python.spec.in
@@ -86,6 +86,7 @@ rm -f %{buildroot}%{_libdir}/python*/site-packages/*egg-info
 %defattr(-,root,root)
 %doc ChangeLog AUTHORS NEWS README COPYING COPYING.LESSER examples/
 %{_libdir}/python3*/site-packages/libvirt.py*
+%{_libdir}/python3*/site-packages/libvirtaio.py*
 %{_libdir}/python3*/site-packages/libvirt_qemu.py*
 %{_libdir}/python3*/site-packages/libvirt_lxc.py*
 %{_libdir}/python3*/site-packages/__pycache__/libvirt.cpython-*.py*
diff --git a/libvirtaio.py b/libvirtaio.py
new file mode 100644
index 000..44c9a5b
--- /dev/null
+++ b/libvirtaio.py
@@ -0,0 +1,398 @@
+#
+# Copyright 2017 Wojtek Porczyk <w...@invisiblethingslab.com>
+#
+# Licensed under the Apache License, Version 2.0 (the 'License');
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an 'AS IS' BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+'''Libvirt event loop implementation using asyncio
+
+Register the implementation of default loop:
+
+>>> import libvirt
+>>> libvirt.virEventRegisterAsyncIOImpl()
+
+.. seealso::
+https://libvirt.org/html/libvirt-libvirt-event.html
+'''
+
+__author__ = 'Wojtek Porczyk <w...@invisiblethingslab.com>'
+__license__ = 'Apache-2.0'
+__all__ = ['virEventAsyncIOImpl', 'virEventRegisterAsyncIOImpl']
+
+import asyncio
+import itertools
+import logging
+import warnings
+
+import libvirt
+
+try:
+from asyncio import ensure_future
+except ImportError:
+from asyncio import async as ensure_future
+
+
+class Callback(object):
+'''Base class for holding callback
+
+:param virEventAsyncIOImpl impl: the implementation in which we run
+:param cb: the callback itself
+:param opaque: the opaque tuple passed by libvirt
+'''
+# pylint: disable=too-few-public-methods
+
+_iden_counter = itertools.count()
+
+def __init__(self, impl, cb, opaque, *args, **kwargs):
+super().__init__(*args, **kwargs)
+self.iden = next(self._iden_counter)
+self.impl = impl
+self.cb = cb
+self.opaque = opaque
+
+assert self.iden not in self.impl.callbacks, \
+'found {} callback: {!r}'.format(
+self.iden, self.impl.callbacks[self.iden])
+self.impl.callbacks[self.iden] = self
+
+def __repr__(self):
+return '<{} iden={}>'.format(self.__clas__.__name__, self.iden)
+
+def close(self):
+'''Schedule *ff* callback'''
+self.impl.log.debug('callback %d close(), scheduling ff', self.iden)
+self.impl.schedule_ff_callback(self.opaque)
+
+#
+# file descriptors
+#
+
+class Descriptor(object):
+'''Manager of one file descriptor
+
+:param virEventAsyncIOImpl impl: the implementation in which we run
+:param int fd: the file descriptor
+'''
+def __init__(self, impl, fd):
+self.impl = impl
+self.fd = fd
+self.callbacks = {}
+
+def _handle(self, event):
+'''Dispatch the event to the descriptors
+
+:param int event: The event (from libvirt's constants) being dispatched
+'''
+for callback in self.callbacks.values():
+if callback.event is not None and callback.event & event:
+callback.cb(callback.iden, self.fd, event, callback.opaque)
+
+

[libvirt] [PATCH v1 1/2] Allow for ff callbacks to be called by custom event implementations

[Wojtek Porczyk]

The documentation says:
> If the opaque user data requires free'ing when the handle is
> unregistered, then a 2nd callback can be supplied for this purpose.
> This callback needs to be invoked from a clean stack. If 'ff'
> callbacks are invoked directly from the virEventRemoveHandleFunc they
> will likely deadlock in libvirt.

And they did deadlock. In removeTimeout too. Now we supply a custom
function to pick it from the opaque blob and fire.

Signed-off-by: Wojtek Porczyk <w...@invisiblethingslab.com>
---
 libvirt-override.c  | 36 
 libvirt-override.py | 39 +++
 sanitytest.py   |  5 +++--
 3 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 9e40f00..37f7ee2 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -5223,6 +5223,9 @@ libvirt_virEventAddHandleFunc(int fd,
 
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 3, cb_args, cleanup);
 
+/* If changing contents of the opaque object, please also change
+ * virEventExecuteFFCallback() in libvirt-override.py
+ */
 VIR_PY_TUPLE_SET_GOTO(cb_args, 0, libvirt_virEventHandleCallbackWrap(cb), 
cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 1, libvirt_virVoidPtrWrap(opaque), cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 2, libvirt_virFreeCallbackWrap(ff), 
cleanup);
@@ -5292,20 +5295,11 @@ libvirt_virEventRemoveHandleFunc(int watch)
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 0, libvirt_intWrap(watch), cleanup);
 
 result = PyEval_CallObject(removeHandleObj, pyobj_args);
-if (!result) {
+if (result) {
+retval = 0;
+} else {
 PyErr_Print();
 PyErr_Clear();
-} else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
-DEBUG("%s: %s must return opaque obj registered with %s"
-  "to avoid leaking libvirt memory\n",
-  __FUNCTION__, NAME(removeHandle), NAME(addHandle));
-} else {
-opaque = PyTuple_GetItem(result, 1);
-ff = PyTuple_GetItem(result, 2);
-cff = PyvirFreeCallback_Get(ff);
-if (cff)
-(*cff)(PyvirVoidPtr_Get(opaque));
-retval = 0;
 }
 
  cleanup:
@@ -5350,6 +5344,9 @@ libvirt_virEventAddTimeoutFunc(int timeout,
 
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 2, cb_args, cleanup);
 
+/* If changing contents of the opaque object, please also change
+ * virEventExecuteFFCallback() in libvirt-override.py
+ */
 VIR_PY_TUPLE_SET_GOTO(cb_args, 0, libvirt_virEventTimeoutCallbackWrap(cb), 
cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 1, libvirt_virVoidPtrWrap(opaque), cleanup);
 VIR_PY_TUPLE_SET_GOTO(cb_args, 2, libvirt_virFreeCallbackWrap(ff), 
cleanup);
@@ -5416,20 +5413,11 @@ libvirt_virEventRemoveTimeoutFunc(int timer)
 VIR_PY_TUPLE_SET_GOTO(pyobj_args, 0, libvirt_intWrap(timer), cleanup);
 
 result = PyEval_CallObject(removeTimeoutObj, pyobj_args);
-if (!result) {
+if (result) {
+retval = 0;
+} else {
 PyErr_Print();
 PyErr_Clear();
-} else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
-DEBUG("%s: %s must return opaque obj registered with %s"
-  "to avoid leaking libvirt memory\n",
-  __FUNCTION__, NAME(removeTimeout), NAME(addTimeout));
-} else {
-opaque = PyTuple_GetItem(result, 1);
-ff = PyTuple_GetItem(result, 2);
-cff = PyvirFreeCallback_Get(ff);
-if (cff)
-(*cff)(PyvirVoidPtr_Get(opaque));
-retval = 0;
 }
 
  cleanup:
diff --git a/libvirt-override.py b/libvirt-override.py
index 63f8ecb..3d09d63 100644
--- a/libvirt-override.py
+++ b/libvirt-override.py
@@ -16,6 +16,7 @@ except ImportError:
 if str(cyg_e).count("No module named"):
 raise lib_e
 
+import ctypes
 import types
 
 # The root of all libvirt errors.
@@ -211,3 +212,41 @@ def virEventAddTimeout(timeout, cb, opaque):
 ret = libvirtmod.virEventAddTimeout(timeout, cbData)
 if ret == -1: raise libvirtError ('virEventAddTimeout() failed')
 return ret
+
+
+#
+# a caller for the ff callbacks for custom event loop implementations
+#
+
+ctypes.pythonapi.PyCapsule_GetPointer.restype = ctypes.c_void_p
+ctypes.pythonapi.PyCapsule_GetPointer.argtypes = (
+ctypes.py_object, ctypes.c_char_p)
+
+_virFreeCallback = ctypes.CFUNCTYPE(None, ctypes.c_void_p)
+
+def virEventExecuteFFCallback(opaque):
+"""
+Execute callback which frees the opaque buffer
+
+@opaque: the opaque object passed to addHandle or addTimeout
+
+WARNING: This function should not be called from any call by libvirt's
+core. It will most probably cause deadlock in C-level libvirt code.
+Instead it should be scheduled and called from implementation's stack.
+
+See 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFunc
+for mo

[libvirt] [PATCH v1 0/2] libvirt-python: virEventAsyncIOImpl (was: libvirtaio)

[Wojtek Porczyk]

Hello,

As promised, this is the event loop implementation which dispatches the
callbacks via asyncio's event loop.

There are two patches: the first one fixes the bug around freeing opaque
objects [1][2], and the second one is the actual implementation.

Compared to standalone version I did earlier, this has public names more
similar in style to other functions in the module. Also some comments are
corrected, and packaging code is lightly touched.

[1] https://www.redhat.com/archives/libvir-list/2017-January/msg00863.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1433028


Wojtek Porczyk (2):
  Allow for ff callbacks to be called by custom event implementations
  Add asyncio event loop implementation

 libvirt-override.c |  36 ++---
 libvirt-override.py|  45 ++
 libvirt-python.spec.in |   1 +
 libvirtaio.py  | 398 +
 sanitytest.py  |   5 +-
 setup.py   |  12 ++
 6 files changed, 471 insertions(+), 26 deletions(-)
 create mode 100644 libvirtaio.py


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] libvirtaio: libvirt-python asyncio event impl

[Wojtek Porczyk]

On Thu, Mar 16, 2017 at 04:38:23PM +, Daniel P. Berrange wrote:
> On Thu, Mar 16, 2017 at 05:30:40PM +0100, Wojtek Porczyk wrote:
> > Hello, libvirt-list,
> > 
> > I wrote an event implementation wrapping an asyncio event loop [1].
> > I would like to contribute it back to libvirt-python, to be offered
> > alongside the Default Impl in C.
> > 
> > [1] https://github.com/woju/libvirtaio
> > 
> > Also it contains a workaround for a memory leak around the custom loop
> > implementation:
> > https://github.com/woju/libvirtaio/blob/8685cfc/libvirtaio.py#L66-L92
> > 
> > How should I submit this? As a patch against libvirt-python.git, appending 
> > to
> > libvirt-override.py?
> 
> IIUC, the asyncio module you're using is new in python 3.4 ? The libvirt
> python code aims to be compatible with python >= 2.6 at this time, so
> there's a mis-match there.
>
> So if we did include that in the main libvirt python module, we'd have
> to figure out a suitable approach to avoid trying to import the
> non-existant module when people do "import libvirt".

That's right, asyncio (and this module) is compatible with 3.3 (mainly because
of "yield from" syntax) and included in standard library from 3.4. We can ship
it as separate module (like libvirt vs libvirtmod) and do something like this
at the beginning on libvirt-override.py:

try:
from libvirtaio import LibvirtAsyncIOEventImpl  # assuming it won't get 
renamed in review
__all__ += ['LibvirtAsyncIOEventImpl']
except (ImportError, SyntaxError):
pass

(Just tested on 2.7).

People will probably import this unconditionally, since they know, what event
loop they want to run. In case of doubt, they always can:

import libvirt
try:
libvirt.LibvirtAsyncIOEventImpl().register()
except AttributeError:
libvirt.virEventRegisterDefaultImpl()
def event_loop():
while True:
libvirt.virEventRunDefaultImpl()
# ...
import threading
threading.Thread(target=event_loop).start()


execute_ff_callback() can be merged regardless, since it is probably
compatible with 2.5 (that's when ctypes arrived).


> Or we could just leave it as a standalone python module and document its
> existence on our python binding page (http://libvirt.org/python.html)

I'd be glad if you merged this module, since 1) it would be more accessible,
2) won't break between releases (currently it is in danger because of the
opaque object handling). Since we would be (at least now) the only user of
this, I volunteer as tester/patch submitter against this part of code.


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] libvirtaio: libvirt-python asyncio event impl

[Wojtek Porczyk]

Hello, libvirt-list,

I wrote an event implementation wrapping an asyncio event loop [1].
I would like to contribute it back to libvirt-python, to be offered
alongside the Default Impl in C.

[1] https://github.com/woju/libvirtaio

Also it contains a workaround for a memory leak around the custom loop
implementation:
https://github.com/woju/libvirtaio/blob/8685cfc/libvirtaio.py#L66-L92

How should I submit this? As a patch against libvirt-python.git, appending to
libvirt-override.py?

-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] libvirt-python bug: custom event loop impl calls ff callback from *Remove(Handle|Timeout)Func

[Wojtek Porczyk]

On Fri, Feb 03, 2017 at 02:04:57PM +0100, Martin Kletzander wrote:
> Would you mind creating a bug in our bugzilla [1] just so we don't lose
> track of this issue?  Thank you very much in advance.

Sorry for the delay. Here it is:
https://bugzilla.redhat.com/show_bug.cgi?id=1433028

-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] libvirt-python bug: custom event loop impl calls ff callback from *Remove(Handle|Timeout)Func

[Wojtek Porczyk]

On Thu, Jan 19, 2017 at 11:56:36AM +, Daniel P. Berrange wrote:
> On Thu, Jan 19, 2017 at 12:48:11PM +0100, Wojtek Porczyk wrote:
> > Hello libvirt-list,
(snip)
> > > } else {
> > > opaque = PyTuple_GetItem(result, 1);
> > > ff = PyTuple_GetItem(result, 2);
> > > cff = PyvirFreeCallback_Get(ff);
> > > if (cff)
> > > (*cff)(PyvirVoidPtr_Get(opaque));
> > > retval = 0;
> > > }
> > 
> > This is exactly what one shoud not be doing according to documentation [1]:
> > 
> > > If the opaque user data requires free'ing when the handle is unregistered,
> > > then a 2nd callback can be supplied for this purpose. This callback needs 
> > > to
> > > be invoked from a clean stack. If 'ff' callbacks are invoked directly 
> > > from the
> > > virEventRemoveHandleFunc they will likely deadlock in libvirt.
> > 
> > [1] 
> > https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFunc
> > 
(snip)
> 
> Yes, your analysis looks correct to me. I don't think it is easy to fix
> this in the libvirt-override.c file really, unless we were to spawn a throw
> away thread to call "ff" in. The better approach would be for the user suppied
> event loop impl of removeHandle to arrange to call 'ff' themselves at the
> correct time.

IIUC, this cannot be done without involving user-supplied event loop at all.
Unless I miss something (I'm not a C programmer really), that's the
implication of "This callback needs to be invoked from a clean stack", since
libvirt-override.c has access to "clean stack" only via whatever is
implemented in python. 

That unfortunately means the python API (specifically the expected addHandle
signature) should be amended to include explicit ff callback to be called
directly from the loop after removeHandleFunc returns. That's not as bad as
it sounds, because the argument can be optional in Python, so new Python code
could be compatible with both old and new libvirt-python, and
libvirt-override.c could catch TypeError exception to retry the function with
different signature for existing code. That would still work not worse than
present, since it will still leak memory. In any case, the ff callback should
be dropped from libvirt_virEventRemoveHandleFunc(), because again, I suppose
all currently running implementations leak like a sieve.

I have no idea how to write that in C.


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] libvirt-python bug: custom event loop impl calls ff callback from *Remove(Handle|Timeout)Func

[Wojtek Porczyk]

Hello libvirt-list,

As of current libvirt-python.git, according to libvirt-override.c, if
implementing custom event loop in Python, ff callback is called from
libvirt_virEventRemoveHandleFunc, which is a C glue between
virEventRegisterImpl and actual removeHandle function written in Python:

> result = PyEval_CallObject(removeHandleObj, pyobj_args);
> if (!result) {
> PyErr_Print();
> PyErr_Clear();
> } else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
> DEBUG("%s: %s must return opaque obj registered with %s"
>   "to avoid leaking libvirt memory\n",
>   __FUNCTION__, NAME(removeHandle), NAME(addHandle));
> } else {
> opaque = PyTuple_GetItem(result, 1);
> ff = PyTuple_GetItem(result, 2);
> cff = PyvirFreeCallback_Get(ff);
> if (cff)
> (*cff)(PyvirVoidPtr_Get(opaque));
> retval = 0;
> }

This is exactly what one shoud not be doing according to documentation [1]:

> If the opaque user data requires free'ing when the handle is unregistered,
> then a 2nd callback can be supplied for this purpose. This callback needs to
> be invoked from a clean stack. If 'ff' callbacks are invoked directly from the
> virEventRemoveHandleFunc they will likely deadlock in libvirt.

[1] https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFunc

This is true, the deadlock occurs. When the "result" tuple is mangled to have
None as third item ("ff"), then cff = PyvirFreeCallback_Get(ff) is NULL and
the deadlock does not happen.

That's also why script examples/event-test.py does not deadlock, because it
does not return anything (that is, returns None) from Python, so the second if
block happens and the ff callback, if any, is not executed (and probably
something leaks, but I didn't check for that). 


Everything also applies to to timeouts (libvirt_virEventRemoteTimeoutFunc).


-- 
pozdrawiam / best regards   _.-._
Wojtek Porczyk   .-^'   '^-.
Invisible Things Lab |'-.-^-.-'|
 |  |   |  |
 I do not fear computers,|  '-.-'  |
 I fear lack of them.'-._ :  ,-'
-- Isaac Asimov `^-^-_>


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list